Messages in this thread | | | Date | Thu, 23 Sep 2021 20:09:48 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v7 08/12] x86/tdx: Add HLT support for TDX guest |
| |
On Thu, Sep 16, 2021 at 11:35:46AM -0700, Kuppuswamy Sathyanarayanan wrote: > +static __cpuidle void _tdx_halt(const bool irq_disabled, const bool do_sti) > +{ > + u64 ret; > + > + /* > + * Emulate HLT operation via hypercall. More info about ABI > + * can be found in TDX Guest-Host-Communication Interface > + * (GHCI), sec 3.8.
"3.8 TDG.VP.VMCALL<Instruction.HLT>"
write that section name because those numbers do change.
> + * > + * The VMM uses the "IRQ disabled" param to understand IRQ > + * enabled status (RFLAGS.IF) of TD guest and determine > + * whether or not it should schedule the halted vCPU if an > + * IRQ becomes pending. E.g. if IRQs are disabled the VMM > + * can keep the vCPU in virtual HLT, even if an IRQ is > + * pending, without hanging/breaking the guest. > + * > + * do_sti parameter is used by __tdx_hypercall() to decide > + * whether to call STI instruction before executing TDCALL > + * instruction. > + */ > + ret = _tdx_hypercall(EXIT_REASON_HLT, irq_disabled, 0, 0, do_sti, NULL);
So that irq_disabled goes into r12. Nothing in that section 3.8 above talks about r12. The doc version I'm looking at is:
344426-001US SEPTEMBER 2020
Where is that "the IRQs in the guest were disabled/enabled" bit documented?
> + > + /* > + * Use WARN_ONCE() to report the failure. Since tdx_*halt() calls > + * are also used in pv_ops, #VE handler error handler cannot be
one "handler"'s enough.
> + * used to report the failure. > + */ > + WARN_ONCE(ret, "HLT instruction emulation failed\n"); > +} > + > +static __cpuidle void tdx_halt(void) > +{ > + const bool irq_disabled = irqs_disabled(); > + const bool do_sti = false;
What is the logic here?
This is not a safe halt so it doesn't matter to the TDX module whether irqs are disabled or not?
That comment above is again keeping it to itself:
"But this change is not required for all HLT cases."
So for which cases is it required?
Is that explained in the comment in _tdx_halt() where irqs_disabled tells the VMM what to do with the guest - to wake it up or to keep it in virtual halt?
> + > + _tdx_halt(irq_disabled, do_sti); > +} > + > +static __cpuidle void tdx_safe_halt(void) > +{ > + const bool irq_disabled = false; /* since sti will be called */
Comments usually go ontop not on the side.
> + const bool do_sti = true; > + > + _tdx_halt(irq_disabled, do_sti); > +} > + > unsigned long tdx_get_ve_info(struct ve_info *ve) > { > struct tdx_module_output out = {0};
Thx.
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |