Re: [PATCH V2 2/4] arm/xen: Switch to use gnttab_setup_auto_xlat_frames() for DT

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
> 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


> +#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) {


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