linux-kernel - Re: [PATCH V4 0/1] KVM: TDX: Decrease TDX VM shutdown time

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: <aHpcBYwqhcw14iR1@google.com>
Date: Fri, 18 Jul 2025 07:36:53 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Nikolay Borisov <nik.borisov@...e.com>
Cc: Xiaoyao Li <xiaoyao.li@...el.com>, pbonzini@...hat.com, 
	Adrian Hunter <adrian.hunter@...el.com>, kvm@...r.kernel.org, rick.p.edgecombe@...el.com, 
	kirill.shutemov@...ux.intel.com, kai.huang@...el.com, 
	reinette.chatre@...el.com, tony.lindgren@...ux.intel.com, 
	binbin.wu@...ux.intel.com, isaku.yamahata@...el.com, 
	linux-kernel@...r.kernel.org, yan.y.zhao@...el.com, chao.gao@...el.com
Subject: Re: [PATCH V4 0/1] KVM: TDX: Decrease TDX VM shutdown time

On Thu, Jul 17, 2025, Nikolay Borisov wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index f4d4fd5cc6e8..783b1046f6c1 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -189,6 +189,8 @@ static int init_kvm_tdx_caps(const struct tdx_sys_info_td_conf *td_conf,
> >          if (!caps->supported_xfam)
> >                  return -EIO;
> > +       caps->supported_caps = KVM_TDX_CAP_TERMINATE_VM;
> 
> nit: For the sake of consistency make that a |= so that all subsequent
> additions to it will be uniform with the first.

Objection, speculation, your honor.  :-D

That assumes that the predominate pattern will be "|=".  But if we end up with a
collection of capabilities that are unconditionally enumerated by KVM, then I
definitely want to express that as:

	caps->supported_caps = KVM_TDX_CAP_TERMINATE_VM |
			       KVM_TDX_CAP_FANCY_THING_1 |
			       KVM_TDX_CAP_FANCY_THING_2 |
			       KVM_TDX_CAP_FANCY_THING_3;

not as

	caps->supported_caps |= KVM_TDX_CAP_TERMINATE_VM;
	caps->supported_caps |= KVM_TDX_CAP_FANCY_THING1;
	caps->supported_caps |= KVM_TDX_CAP_FANCY_THING2;
	caps->supported_caps |= KVM_TDX_CAP_FANCY_THING3;

I find the former to be much easier to read, and it provides some amount of
defense-in-depth against uninitialized data.  The downside is that if there are
conditional capabilities, then we need to ensure that they are added after the
unconditional set is initialized.  But we absolutely should be able to detect
such bugs via selftests.  And again, I find that this:

	caps->supported_caps = KVM_TDX_CAP_TERMINATE_VM |
			       KVM_TDX_CAP_FANCY_THING_1 |
			       KVM_TDX_CAP_FANCY_THING_2 |
			       KVM_TDX_CAP_FANCY_THING_3;

	if (i_can_has_cheezburger())
		caps->supported_caps |= KVM_TDX_CAP_CHEEZBURGER;

makes it easy to identify which capabilities are unconditional versus those that
are dependent on something else.