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>
