lkml.org 
[lkml]   [2021]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 08/12] x86/tdx: Add HLT support for TDX guest
From
Date


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

\
 
 \ /
  Last update: 2021-09-23 21:34    [W:1.666 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site