On Fri, 2025-08-29 at 13:11 -0700, Sean Christopherson wrote:
> > I guess the two approaches could be to make KVM_TDX_INIT_MEM_REGION more
> > robust,
>
> This. First and foremost, KVM's ordering and locking rules need to be
> explicit (ideally documented, but at the very least apparent in the code),
> *especially* when the locking (or lack thereof) impacts userspace. Even if
> effectively relying on the TDX-module to provide ordering "works", it's all
> but impossible to follow.
>
> And it doesn't truly work, as everything in the TDX-Module is a trylock, and
> that in turn prevents KVM from asserting success. Sometimes KVM has better
> option than to rely on hardware to detect failure, but it really should be a
> last resort, because not being able to expect success makes debugging no fun.
> Even worse, it bleeds hard-to-document, specific ordering requirements into
> userspace, e.g. in this case, it sounds like userspace can't do _anything_ on
> vCPUs while doing KVM_TDX_INIT_MEM_REGION. Which might not be a burden for
> userspace, but oof is it nasty from an ABI perspective.
I could see that. I didn't think of the below.
>
> > or prevent the contention. For the latter case:
> > tdh_vp_create()/tdh_vp_addcx()/tdh_vp_init*()/tdh_vp_rd()/tdh_vp_wr()
> > ...I think we could just take slots_lock during KVM_TDX_INIT_VCPU and
> > KVM_TDX_GET_CPUID.
> >
> > For tdh_vp_flush() the vcpu_load() in kvm_arch_vcpu_ioctl() could be hard to
> > handle.
> >
> > So I'd think maybe to look towards making KVM_TDX_INIT_MEM_REGION more
> > robust, which would mean the eventual solution wouldn't have ABI concerns by
> > later blocking things that used to be allowed.
> >
> > Maybe having kvm_tdp_mmu_map_private_pfn() return success for spurious
> > faults is enough. But this is all for a case that userspace isn't expected
> > to actually hit, so seems like something that could be kicked down the road
> > easily.
>
> You're trying to be too "nice", just smack 'em with a big hammer. For all
> intents and purposes, the paths in question are fully serialized, there's no
> reason to try and allow anything remotely interesting to happen.
>
> Acquire kvm->lock to prevent VM-wide things from happening, slots_lock to
> prevent kvm_mmu_zap_all_fast(), and _all_ vCPU mutexes to prevent vCPUs from
> interefering.
>
> Doing that for a vCPU ioctl is a bit awkward, but not awful. E.g. we can
> abuse kvm_arch_vcpu_async_ioctl(). In hindsight, a more clever approach would
> have been to make KVM_TDX_INIT_MEM_REGION a VM-scoped ioctl that takes a vCPU
> fd. Oh well.
Yea.
>
> Anyways, I think we need to avoid the "synchronous" ioctl path anyways,
> because taking kvm->slots_lock inside vcpu->mutex is gross. AFAICT it's not
> actively problematic today, but it feels like a deadlock waiting to happen.
>
> The other oddity I see is the handling of kvm_tdx->state. I don't see how
> this check in tdx_vcpu_create() is safe:
>
> if (kvm_tdx->state != TD_STATE_INITIALIZED)
> return -EIO;
>
> kvm_arch_vcpu_create() runs without any locks held, and so TDX effectively has
> the same bug that SEV intra-host migration had, where an in-flight vCPU
> creation could race with a VM-wide state transition (see commit ecf371f8b02d
> ("KVM: SVM: Reject SEV{-ES} intra host migration if vCPU creation is in-
> flight"). To fix that, kvm->lock needs to be taken and KVM needs to verify
> there's no in-flight vCPU creation, e.g. so that a vCPU doesn't pop up and
> contend a TDX-Module lock.
>
> We an even define a fancy new CLASS to handle the lock+check => unlock logic
> with guard()-like syntax:
>
> CLASS(tdx_vm_state_guard, guard)(kvm);
> if (IS_ERR(guard))
> return PTR_ERR(guard);
>
> IIUC, with all of those locks, KVM can KVM_BUG_ON() both TDH_MEM_PAGE_ADD and
> TDH_MR_EXTEND, with no exceptions given for -EBUSY. Attached patches are very
> lightly tested as usual and need to be chunked up, but seem do to what I want.
Ok, the direction seem clear. The patch has an issue, need to debug.
