Message-ID: <c2e8c00a-3856-8330-8e8f-ab8a92e93e47@gmail.com>
Date:   Thu, 25 Nov 2021 16:01:31 +0200
From:   Oleksandr <olekstysh@...il.com>
To:     Stefano Stabellini <sstabellini@...nel.org>
Cc:     xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>, Julien Grall <julien@....org>
Subject: Re: [PATCH V3 4/6] xen/unpopulated-alloc: Add mechanism to use Xen
 resource


On 25.11.21 03:03, Stefano Stabellini wrote:

Hi Stefano, all

> On Wed, 24 Nov 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>
>>
>> The main reason of this change is that unpopulated-alloc
>> code cannot be used in its current form on Arm, but there
>> is a desire to reuse it to avoid wasting real RAM pages
>> for the grant/foreign mappings.
>>
>> The problem is that system "iomem_resource" is used for
>> the address space allocation, but the really unallocated
>> space can't be figured out precisely by the domain on Arm
>> without hypervisor involvement. For example, not all device
>> I/O regions are known by the time domain starts creating
>> grant/foreign mappings. And following the advise from
>> "iomem_resource" we might end up reusing these regions by
>> a mistake. So, the hypervisor which maintains the P2M for
>> the domain is in the best position to provide unused regions
>> of guest physical address space which could be safely used
>> to create grant/foreign mappings.
>>
>> Introduce new helper arch_xen_unpopulated_init() which purpose
>> is to create specific Xen resource based on the memory regions
>> provided by the hypervisor to be used as unused space for Xen
>> scratch pages. If arch doesn't define arch_xen_unpopulated_init()
>> the default "iomem_resource" will be used.
>>
>> Update the arguments list of allocate_resource() in fill_list()
>> to always allocate a region from the hotpluggable range
>> (maximum possible addressable physical memory range for which
>> the linear mapping could be created). If arch doesn't define
>> arch_get_mappable_range() the default range (0,-1) will be used.
>>
>> The behaviour on x86 won't be changed by current patch as both
>> arch_xen_unpopulated_init() and arch_get_mappable_range()
>> are not implemented for it.
>>
>> Also fallback to allocate xenballooned pages (balloon out RAM
>> pages) if we do not have any suitable resource to work with
>> (target_resource is invalid) and as the result we won't be able
>> to provide unpopulated pages on a request.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>
>
> Reviewed-by: Stefano Stabellini <sstabellini@...nel.org>

Thanks!


What still worries me is a concern with x86's xen_pvh_gnttab_setup() I 
described in post commit remark ...


>
>
>> ---
>> 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?


