Messages in this thread | | | Subject | Re: [PATCH V2 2/4] arm/xen: Switch to use gnttab_setup_auto_xlat_frames() for DT | From | Oleksandr <> | Date | Fri, 19 Nov 2021 20:25:16 +0200 |
| |
On 19.11.21 02:32, Stefano Stabellini wrote:
Hi Stefano
> On Thu, 11 Nov 2021, Oleksandr wrote: >> On 28.10.21 04:28, Stefano Stabellini wrote: >> >> Hi Stefano >> >> I am sorry for the late response. >> >>> On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Read the start address of the grant table space from DT >>>> (region 0). >>>> >>>> This patch mostly restores behaviour before commit 3cf4095d7446 >>>> ("arm/xen: Use xen_xlate_map_ballooned_pages to setup grant table") >>>> but trying not to break the ACPI support added after that commit. >>>> So the patch touches DT part only and leaves the ACPI part with >>>> xen_xlate_map_ballooned_pages(). >>>> >>>> This is a preparation for using Xen extended region feature >>>> where unused regions of guest physical address space (provided >>>> by the hypervisor) will be used to create grant/foreign/whatever >>>> mappings instead of wasting real RAM pages from the domain memory >>>> for establishing these mappings. >>>> >>>> The immediate benefit of this change: >>>> - Avoid superpage shattering in Xen P2M when establishing >>>> stage-2 mapping (GFN <-> MFN) for the grant table space >>>> - Avoid wasting real RAM pages (reducing the amount of memory >>>> usuable) for mapping grant table space >>>> - The grant table space is always mapped at the exact >>>> same place (region 0 is reserved for the grant table) >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> Changes RFC -> V2: >>>> - new patch >>>> --- >>>> arch/arm/xen/enlighten.c | 32 +++++++++++++++++++++++++------- >>>> 1 file changed, 25 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c >>>> index 7f1c106b..dea46ec 100644 >>>> --- a/arch/arm/xen/enlighten.c >>>> +++ b/arch/arm/xen/enlighten.c >>>> @@ -59,6 +59,9 @@ unsigned long xen_released_pages; >>>> struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS] >>>> __initdata; >>>> static __read_mostly unsigned int xen_events_irq; >>>> +static phys_addr_t xen_grant_frames; >>> __read_mostly >> ok >> >> >>> >>>> +#define GRANT_TABLE_INDEX 0 >>>> uint32_t xen_start_flags; >>>> EXPORT_SYMBOL(xen_start_flags); >>>> @@ -303,6 +306,7 @@ static void __init xen_acpi_guest_init(void) >>>> static void __init xen_dt_guest_init(void) >>>> { >>>> struct device_node *xen_node; >>>> + struct resource res; >>>> xen_node = of_find_compatible_node(NULL, NULL, "xen,xen"); >>>> if (!xen_node) { >>>> @@ -310,6 +314,12 @@ static void __init xen_dt_guest_init(void) >>>> return; >>>> } >>>> + if (of_address_to_resource(xen_node, GRANT_TABLE_INDEX, &res)) { >>>> + pr_err("Xen grant table region is not found\n"); >>>> + return; >>>> + } >>>> + xen_grant_frames = res.start; >>>> + >>>> xen_events_irq = irq_of_parse_and_map(xen_node, 0); >>>> } >>>> @@ -317,16 +327,20 @@ static int __init xen_guest_init(void) >>>> { >>>> struct xen_add_to_physmap xatp; >>>> struct shared_info *shared_info_page = NULL; >>>> - int cpu; >>>> + int rc, cpu; >>>> if (!xen_domain()) >>>> return 0; >>>> if (!acpi_disabled) >>>> xen_acpi_guest_init(); >>>> - else >>>> + else { >>>> xen_dt_guest_init(); >>>> + if (!xen_grant_frames) >>>> + return -ENODEV; >>> maybe we can avoid this, see below >>> >>> >>>> + } >>>> + >>>> if (!xen_events_irq) { >>>> pr_err("Xen event channel interrupt not found\n"); >>>> return -ENODEV; >>>> @@ -370,12 +384,16 @@ static int __init xen_guest_init(void) >>>> for_each_possible_cpu(cpu) >>>> per_cpu(xen_vcpu_id, cpu) = cpu; >>>> - xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames(); >>>> - if (xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn, >>>> - &xen_auto_xlat_grant_frames.vaddr, >>>> - xen_auto_xlat_grant_frames.count)) { >>>> + if (!acpi_disabled) { >>> To make the code more resilient couldn't we do: >>> >>> if (!acpi_disabled || !xen_grant_frames) { >> I think, we can. >> >> On the one hand, indeed the code more resilient and less change. >> From the other hand if grant table region is not found then something weird >> happened as region 0 is always present in reg property if hypervisor node is >> exposed to the guest. >> The behavior before commit 3cf4095d7446 ("arm/xen: Use >> xen_xlate_map_ballooned_pages to setup grant table") was exactly the same in >> the context of the failure if region wasn't found. >> >> ... >> >> Well, if we want to make code more resilient, I will update. But, looks like >> we also need to switch actions in xen_dt_guest_init() in order to process >> xen_events_irq before xen_grant_frames, otherwise we may return after failing >> with region and end up not initializing xen_events_irq so xen_guest_init() >> will fail earlier than reaches that check. >> What do you think? > > Yes, you are right. I was re-reading the patch to refresh my memory and > I noticed immediately that xen_dt_guest_init also need to be changed so > that xen_events_irq is set before xen_grant_frames. > > I think it is a minor change that doesn't add complexity but make the > code more robust so I think it is a good idea
Great, thank you. Will do in next version.
> > >>>> + xen_auto_xlat_grant_frames.count = gnttab_max_grant_frames(); >>>> + rc = >>>> xen_xlate_map_ballooned_pages(&xen_auto_xlat_grant_frames.pfn, >>>> + >>>> &xen_auto_xlat_grant_frames.vaddr, >>>> + >>>> xen_auto_xlat_grant_frames.count); >>>> + } else >>>> + rc = gnttab_setup_auto_xlat_frames(xen_grant_frames); >>>> + if (rc) { >>>> free_percpu(xen_vcpu_info); >>>> - return -ENOMEM; >>>> + return rc; >>>> } >>>> gnttab_init();
-- Regards,
Oleksandr Tyshchenko
| |