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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 28/09/2020 09:18, Jan Beulich wrote:
> 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.
I will drop the stray blanks.

> > --- 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.
How about "No arch specific implementation after early boot but for the 
first 1MB"?
> > @@ -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 *"?
It might be possible, I will have a look.

> > +{
> > +       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.
This is Linux coding style and therefore is using hard tab. Care to 
explain the problem?
> 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.
I will add extra parentheses in the if.

> > --- 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()?
I was looking for a macro to avoid open-coding but I couldn't find it. I 
will use it in the next version.
> > +       /* 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.
Will do.

> > +       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,
I don't really see the problem. Are you nitpicking?

> the more that it's minimally shorter:
>
>         return ptr ? ptr + offs : NULL;
Cheers,

--
Julien Grall