Re: [PATCH v2 06/19] iommufd/viommu: Add IOMMU_VIOMMU_SET/UNSET_VDEV_ID ioctl

[Nicolin Chen]

On Fri, Oct 04, 2024 at 05:17:46PM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2024 at 12:25:19PM -0700, Nicolin Chen wrote:
> 
> > With that, I wonder what is better for the initial version of
> > this structure, a generic virtual ID or a driver-named ID like
> > "Stream ID"? The latter might be more understandable/flexible, 
> > so we won't need to justify a generic virtual ID along the way
> > if something changes in the nature?
> 
> I think the name could be a bit more specific "viommu_device_id"
> maybe? And elaborate in the kdoc that this is about the identifier
> that the iommu HW itself uses.

A "vIOMMU Device ID" might sound a bit confusing with an ID of a
vIOMMU itself :-/

At this moment, I named it "virt_id" in uAPI with a description:
" * @virt_id: Virtual device ID per vIOMMU"

I could add to that (or just in the documentation):
"e.g. ARM's Stream ID, AMD's DeviceID, Intel's Context Table ID"
to clarify it further.

> > That sounds wider than what I defined it for in my patch:
> >  * struct iommu_vdevice_alloc - ioctl(IOMMU_VDEVICE_ALLOC)
> >  * ...
> >  * Allocate a virtual device instance (for a physical device) against a vIOMMU.
> >  * This instance holds the device's information in a VM, related to its vIOMMU.
> > 
> > Would you please help rephrase it? It'd be also helpful for me
> > to update the doc.
> 
> I think that is still OK for the moment.
> 
> > Though I feel slightly odd if we define it wider than "vIOMMU"
> > since this is an iommufd header...
> 
> The notion I have is that vIOMMU would expand to encompass not just
> the physical hypervisor controled vIOMMU but also the vIOMMU
> controlled by the trusted "lowervisor" in a pkvm/cc/whatever world.
> 
> Alexey is working on vIOMMU support in CC which has the trusted world
> do some of the trusted vIOMMU components. I'm hoping the other people
> in this area will look at his design and make it fit nicely to
> everyone.

Oh, I didn't connect the dots that lowervisor must rely on the
vIOMMU too -- I'd need to check the CC stuff in detail. In that
case, having it in vIOMMU uAPI totally makes sense.

Thanks
Nicolin