On Friday, September 6, 2024 12:33 PM, Tony Lindgren wrote:
> On Fri, Sep 06, 2024 at 12:05:41PM +0800, Yan Zhao wrote:
> > On Thu, Sep 05, 2024 at 12:27:54PM +0300, Tony Lindgren wrote:
> > > On Thu, Sep 05, 2024 at 02:59:25PM +0800, Yan Zhao wrote:
> > > > On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> > > > > On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > > > > > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > > > > > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > > > > > >
> > > > > > ...
> > > > > > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd
> > > > > > > +*cmd) {
> > > > > ...
> > > > >
> > > > > > > + kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx,
> TD_TDCS_EXEC_TSC_OFFSET);
> > > > > > > + kvm_tdx->attributes = td_params->attributes;
> > > > > > > + kvm_tdx->xfam = td_params->xfam;
> > > > > > > +
> > > > > > > + if (td_params->exec_controls &
> TDX_EXEC_CONTROL_MAX_GPAW)
> > > > > > > + kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > > > > > + else
> > > > > > > + kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > > > > > +
> > > > > > Could we introduce a initialized field in struct kvm_tdx and
> > > > > > set it true here? e.g
> > > > > > + kvm_tdx->initialized = true;
> > > > > >
> > > > > > Then reject vCPU creation in tdx_vcpu_create() before
> > > > > > KVM_TDX_INIT_VM is executed successfully? e.g.
> > > > > >
> > > > > > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > > > > > struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > > > > > struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > > >
> > > > > > + if (!kvm_tdx->initialized)
> > > > > > + return -EIO;
> > > > > > +
> > > > > > /* TDX only supports x2APIC, which requires an in-kernel local
> APIC. */
> > > > > > if (!vcpu->arch.apic)
> > > > > > return -EINVAL;
> > > > > >
> > > > > > Allowing vCPU creation only after TD is initialized can
> > > > > > prevent unexpected userspace access to uninitialized TD primitives.
> > > > >
> > > > > Makes sense to check for initialized TD before allowing other
> > > > > calls. Maybe the check is needed in other places too in additoin to the
> tdx_vcpu_create().
> > > > Do you mean in places checking is_hkid_assigned()?
> > >
> > > Sounds like the state needs to be checked in multiple places to
> > > handle out-of-order ioctls to that's not enough.
> > >
> > > > > How about just a function to check for one or more of the
> > > > > already existing initialized struct kvm_tdx values?
> > > > Instead of checking multiple individual fields in kvm_tdx or
> > > > vcpu_tdx, could we introduce a single state field in the two
> > > > strutures and utilize a state machine for check (as Chao Gao pointed out
> at [1]) ?
> > >
> > > OK
> > >
> > > > e.g.
> > > > Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
> > > > (4)destroyed, (5)freed.
> > > > Each vCPU has 3 states: (1) created, (2) initialized, (3)freed
> > > >
> > > > All the states are updated by a user operation (e.g.
> > > > KVM_TDX_INIT_VM, KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or
> a x86
> > > > op (e.g. vm_init, vm_destroy, vm_free, vcpu_create, vcpu_free).
> > > >
> > > >
> > > > TD vCPU
> > > > (1) created(set in op vm_init)
> > > > (2) initialized
> > > > (indicate tdr_pa != 0 && HKID assigned)
> > > >
> > > > (1) created (set in op
> > > > vcpu_create)
> > > >
> > > > (2) initialized
> > > >
> > > > (can call INIT_MEM_REGION,
> > > > GET_CPUID here)
> > > >
> > > >
> > > > (3) finalized
> > > >
> > > > (tdx_vcpu_run(),
> > > > tdx_handle_exit() can be here)
> > > >
> > > >
> > > > (4) destroyed (indicate HKID released)
> > > >
> > > > (3) freed
> > > >
> > > > (5) freed
> > >
> > > So an enum for the TD state, and also for the vCPU state?
> >
> > A state for TD, and a state for each vCPU.
> > Each vCPU needs to check TD state and vCPU state of itself for vCPU
> > state transition.
> >
> > Does it make sense?
>
> That sounds good to me :)
+1 sounds good.
I also thought about this. KVM could create a shadow of the TD and vCPU
states that are already defined and maintained by the TDX module.
This should also be more extensible for adding the TD migration support later
(compared to adding various Booleans).
