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

[Jan Beulich <jbeulich@suse.com>]

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 ...".

>>> +{
>>> +	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?

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

Well, I said "slightly odd" as sort of a replacement of "Nit:".
But I really think it would be more logical the other way
around, not so much for how it looks like, but to aid the not
uncommon compiler heuristics of assuming the "true" path is
the common one.

Jan