[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.