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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

See below for a comment.

> 
> --- 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;
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -174,17 +174,20 @@ static void acpi_sleep_prepare(u32 state
>      if ( state != ACPI_STATE_S3 )
>          return;
>  
> -    wakeup_vector_va = __acpi_map_table(
> -        acpi_sinfo.wakeup_vector, sizeof(uint64_t));
> -
>      /* TBoot will set resume vector itself (when it is safe to do so). */
>      if ( tboot_in_measured_env() )
>          return;
>  
> +    set_fixmap(FIX_ACPI_END, acpi_sinfo.wakeup_vector);
> +    wakeup_vector_va = fix_to_virt(FIX_ACPI_END) +
> +                       PAGE_OFFSET(acpi_sinfo.wakeup_vector);
> +
>      if ( acpi_sinfo.vector_width == 32 )
>          *(uint32_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
>      else
>          *(uint64_t *)wakeup_vector_va = bootsym_phys(wakeup_start);
> +
> +    clear_fixmap(FIX_ACPI_END);
>  }
>  
>  static void acpi_sleep_post(u32 state) {}
> @@ -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?

Thanks, Roger.