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
