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

[Nicolin Chen]

On Fri, Oct 04, 2024 at 02:32:28PM +1000, Alexey Kardashevskiy wrote:
> > +/**
> > + * struct iommu_viommu_set_vdev_id - ioctl(IOMMU_VIOMMU_SET_VDEV_ID)
> > + * @size: sizeof(struct iommu_viommu_set_vdev_id)
> > + * @viommu_id: viommu ID to associate with the device to store its virtual ID
> > + * @dev_id: device ID to set its virtual ID
> > + * @__reserved: Must be 0
> > + * @vdev_id: Virtual device ID
> > + *
> > + * Set a viommu-specific virtual ID of a device
> > + */
> > +struct iommu_viommu_set_vdev_id {
> > +     __u32 size;
> > +     __u32 viommu_id;
> > +     __u32 dev_id;
> 
> Is this ID from vfio_device_bind_iommufd.out_devid?

Yes.

> > +     __u32 __reserved;
> > +     __aligned_u64 vdev_id;
> 
> What is the nature of this id? It is not the guest's BDFn, is it? The

Not exactly but certainly can be related. Explaining below..

> code suggests it is ARM's "SID" == "stream ID" and "

Yes. That's the first use case of that.

> a device might be
> able to generate multiple StreamIDs" (how, why?) 🤯 And these streams
> seem to have nothing to do with PCIe IDE streams, right?

PCI device only has one stream ID per its SMMU.

So the Stream ID is more like a channel ID or client ID from the
SMMU (IOMMU) view. A PCI device's Stream ID can be calculated from
the BDF numbers + the Stream-ID base of that PCI bus.

That said, this is all about IOMMU. So, it is likely more natural
to forward an IOMMU-specific ID (vStream ID for a vSMMU) v.s. BDF.

> For my SEV-TIO exercise ("trusted IO"), I am looking for a kernel
> interface to pass the guest's BDFs for a specific host device (which is
> passed through) and nothing in the kernel has any knowledge of it atm,
> is this the right place, or another ioctl() is needed here?
> 
> Sorry, I am too ignorant about ARM :)

We are reworking this ioctl to an IOMMU_VDEVICE_ALLOC cmd, meaning
a virtual device allocation. A virtual device is another bond when
an iommufd_device connects to an iommufd_viommu in the VM. The name
"vDEVICE" and "virtual device" still need to go through discussion,
so they aren't finalized. But the idea here is to have a structure
to gather all virtualization information of the intersection of the
device and the vIOMMU in the VM.

On the other hand, BDF is very PCI specific yet IOMMU independent.
E.g. it could exist for a PCI device even without a vIOMMU in the
VM, i.e. there is no vDEVICE in such case. Right?

So, if your use case relies on IOMMU and it is even a part of the
IOMMU virtualization features, I think you are looking at the right
place. And we should discuss how to incorporate that. Otherwise, I
feel the struct vfio_pci might be the one to extend?

> > +};
> > +#define IOMMU_VIOMMU_SET_VDEV_ID _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_SET_VDEV_ID)
> > +
> > +/**
> > + * struct iommu_viommu_unset_vdev_id - ioctl(IOMMU_VIOMMU_UNSET_VDEV_ID)
> > + * @size: sizeof(struct iommu_viommu_unset_vdev_id)
> > + * @viommu_id: viommu ID associated with the device to delete its virtual ID
> > + * @dev_id: device ID to unset its virtual ID
> > + * @__reserved: Must be 0
> > + * @vdev_id: Virtual device ID (for verification)
> > + *
> > + * Unset a viommu-specific virtual ID of a device
> > + */
> > +struct iommu_viommu_unset_vdev_id {
> > +     __u32 size;
> > +     __u32 viommu_id;
> > +     __u32 dev_id;
> > +     __u32 __reserved;
> > +     __aligned_u64 vdev_id;
> > +};
> > +#define IOMMU_VIOMMU_UNSET_VDEV_ID _IO(IOMMUFD_TYPE, IOMMUFD_CMD_VIOMMU_UNSET_VDEV_ID)
> >   #endif
> 
> Nit: "git format-patch -O orderfile" makes patches nicer by putting the
> documentation first (.h before .c, in this case) with the "ordefile"
> looking like this:
> 
> ===
> *.txt
> configure
> *Makefile*
> *.json
> *.h
> *.c
> ===

Interesting :)

Will try it!

Thanks
Nicolin