Messages in this thread | | | Subject | Re: [PATCH v7 08/12] x86/tdx: Add HLT support for TDX guest | From | "Kuppuswamy, Sathyanarayanan" <> | Date | Thu, 23 Sep 2021 12:33:02 -0700 |
| |
On 9/23/21 11:09 AM, Borislav Petkov wrote: > 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.
Sorry, I fixed it in commit log, but missed it here. I will fix it in next submission.
> >> + * >> + * 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?
IRQ parameter specification update is not yet released for public. I think it will be released in 2-3 weeks.
> >> + >> + /* >> + * 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.
Ok. I will fix this in next version.
> >> + * 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?
It's only needed for the safe hlt case because the non safe hlt case doesn't change anything about the interrupt.
> > 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?
I think it is left in halt state. Sean, any comment?
> >> + >> + _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.
I will fix this in next version.
> >> + 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. >
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |