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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.11.2020 14:02, Jan Beulich wrote:
> On 24.11.2020 12:04, Jan Beulich wrote:
>> On 23.11.2020 17:14, Andrew Cooper wrote:
>>> 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.
>>
>> That's not the point of my reservation. The code here runs when the
>> system already isn't "fully up and running" anymore. Secondary CPUs
>> have already been offlined, and we're around the point where we
>> disable interrupts. Granted when we disable them, we also turn off
>> spin debugging, but I'd still prefer a path that's not susceptible
>> to IRQ state. What I admit I didn't pay attention to is that
>> set_fixmap(), by virtue of being a thin wrapper around
>> map_pages_to_xen(), similarly uses locks. IOW - okay, I'll switch
>> to vmap(). You're both aware that it, unlike set_fixmap(), can
>> fail, aren't you?
> 
> Would at least one of the two of you please explicitly reply to
> this last question, clarifying that you're indeed okay with this
> new possible source of S3 entry failing?

I think we want to get this regression addressed, but without the
explicit consent of at least one of you that introducing a new
error source to the S3 path is indeed okay I'd prefer not to
prepare and then send v2. I expect there's going to be some code
churn (not the least because acpi_sleep_prepare() currently
returns void), and I'd rather avoid doing the conversion work
just to then be told to go back to the previous approach.

Jan