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

[Julien Grall <julien@xen.org>]

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