RE: [RFC PATCH v2 4/7] iommufd: Associate kvm pointer to iommufd ctx

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Monday, June 24, 2024 6:55 PM
> To: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> kvmarm@lists.linux.dev; iommu@lists.linux.dev; linux-arm-
> kernel@lists.infradead.org; Linuxarm <linuxarm@huawei.com>;
> kevin.tian@intel.com; alex.williamson@redhat.com; maz@kernel.org;
> oliver.upton@linux.dev; will@kernel.org; robin.murphy@arm.com; jean-
> philippe@linaro.org; Jonathan Cameron <jonathan.cameron@huawei.com>
> Subject: Re: [RFC PATCH v2 4/7] iommufd: Associate kvm pointer to iommufd
> ctx
> 
> On Mon, Jun 24, 2024, Jason Gunthorpe wrote:
> > On Mon, Jun 24, 2024 at 09:53:00AM -0700, Sean Christopherson wrote:
> > > If kvm_pinned_vmid_{get,put}() are implemented directly by KVM ARM,
> then I don't
> > > have any immediate concerns, as KVM ARM is a long, long way from
> being able to
> > > isolate KVM from the core kernel.
> >
> > I think that is a reasonable thing, I also don't really see VMID as
> > being general. We will have to figure out how to ensure that the KVM
> > FD we got is an ARM KVM FD..
> 
> Isn't the caller in ARM specific code?  I was assuming
> kvm_pinned_vmid_{get,put}()
> would simply not exist for non-ARM builds.

The caller is in ARM specific code(SMMUv3 driver). But at present, the kvm pointer
is passed to it by IOMMUFD during nested domain allocation time. And IOMMUFD
gets the kvm pointer from VFIO during device bind operation.

Not sure,, how this flow is going to be in  the new IOMMUFD VIOMMU object model
Jason referred. If not through IOMMUFD, SMMUv3 has to have a way to figure out
the KVM associated with the device.  

> 
> > > That said, I find the on-demand pinning to be very odd.  IIUC, if KVM runs
> out
> > > of pinnable VMIDs, attaching a device to the KVM+iommu will fail.  Failing
> an
> > > iommufd operation because of a (potentially transient) KVM resource
> issue is
> > > rather unpleasant.
> >
> > It is kind of subtle, but the only thing that will consume VMIDs is
> > IOMMUFD operations that are working with nested translation but not
> > providing KVMs. This is a pretty small blast radius - ie a specific
> > qemu will fail to start - that I think we can tolerate it.
> >
> > More normal iommu operation will not require VMIDs so things like
> > driver attaching/etc is fine.
> >
> > > And assuming that pinnable VMIDs are a somewhat scarce resource, it
> wouldn't
> > > suprise me if someone wanted to add cgroup integration, e.g. similar to
> the
> > > misc cgroup that's used to manage SEV(-ES) ASIDs on KVM AMD (IIUC, an
> SEV ASID
> > > is analagous to an ARM VMID).
> >
> > Yeah, but if someone is using such a cgroup then I expect they will
> > also have an up to date VMM that doesn't trigger this VMID allocation
> > in the first place...
> 
> I suspect we're talking about two different things.  Either that, or I am really
> lost.
> 
> > > Rather than on-demand pinning, would it make sense to have KVM
> provide an ioctl()
> > > (or capability, or VM type) to let userspace pin a VM's VMID?  That would
> allow
> > > for a much saner failure mode, and I suspect would be cleaner in general
> for iommufd.
> >
> > The point of this mechanism is to support using this iommufd feature
> > without a KVM at all. We could instead prevent this directly 100% of
> > the time, but it means that HW with this BTM capability would not run
> > the legacy VMMs at all, so I'm not that keen on it..
> >
> > When a KVM is present then the iommu needs to adopt the VMID of KVM,
> > and that should have a mechanism to ensure the VMID is valid so long
> > as the IOMMU is using it (eg because the KVM FD is open)
> 
> Right, and that's what I'm referring to as "on-demand pinning".  For the
> IOMMU
> to adopt a KVM VMID, the VMID needs to be pinned (or KVM would need to
> notify
> the IOMMU every time the VMID changed), i.e. every KVM+IOMMU pair pins
> a VMID
> that is managed by KVM.
> 
> Hmm, kvm_arm_pinned_vmid_get() doesn't fail, it just falls back to VMID=0.
> Which
> seems odd.

See patch 3, kvm_arch_pinned_vmid_get() return -EINVAL if VMID == 0. For ARM64/KVM
VMID 0 is always reserved and never allocated for a Guest.

Thanks,
Shameer