[V3,4/6] xen/unpopulated-alloc: Add mechanism to use Xen resource

Commit Message

Oleksandr Tyshchenko Nov. 24, 2021, 8:53 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

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 <[e-mail]>
---
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?

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(-)

Comments

Stefano Stabellini Nov. 25, 2021, 1:03 a.m. UTC | #1

On Wed, 24 Nov 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> 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 <[e-mail]>


Reviewed-by: Stefano Stabellini <[e-mail]>


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

Oleksandr Tyshchenko Nov. 25, 2021, 2:01 p.m. UTC | #2

On 25.11.21 03:03, Stefano Stabellini wrote:

Hi Stefano, all

> On Wed, 24 Nov 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> 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 <[e-mail]>
>
> Reviewed-by: Stefano Stabellini <[e-mail]>

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

Boris Ostrovsky Nov. 26, 2021, 3:17 p.m. UTC | #3

On 11/24/21 3:53 PM, Oleksandr Tyshchenko wrote:
> +	if (target_resource != &iomem_resource) {
> +		tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
> +		if (!res) {


If (!tmp_res)


> +			ret = -ENOMEM;
> +			goto err_insert;
> +		}

Oleksandr Tyshchenko Nov. 26, 2021, 3:23 p.m. UTC | #4

On 26.11.21 17:17, Boris Ostrovsky wrote:

Hi Boris


>
> On 11/24/21 3:53 PM, Oleksandr Tyshchenko wrote:
>> +    if (target_resource != &iomem_resource) {
>> +        tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
>> +        if (!res) {
>
>
> If (!tmp_res)


Good catch, thank you!


>
>
>> +            ret = -ENOMEM;
>> +            goto err_insert;
>> +        }

Stefano Stabellini Dec. 7, 2021, 11:36 p.m. UTC | #5

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 :)

Oleksandr Tyshchenko Dec. 9, 2021, 12:04 a.m. UTC | #6

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);

Stefano Stabellini Dec. 9, 2021, 12:59 a.m. UTC | #7

On Thu, 9 Dec 2021, Oleksandr wrote:
> 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);

Sounds good

Patch

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,