RE: [PATCH RFCv1 04/14] iommufd: Add struct iommufd_viommu and iommufd_viommu_ops

[Tian, Kevin]

> From: Nicolin Chen <nicolinc@xxxxxxxxxx>
> Sent: Thursday, May 23, 2024 12:02 PM
> 
> On Thu, May 23, 2024 at 01:43:45AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Sent: Wednesday, May 22, 2024 9:39 PM
> > > VIOMMU contains:
> > >  - A nesting parent
> > >  - A KVM
> > >  - Any global per-VM data the driver needs
> > >    * In ARM case this is VMID, sometimes shared with KVM
> >
> > In which case is it not shared with KVM? I had the impression that
> > VMID always comes from KVM in this VCMDQ usage. 😊
> 
> Not actually. I guess that shared VMID is for BTM.

yes. now that is clear to me after reading more context.

> 
> > > On ARM the S2 is not divorced from the VIOMMU, ARM requires a single
> > > VMID, shared with KVM, and localized to a single VM for some of the
> > > bypass features (vBTM, vCMDQ). So to attach a S2 you actually have 

but here the description should be only for vBTM too. vCMDQ has
its own VMID policy.

> > > attach the VIOMMU to pick up the correct VMID.
> > >
> > > I imagine something like this:
> > >    hwpt_alloc(deva, nesting_parent=true) = shared_s2
> > >    viommu_alloc(deva, shared_s2) = viommu1
> > >    viommu_alloc(devb, shared_s2) = viommu2
> > >    hwpt_alloc(deva, viommu1, vste) = deva_vste
> > >    hwpt_alloc(devb, viommu2, vste) = devb_vste
> > >    attach(deva, deva_vste)
> > >    attach(devb, devb_vste)
> > >    attach(devc, shared_s2)
> >
> > I wonder whether we want to make viommu as the 1st-class citizen
> > for any nested hwpt if it is desirable to enable it even for VT-d which
> > lacks of a hw viommu concept at the moment.
> 
> I think Jason is completely using SMMU as an example here.
> 
> Also FWIW, I am trying a core-allocated core-managed viommu for
> IOMMU_VIOMMU_TYPE_DEFAULT. So VT-d driver doesn't need to hold
> a viommu while VMM could still allocate one if it wants. And the
> VIOMMU interface can provide some helpers if driver wants some
> info from the core-managed viommu: a virtual dev ID to physical
> dev ID (returning device pointer) translation for example. And
> we can add more after we brain storm.

that'd be nice.
 
> > > The driver will then know it should program three different VMIDs for
> > > the same S2 page table, which matches the ARM expectation for
> > > VMID. That is to say we'd pass in the viommu as the pt_id for the
> > > iommu_hwpt_alloc. The viommu would imply both the S2 page table and
> > > any meta information like VMID the driver needs.
> >
> > Can you elaborate the aspect about "three different VMIDs"? They are
> > all for the same VM hence sharing the same VMID per the earlier
> > description. This is also echo-ed in patch14:
> >
> > tegra241_cmdqv_viommu_alloc()
> >         vintf->vmid = smmu_domain->vmid;
> 
> The design in the series is still old using a 1:1 relationship
> between a viommu and an S2 domain. I think the "three" is from
> his SMMU example above? Leaving it to Jason to reply though.

I'd expect some detailed explanation in the commit msg about how
VCMDQ actually works. It's unclear to me whether this "three
different VMIDs" thing comes from the hw requirement or certain
software design choices.