Re: [PATCH 2/4] xen/arm: acpi: The fixmap area should always be cleared during failure/unmap

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 01/10/2020 01:30, Stefano Stabellini wrote:
> On Sat, 26 Sep 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Commit 022387ee1ad3 "xen/arm: mm: Don't open-code Xen PT update in
>> {set, clear}_fixmap()" enforced that each set_fixmap() should be
>> paired with a clear_fixmap(). Any failure to follow the model would
>> result to a platform crash.
>>
>> Unfortunately, the use of fixmap in the ACPI code was overlooked as it
>> is calling set_fixmap() but not clear_fixmap().
>>
>> The function __acpi_os_map_table() is reworked so:
>>      - We know before the mapping whether the fixmap region is big
>>      enough for the mapping.
>>      - It will fail if the fixmap is always inuse.
> 
> I take you mean "it will fail if the fixmap is *already* in use"?

Yes.

> 
> If so, can it be a problem? Or the expectation is that in practice
> __acpi_os_map_table() will only get called once before SYS_STATE_boot?
> 
> Looking at the code it would seem that even before this patch
> __acpi_os_map_table() wasn't able to handle multiple calls before
> SYS_STATE_boot.

Correct, I am not changing any expectation here. It is only making 
clearer because before commit 022387ee1ad3 we would just overwrite the 
existing mapping with no warning.

After commit 022387ee1ad3, we would just hit the BUG_ON() in set_fixmap().

I will clarify it in the commit message.

[...]

>>   bool __acpi_unmap_table(void *ptr, unsigned long size)
>>   {
>> -    return ( vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) &&
>> -             vaddr < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE) );
>> +    vaddr_t vaddr = (vaddr_t)ptr;
>> +    unsigned int idx;
>> +
>> +    /* We are only handling fixmap address in the arch code */
>> +    if ( vaddr < FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) ||
>> +         vaddr >= FIXMAP_ADDR(FIXMAP_ACPI_END) )
> 
> The "+ PAGE_SIZE" got lost

Hmmm yes.

> 
> 
>> +        return false;
>> +
>> +    /*
>> +     * __acpi_map_table() will always return a pointer in the first page
>> +     * for the ACPI fixmap region. The caller is expected to free with
>> +     * the same address.
>> +     */
>> +    ASSERT((vaddr & PAGE_MASK) == FIXMAP_ADDR(FIXMAP_ACPI_BEGIN));
>> +
>> +    /* The region allocated fit in the ACPI fixmap region. */
>> +    ASSERT(size < (FIXMAP_ADDR(FIXMAP_ACPI_END) + PAGE_SIZE - vaddr));
>> +    ASSERT(fixmap_inuse);
>> +
>> +    fixmap_inuse = false;
>> +
>> +    size += FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) - vaddr;
> 
> Sorry I got confused.. Shouldn't this be:
> 
>    size += vaddr - FIXMAP_ADDR(FIXMAP_ACPI_BEGIN);
> 
> ?

It should be. :) I guess this was unoticed because vaddr == 
FIXMAP_ADDR(FIXMAP_ACPI_BEGIN) in my testing.

I will fix it.

> 
> 
>> +    idx = FIXMAP_ACPI_BEGIN;
>> +
>> +    do
>> +    {
>> +        clear_fixmap(idx);
>> +        size -= min(size, (unsigned long)PAGE_SIZE);
>> +        idx++;
>> +    } while ( size > 0 );
>> +
>> +    return true;
>>   }
>>   
>>   /* True to indicate PSCI 0.2+ is implemented */
>> -- 
>> 2.17.1
>>

Cheers,

-- 
Julien Grall