>>
>> Changes RFC -> V2:
>>     - new patch, instead of
>>      "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to provide unallocated space"
>>
>> Changes V2 -> V3:
>>     - update patch description and comments in code
>>     - modify arch_xen_unpopulated_init() to pass target_resource as an argument
>>       and update default helper to assign iomem_resource to it, also drop
>>       xen_resource as it will be located in arch code in the future
>>     - allocate region from hotpluggable range instead of hardcoded range (0,-1)
>>       in fill_list()
>>     - use %pR specifier in error message
>>     - do not call unpopulated_init() at runtime from xen_alloc_unpopulated_pages(),
>>       drop an extra helper and call arch_xen_unpopulated_init() directly from __init()
>>     - include linux/ioport.h instead of forward declaration of struct resource
>>     - replace insert_resource() with request_resource() in fill_list()
>>     - add __init specifier to arch_xen_unpopulated_init()
>> ---
>>   drivers/xen/unpopulated-alloc.c | 83 +++++++++++++++++++++++++++++++++++++----
>>   include/xen/xen.h               |  2 +
>>   2 files changed, 78 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/xen/unpopulated-alloc.c b/drivers/xen/unpopulated-alloc.c
>> index a03dc5b..07d3578 100644
>> --- a/drivers/xen/unpopulated-alloc.c
>> +++ b/drivers/xen/unpopulated-alloc.c
>> @@ -8,6 +8,7 @@
>>   
>>   #include <asm/page.h>
>>   
>> +#include <xen/balloon.h>
>>   #include <xen/page.h>
>>   #include <xen/xen.h>
>>   
>> @@ -15,13 +16,29 @@ static DEFINE_MUTEX(list_lock);
>>   static struct page *page_list;
>>   static unsigned int list_count;
>>   
>> +static struct resource *target_resource;
>> +
>> +/*
>> + * If arch is not happy with system "iomem_resource" being used for
>> + * the region allocation it can provide it's own view by creating specific
>> + * Xen resource with unused regions of guest physical address space provided
>> + * by the hypervisor.
>> + */
>> +int __weak __init arch_xen_unpopulated_init(struct resource **res)
>> +{
>> +	*res = &iomem_resource;
>> +
>> +	return 0;
>> +}
>> +
>>   static int fill_list(unsigned int nr_pages)
>>   {
>>   	struct dev_pagemap *pgmap;
>> -	struct resource *res;
>> +	struct resource *res, *tmp_res = NULL;
>>   	void *vaddr;
>>   	unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
>> -	int ret = -ENOMEM;
>> +	struct range mhp_range;
>> +	int ret;
>>   
>>   	res = kzalloc(sizeof(*res), GFP_KERNEL);
>>   	if (!res)
>> @@ -30,14 +47,40 @@ static int fill_list(unsigned int nr_pages)
>>   	res->name = "Xen scratch";
>>   	res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>>   
>> -	ret = allocate_resource(&iomem_resource, res,
>> -				alloc_pages * PAGE_SIZE, 0, -1,
>> +	mhp_range = mhp_get_pluggable_range(true);
>> +
>> +	ret = allocate_resource(target_resource, res,
>> +				alloc_pages * PAGE_SIZE, mhp_range.start, mhp_range.end,
>>   				PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);
>>   	if (ret < 0) {
>>   		pr_err("Cannot allocate new IOMEM resource\n");
>>   		goto err_resource;
>>   	}
>>   
>> +	/*
>> +	 * Reserve the region previously allocated from Xen resource to avoid
>> +	 * re-using it by someone else.
>> +	 */
>> +	if (target_resource != &iomem_resource) {
>> +		tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
>> +		if (!res) {
>> +			ret = -ENOMEM;
>> +			goto err_insert;
>> +		}
>> +
>> +		tmp_res->name = res->name;
>> +		tmp_res->start = res->start;
>> +		tmp_res->end = res->end;
>> +		tmp_res->flags = res->flags;
>> +
>> +		ret = request_resource(&iomem_resource, tmp_res);
>> +		if (ret < 0) {
>> +			pr_err("Cannot request resource %pR (%d)\n", tmp_res, ret);
>> +			kfree(tmp_res);
>> +			goto err_insert;
>> +		}
>> +	}
>> +
>>   	pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
>>   	if (!pgmap) {
>>   		ret = -ENOMEM;
>> @@ -95,6 +138,11 @@ static int fill_list(unsigned int nr_pages)
>>   err_memremap:
>>   	kfree(pgmap);
>>   err_pgmap:
>> +	if (tmp_res) {
>> +		release_resource(tmp_res);
>> +		kfree(tmp_res);
>> +	}
>> +err_insert:
>>   	release_resource(res);
>>   err_resource:
>>   	kfree(res);
>> @@ -112,6 +160,14 @@ int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
>>   	unsigned int i;
>>   	int ret = 0;
>>   
>> +	/*
>> +	 * Fallback to default behavior if we do not have any suitable resource
>> +	 * to allocate required region from and as the result we won't be able to
>> +	 * construct pages.
>> +	 */
>> +	if (!target_resource)
>> +		return xen_alloc_ballooned_pages(nr_pages, pages);
>> +
>>   	mutex_lock(&list_lock);
>>   	if (list_count < nr_pages) {
>>   		ret = fill_list(nr_pages - list_count);
>> @@ -159,6 +215,11 @@ void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages)
>>   {
>>   	unsigned int i;
>>   
>> +	if (!target_resource) {
>> +		xen_free_ballooned_pages(nr_pages, pages);
>> +		return;
>> +	}
>> +
>>   	mutex_lock(&list_lock);
>>   	for (i = 0; i < nr_pages; i++) {
>>   		pages[i]->zone_device_data = page_list;
>> @@ -169,9 +230,11 @@ void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages)
>>   }
>>   EXPORT_SYMBOL(xen_free_unpopulated_pages);
>>   
>> -#ifdef CONFIG_XEN_PV
>>   static int __init init(void)
>>   {
>> +	int ret;
>> +
>> +#ifdef CONFIG_XEN_PV
>>   	unsigned int i;
>>   
>>   	if (!xen_domain())
>> @@ -196,8 +259,14 @@ static int __init init(void)
>>   			list_count++;
>>   		}
>>   	}
>> +#endif
>>   
>> -	return 0;
>> +	ret = arch_xen_unpopulated_init(&target_resource);
>> +	if (ret) {
>> +		pr_err("xen:unpopulated: Cannot initialize target resource\n");
>> +		target_resource = NULL;
>> +	}
>> +
>> +	return ret;
>>   }
>>   subsys_initcall(init);
>> -#endif
>> diff --git a/include/xen/xen.h b/include/xen/xen.h
>> index 86c5b37..a99bab8 100644
>> --- a/include/xen/xen.h
>> +++ b/include/xen/xen.h
>> @@ -55,6 +55,8 @@ extern u64 xen_saved_max_mem_size;
>>   #ifdef CONFIG_XEN_UNPOPULATED_ALLOC
>>   int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages);
>>   void xen_free_unpopulated_pages(unsigned int nr_pages, struct page **pages);
>> +#include <linux/ioport.h>
>> +int arch_xen_unpopulated_init(struct resource **res);
>>   #else
>>   #include <xen/balloon.h>
>>   static inline int xen_alloc_unpopulated_pages(unsigned int nr_pages,
>> -- 
>> 2.7.4

-- 
Regards,

Oleksandr Tyshchenko

Powered by blists - more mailing lists