RE: [RFC v3 03/25] hw/iommu: introduce IOMMUContext

[Liu, Yi L]

Hi Peter,

> From: Peter Xu <address@hidden>
> Sent: Wednesday, February 12, 2020 12:59 AM
> To: Liu, Yi L <address@hidden>
> Subject: Re: [RFC v3 03/25] hw/iommu: introduce IOMMUContext
> 
> On Fri, Jan 31, 2020 at 11:42:13AM +0000, Liu, Yi L wrote:
> > > I'm not very clear on the relationship betwen an IOMMUContext and a
> > > DualStageIOMMUObject.  Can there be many IOMMUContexts to a
> > > DualStageIOMMUOBject?  The other way around?  Or is it just
> > > zero-or-one DualStageIOMMUObjects to an IOMMUContext?
> >
> > It is possible. As the below patch shows, DualStageIOMMUObject is per vfio
> > container. IOMMUContext can be either per-device or shared across devices,
> > it depends on vendor specific vIOMMU emulators.
> 
> Is there an example when an IOMMUContext can be not per-device?

No, I don’t have such example so far. But as IOMMUContext is got from
pci_device_iommu_context(),  in concept it possible to be not per-device.
It is kind of leave to vIOMMU to decide if different devices could share a
single IOMMUContext.

> It makes sense to me to have an object that is per-container (in your
> case, the DualStageIOMMUObject, IIUC), then we can connect that object
> to a device.  However I'm a bit confused on why we've got two abstract
> layers (the other one is IOMMUContext)?  That was previously for the
> whole SVA new APIs, now it's all moved over to the other new object,
> then IOMMUContext only register/unregister...

Your understanding is correct. Actually, I also struggled on adding two
abstract layer. But, you know, there are two function calling requirements
around vSVA enabling. First one is explicit method for vIOMMU calls into
VFIO (pasid allocation, bind guest page table, cache invalidate). Second
one is explicit method for VFIO calls into vIOMMU (DMA fault/PRQ injection
which is not included in this series yet, but will be upstreamed later). 
So I added the DualStageIOMMUObject to cover vIOMMU to VFIO callings, and
IOMMUContext to cover VFIO to vIOMMU callings. As IOMMUContext covers VFIO
to vIOMMU callings, so I made it include register/unregister.

> Can we put the reg/unreg
> procedures into DualStageIOMMUObject as well?  Then we drop the
> IOMMUContext (or say, keep IOMMUContext and drop DualStageIOMMUObject
> but let IOMMUContext to be per-vfio-container, the major difference is
> the naming here, say, PASID allocation does not seem to be related to
> dual-stage at all).
>
> Besides that, not sure I read it right... but even with your current
> series, the container->iommu_ctx will always only be bound to the
> first device created within that container, since you've got:
> 
>     group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev),
>                            pci_device_iommu_context(pdev), errp);
> 
> And:
> 
>     if (vfio_connect_container(group, as, iommu_ctx, errp)) {
>         error_prepend(errp, "failed to setup container for group %d: ",
>                       groupid);
>         goto close_fd_exit;
>     }
> 
> The iommu_ctx will be set to container->iommu_ctx if there's no
> existing container.

yes, it's true. May need to add a iommu_ctx list in VFIO container or
add check on the input iommu_ctx of vfio_get_group() if sticking on this
direction.

While considering your suggestion on dropping one of the two abstract
layers. I came up a new proposal as below:

We may drop the IOMMUContext in this series, and rename DualStageIOMMUObject
to HostIOMMUContext, which is per-vfio-container. Add an interface in PCI
layer(e.g. an callback in  PCIDevice) to let vIOMMU get HostIOMMUContext.
I think this could cover the requirement of providing explicit method for
vIOMMU to call into VFIO and then program host IOMMU.

While for the requirement of VFIO to vIOMMU callings (e.g. PRQ), I think it
could be done via PCI layer by adding an operation in PCIIOMMUOps. Thoughts?

Thanks,
Yi Liu