Messages in this thread | | | Subject | Re: [PATCH V3 4/6] xen/unpopulated-alloc: Add mechanism to use Xen resource | From | Oleksandr <> | Date | Thu, 9 Dec 2021 02:04:09 +0200 |
| |
On 08.12.21 01:36, Stefano Stabellini wrote:
Hi Stefano
> On Thu, 25 Nov 2021, Oleksandr wrote: >>>> Please note the following: >>>> for V3 arch_xen_unpopulated_init() was moved to init() as was agreed >>>> and gained __init specifier. So the target_resource is initialized there. >>>> >>>> With current patch series applied if CONFIG_XEN_UNPOPULATED_ALLOC >>>> is enabled: >>>> >>>> 1. On Arm, under normal circumstances, the xen_alloc_unpopulated_pages() >>>> won't be called “before” arch_xen_unpopulated_init(). It will only be >>>> called "before" when either ACPI is in use or something wrong happened >>>> with DT (and we failed to read xen_grant_frames), so we fallback to >>>> xen_xlate_map_ballooned_pages() in arm/xen/enlighten.c:xen_guest_init(), >>>> please see "arm/xen: Switch to use gnttab_setup_auto_xlat_frames() for DT" >>>> for details. But in that case, I think, it doesn't matter much whether >>>> xen_alloc_unpopulated_pages() is called "before" of "after" >>>> target_resource >>>> initialization, as we don't have extended regions in place the >>>> target_resource >>>> will remain invalid even after initialization, so >>>> xen_alloc_ballooned_pages() >>>> will be used in both scenarios. >>>> >>>> 2. On x86, I am not quite sure which modes use unpopulated-alloc (PVH?), >>>> but it looks like xen_alloc_unpopulated_pages() can (and will) be called >>>> “before” arch_xen_unpopulated_init(). >>>> At least, I see that xen_xlate_map_ballooned_pages() is called in >>>> x86/xen/grant-table.c:xen_pvh_gnttab_setup(). According to the initcall >>>> levels for both xen_pvh_gnttab_setup() and init() I expect the former >>>> to be called earlier. >>>> If it is true, the sentence in the commit description which mentions >>>> that “behaviour on x86 is not changed” is not precise. I don’t think >>>> it would be correct to fallback to xen_alloc_ballooned_pages() just >>>> because we haven’t initialized target_resource yet (on x86 it is just >>>> assigning it iomem_resource), at least this doesn't look like an expected >>>> behaviour and unlikely would be welcome. >>>> >>>> I am wondering whether it would be better to move >>>> arch_xen_unpopulated_init() >>>> to a dedicated init() marked with an appropriate initcall level >>>> (early_initcall?) >>>> to make sure it will always be called *before* >>>> xen_xlate_map_ballooned_pages(). >>>> What do you think? >> ... here (#2). Or I really missed something and there wouldn't be an issue? > Yes, I see your point. Yeah, it makes sense to make sure that > drivers/xen/unpopulated-alloc.c:init is executed before > xen_pvh_gnttab_setup. > > If we move it to early_initcall, then we end up running it before > xen_guest_init on ARM. But that might be fine: it looks like it should > work OK and would also allow us to execute xen_xlate_map_ballooned_pages > with target_resource already set. > > So I'd say go for it :)
Thank you for the confirmation! In order to be on the safe side, I would probably leave drivers/xen/unpopulated-alloc.c:init as is, I mean with current subsys initcall level (it expects the extra memory regions to be already filled) and create a separate unpopulated_init() to put arch_xen_unpopulated_init() into. Something like the following:
static int __init unpopulated_init(void) { int ret;
if (!xen_domain()) return -ENODEV;
ret = arch_xen_unpopulated_init(&target_resource); if (ret) { pr_err("xen:unpopulated: Cannot initialize target resource\n"); target_resource = NULL; }
return ret; } early_initcall(unpopulated_init);
-- Regards,
Oleksandr Tyshchenko
| |