Received-SPF: pass (google.com: domain of linux-kernel+bounces-143175-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 12 Apr 2024 11:09:57 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Isaku Yamahata <isaku.yamahata@intel.com>, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Edgecombe, Rick P" <rick.p.edgecombe@intel.com>,
	isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
	erdemaktas@google.com, Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 076/130] KVM: TDX: Finalize VM initialization
Message-ID: <20240412180957.GI3039520@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <e3c862ae9c78bda2988768c1038fec100bb372cf.1708933498.git.isaku.yamahata@intel.com>
 <f3381541-822b-4e94-93f7-699afc6aa6a3@intel.com>
 <20240412010848.GG3039520@ls.amr.corp.intel.com>
 <a876accc-a7bf-4317-9612-d6d5a1fbaf9c@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <a876accc-a7bf-4317-9612-d6d5a1fbaf9c@intel.com>

On Fri, Apr 12, 2024 at 03:22:00PM +0300,
Adrian Hunter <adrian.hunter@intel.com> wrote:

> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 0d3b79b5c42a..c7ff819ccaf1 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -2757,6 +2757,12 @@ static int tdx_td_finalizemr(struct kvm *kvm)
> >  		return -EINVAL;
> >  
> >  	err = tdh_mr_finalize(kvm_tdx);
> > +	kvm_tdx->hw_error = err;
> > +
> > +	if (err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX))
> 
> There seem to be also implicit operand codes.  How sure are
> we that TDX_OPERAND_ID_RCX is the only valid busy operand?

According to the description of TDH.MR.FINALIZE, it locks exclusively,
RCX in TDR, TDCS as implicit, OP_STATE as implicit.  And the basic TDX feature
to run guest TD, TDX module locks in order of TDR => OP_STATE. We won't see
OP_STATE lock failure after gaining TDR lock.

If you worry for future, we can code it as
(err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_BUSY.  We should do it
consistently, though.

> > +		return -EAGAIN;
> > +	if (err == TDX_NO_VCPUS)
> 
> TDX_NO_VCPUS is not one of the completion status codes for
> TDH.MR.FINALIZE

It depends on the document version.  Need to check TDX_OP_STATE_INCORRECT
to be defensive.


> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index 98f5d7c5891a..dc150b8bdd5f 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -18,6 +18,9 @@ struct kvm_tdx {
> >  	u64 xfam;
> >  	int hkid;
> >  
> > +	/* For KVM_TDX ioctl to return SEAMCALL status code. */
> > +	u64 hw_error;
> 
> For this case, it seems weird to have a struct member
> to pass back a return status code,  why not make it a parameter
> of tdx_td_finalizemr() or pass &tdx_cmd?

I created the patch too quick. Given KVM_TDX_CAPABILITIES and KVM_TDX_INIT_VM
take tdx_cmd already, it's consistent to make tdx_td_finalize() take it.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>