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()?
>
> 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]) ?
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
[1] https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/#t
