Hi Igor and Thomas, Thank you for your review! On 1/19/23 1:12 PM, Thomas Gleixner wrote: > On Mon, Jan 16 2023 at 15:55, Igor Mammedov wrote: >> "Srivatsa S. Bhat" <srivatsa@xxxxxxxxxxxxx> wrote: >>> Fix this by preventing the use of mwait idle state in the vCPU offline >>> play_dead() path for any hypervisor, even if mwait support is >>> available. >> >> if mwait is enabled, it's very likely guest to have cpuidle >> enabled and using the same mwait as well. So exiting early from >> mwait_play_dead(), might just punt workflow down: >> native_play_dead() >> ... >> mwait_play_dead(); >> if (cpuidle_play_dead()) <- possible mwait here >> hlt_play_dead(); >> >> and it will end up in mwait again and only if that fails >> it will go HLT route and maybe transition to VMM. > > Good point. > >> Instead of workaround on guest side, >> shouldn't hypervisor force VMEXIT on being uplugged vCPU when it's >> actually hot-unplugging vCPU? (ex: QEMU kicks vCPU out from guest >> context when it is removing vCPU, among other things) > > For a pure guest side CPU unplug operation: > > guest$ echo 0 >/sys/devices/system/cpu/cpu$N/online > > the hypervisor is not involved at all. The vCPU is not removed in that > case. > Agreed, and this is indeed the scenario I was targeting with this patch, as opposed to vCPU removal from the host side. I'll add this clarification to the commit message. > So to ensure that this ends up in HLT something like the below is > required. > > Note, the removal of the comment after mwait_play_dead() is intentional > because the comment is completely bogus. Not having MWAIT is not a > failure. But that wants to be a seperate patch. > Sounds good, will do and post a new version. Thank you! Regards, Srivatsa VMware Photon OS > Thanks, > > tglx > --- > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c > index 55cad72715d9..3f1f20f71ec5 100644 > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1833,7 +1833,10 @@ void native_play_dead(void) > play_dead_common(); > tboot_shutdown(TB_SHUTDOWN_WFS); > > - mwait_play_dead(); /* Only returns on failure */ > + if (this_cpu_has(X86_FEATURE_HYPERVISOR)) > + hlt_play_dead(); > + > + mwait_play_dead(); > if (cpuidle_play_dead()) > hlt_play_dead(); > } > > > >
