linux-kernel - Re: [RFC PATCH 2/2] KVM: TDX: Kick off vCPUs when SEAMCALL is busy during TD page removal

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] [day] [month] [year] [list]

Message-ID: <Z2JhXfA14UjC1/fs@yzhao56-desk.sh.intel.com>
Date: Wed, 18 Dec 2024 13:45:01 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
	<dave.hansen@...ux.intel.com>, <rick.p.edgecombe@...el.com>,
	<kai.huang@...el.com>, <adrian.hunter@...el.com>,
	<reinette.chatre@...el.com>, <xiaoyao.li@...el.com>,
	<tony.lindgren@...el.com>, <binbin.wu@...ux.intel.com>,
	<dmatlack@...gle.com>, <isaku.yamahata@...el.com>,
	<isaku.yamahata@...il.com>, <nik.borisov@...e.com>,
	<linux-kernel@...r.kernel.org>, <x86@...nel.org>
Subject: Re: [RFC PATCH 2/2] KVM: TDX: Kick off vCPUs when SEAMCALL is busy
 during TD page removal

On Tue, Dec 17, 2024 at 03:29:03PM -0800, Sean Christopherson wrote:
> On Thu, Nov 21, 2024, Yan Zhao wrote:
> > For tdh_mem_range_block(), tdh_mem_track(), tdh_mem_page_remove(),
> > 
> > - Upon detection of TDX_OPERAND_BUSY, retry each SEAMCALL only once.
> > - During the retry, kick off all vCPUs and prevent any vCPU from entering
> >   to avoid potential contentions.
> > 
> > Signed-off-by: Yan Zhao <yan.y.zhao@...el.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  2 ++
> >  arch/x86/kvm/vmx/tdx.c          | 49 +++++++++++++++++++++++++--------
> >  2 files changed, 40 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 521c7cf725bc..bb7592110337 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -123,6 +123,8 @@
> >  #define KVM_REQ_HV_TLB_FLUSH \
> >  	KVM_ARCH_REQ_FLAGS(32, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_UPDATE_PROTECTED_GUEST_STATE	KVM_ARCH_REQ(34)
> > +#define KVM_REQ_NO_VCPU_ENTER_INPROGRESS \
> > +	KVM_ARCH_REQ_FLAGS(33, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  
> >  #define CR0_RESERVED_BITS                                               \
> >  	(~(unsigned long)(X86_CR0_PE | X86_CR0_MP | X86_CR0_EM | X86_CR0_TS \
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 60d9e9d050ad..ed6b41bbcec6 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -311,6 +311,20 @@ static void tdx_clear_page(unsigned long page_pa)
> >  	__mb();
> >  }
> >  
> > +static void tdx_no_vcpus_enter_start(struct kvm *kvm)
> > +{
> > +	kvm_make_all_cpus_request(kvm, KVM_REQ_NO_VCPU_ENTER_INPROGRESS);
> 
> I vote for making this a common request with a more succient name, e.g. KVM_REQ_PAUSE.
KVM_REQ_PAUSE looks good to me. But will the "pause" cause any confusion with
the guest's pause state?

> And with appropriate helpers in common code.  I could have sworn I floated this
> idea in the past for something else, but apparently not.  The only thing I can
Yes, you suggested me to implement it via a request, similar to
KVM_REQ_MCLOCK_INPROGRESS. [1].
(I didn't add your suggested-by tag in this patch because it's just an RFC).

[1] https://lore.kernel.org/kvm/ZuR09EqzU1WbQYGd@google.com/

> find is an old arm64 version for pausing vCPUs to emulated.  Hmm, maybe I was
> thinking of KVM_REQ_OUTSIDE_GUEST_MODE?
KVM_REQ_OUTSIDE_GUEST_MODE just kicks vCPUs outside guest mode, it does not set
a bit in vcpu->requests to prevent later vCPUs entering.

> Anyways, I don't see any reason to make this an arch specific request.
After making it non-arch specific, probably we need an atomic counter for the
start/stop requests in the common helpers. So I just made it TDX-specific to
keep it simple in the RFC.

Will do in non-arch specific way if you think it's worth.