Re: [PATCH 2/4] x86/ACPI: fix S3 wakeup vector mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.12.2020 11:51, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>> Use of __acpi_map_table() here was at least close to an abuse already
>> before, but it will now consistently return NULL here. Drop the layering
>> violation and use set_fixmap() directly. Re-use of the ACPI fixmap area
>> is hopefully going to remain "fine" for the time being.
>>
>> Add checks to acpi_enter_sleep(): The vector now needs to be contained
>> within a single page, but the ACPI spec requires 64-byte alignment of
>> FACS anyway. Also bail if no wakeup vector was determined in the first
>> place, in part as preparation for a subsequent relaxation change.
>>
>> Fixes: 1c4aa69ca1e1 ("xen/acpi: Rework acpi_os_map_memory() and 
>> acpi_os_unmap_memory()")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

> See below for a comment.

For this please also note ...

>> --- a/xen/arch/x86/acpi/boot.c
>> +++ b/xen/arch/x86/acpi/boot.c
>> @@ -443,6 +443,11 @@ acpi_fadt_parse_sleep_info(struct acpi_t
>>                      "FACS is shorter than ACPI spec allow: %#x",
>>                      facs->length);
>>  
>> +    if (facs_pa % 64)
>> +            printk(KERN_WARNING PREFIX
>> +                    "FACS is not 64-byte aligned: %#lx",
>> +                    facs_pa);
>> +
>>      acpi_sinfo.wakeup_vector = facs_pa + 
>>              offsetof(struct acpi_table_facs, firmware_waking_vector);
>>      acpi_sinfo.vector_width = 32;

... the printk() getting added here. Violation of the spec here
implies entering S3 may fail because of ...

>> @@ -331,6 +334,12 @@ static long enter_state_helper(void *dat
>>   */
>>  int acpi_enter_sleep(struct xenpf_enter_acpi_sleep *sleep)
>>  {
>> +    if ( sleep->sleep_state == ACPI_STATE_S3 &&
>> +         (!acpi_sinfo.wakeup_vector || !acpi_sinfo.vector_width ||
>> +          (PAGE_OFFSET(acpi_sinfo.wakeup_vector) >
>> +           PAGE_SIZE - acpi_sinfo.vector_width / 8)) )
> 
> Shouldn't this last check better be done in acpi_fadt_parse_sleep_info
> and then avoid setting wakeup_vector in the first place?

... the check you talk about here, albeit these are independent
aspects: The spec requires even more strict alignment, and what
gets checked here is merely a precondition for the specific
implementation of ours, not tolerating the storage for the
vector to cross a page boundary. As such, I consider it more
appropriate for the check to live here, but yes, it could in
principle also be put there.

Jan