linux-kernel - Re: [PATCH v7 08/12] x86/tdx: Add HLT support for TDX guest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YUzC7N8/MHI++y/G@zn.tnic>
Date:   Thu, 23 Sep 2021 20:09:48 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Juergen Gross <jgross@...e.com>, Deep Shah <sdeep@...are.com>,
        VMware Inc <pv-drivers@...are.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, Peter H Anvin <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        linux-kernel@...r.kernel.org
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