Re: [PATCH 1/4] xen/acpi: Rework acpi_os_map_memory() and acpi_os_unmap_memory()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2020 22:55, Julien Grall wrote:
> @@ -49,6 +53,12 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>      return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)
> +{
> +    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
> +             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );

Nit: If this wasn't Arm code, I'd ask for the parentheses to be
dropped altogether, but at least the stray blanks should go away
imo.

> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -46,6 +46,10 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>       if ((phys + size) <= (1 * 1024 * 1024))
>               return __va(phys);
>  
> +     /* No arch specific implementation after early boot */
> +     if (system_state >= SYS_STATE_boot)
> +             return NULL;

Considering the code in context above, the comment isn't entirely
correct.

> @@ -66,6 +70,20 @@ char *__acpi_map_table(paddr_t phys, unsigned long size)
>       return ((char *) base + offset);
>  }
>  
> +bool __acpi_unmap_table(void *ptr, unsigned long size)

Is there anything standing in the way of making ptr "const void *"?

> +{
> +     unsigned long vaddr = (unsigned long)ptr;
> +
> +     if (vaddr >= DIRECTMAP_VIRT_START &&
> +         vaddr < DIRECTMAP_VIRT_END) {
> +             ASSERT(!((__pa(ptr) + size - 1) >> 20));
> +             return true;
> +     }
> +
> +     return (vaddr >= __fix_to_virt(FIX_ACPI_END)) &&
> +             (vaddr < (__fix_to_virt(FIX_ACPI_BEGIN) + PAGE_SIZE));

Indentation is slightly wrong here. Also please consider reducing
the number of parentheses here; at the very least the return and
the earlier if() want to be consistent in when ones are(n't) used.

> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -92,27 +92,29 @@ acpi_physical_address __init 
> acpi_os_get_root_pointer(void)
>  void __iomem *
>  acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>  {
> -     if (system_state >= SYS_STATE_boot) {
> -             mfn_t mfn = _mfn(PFN_DOWN(phys));
> -             unsigned int offs = phys & (PAGE_SIZE - 1);
> -
> -             /* The low first Mb is always mapped on x86. */
> -             if (IS_ENABLED(CONFIG_X86) && !((phys + size - 1) >> 20))
> -                     return __va(phys);
> -             return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> -                           ACPI_MAP_MEM_ATTR, VMAP_DEFAULT) + offs;
> -     }
> -     return __acpi_map_table(phys, size);
> +     void *ptr;
> +     mfn_t mfn = _mfn(PFN_DOWN(phys));
> +     unsigned int offs = phys & (PAGE_SIZE - 1);

Open-coding PAGE_OFFSET()?

> +     /* Try the arch specific implementation first */
> +     ptr = __acpi_map_table(phys, size);
> +     if (ptr)
> +             return ptr;
> +
> +     /* No common implementation for early boot map */
> +     if (unlikely(system_state < SYS_STATE_boot))
> +          return NULL;

Consistently hard tabs for indentation here, please.

> +     ptr = __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> +                  ACPI_MAP_MEM_ATTR, VMAP_DEFAULT);
> +
> +     return !ptr ? NULL : (ptr + offs);

Slightly odd that you don't let the success case go first, the
more that it's minimally shorter:

        return ptr ? ptr + offs : NULL;

Jan