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

[Julien Grall <julien@xen.org>]

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

I will force the indentation manually.

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

Ah, you are trying to outsmart the compilers again...

Cheers,

-- 
Julien Grall