[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |