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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/11/2020 16:07, Roger Pau Monné wrote:
> On Mon, Nov 23, 2020 at 04:30:05PM +0100, Jan Beulich wrote:
>> On 23.11.2020 16:24, Roger Pau Monné wrote:
>>> On Mon, Nov 23, 2020 at 01:40:12PM +0100, Jan Beulich wrote:
>>>> --- 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);
>>> Why not use vmap here instead of the fixmap?
>> Considering the S3 path is relatively fragile (as in: we end up
>> breaking it more often than about anything else) I wanted to
>> make as little of a change as possible. Hence I decided to stick
>> to the fixmap use that was (indirectly) used before as well.
> Unless there's a restriction to use the ACPI fixmap entry I would just
> switch to use vmap, as it's used extensively in the code and less
> likely to trigger issues in the future, or else a bunch of other stuff
> would also be broken.
>
> IMO doing the mapping differently here when it's not required will end
> up turning this code more fragile in the long run.

We can't enter S3 at all until dom0 has booted, as one detail has to
come from AML.

Therefore, we're fully up and running by this point, and vmap() will be
fine.

However, why are we re-writing the wakeup vector every time?  Its fixed
by the position of the trampoline, so we'd actually simplify the S3 path
by only setting it up once.

~Andrew

(The fix for fragility is to actually test it, not shy away from making
any change)