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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 28/09/2020 11:39, Julien Grall wrote:
> On 28/09/2020 11:09, Jan Beulich wrote:
> > On 28.09.2020 11:58, Julien Grall wrote:
> > > On 28/09/2020 09:18, Jan Beulich wrote:
> > > > On 26.09.2020 22:55, Julien Grall wrote:
> > > > > --- 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"?
> > That or simply "No further ...".
> I will do that.
>
> > > > > +{
> > > > > +    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?
> > The two opening parentheses should align with one another,
> > shouldn't they?
> Hmmm... somehow vim wants to indent this way. I am not entirely sure why...
Looking at the Linux codebase this is the expected indentation. This is 
because the first ( on the first line is not closed and until the last ) 
on the second line.
So I will stick with this code.

Cheers,

--
Julien Grall