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 <jbeul...@suse.com>
>
> Acked-by: Roger Pau Monné <roger....@citrix.com>
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
