lkml.org 
[lkml]   [2021]   [Nov]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V3 4/6] xen/unpopulated-alloc: Add mechanism to use Xen resource
From
Date

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@epam.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@epam.com>
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.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

\
 
 \ /
  Last update: 2021-11-25 15:05    [W:0.121 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site