Re: [PATCH v19 076/130] KVM: TDX: Finalize VM initialization

[Isaku Yamahata]

On Fri, Apr 12, 2024 at 03:22:00PM +0300,
Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxx>