RE: [PATCH v1 9/9] smaples: add vfio-mdev-pci driver

[Liu, Yi L]

Hi Alex,

> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: Thursday, July 4, 2019 1:22 AM
> To: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> Subject: Re: [PATCH v1 9/9] smaples: add vfio-mdev-pci driver
> 
> On Wed, 3 Jul 2019 08:25:25 +0000
> "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
> 
> > Hi Alex,
> >
> > Thanks for the comments. Have four inline responses below. And one
> > of them need your further help. :-)

[...]

> > > > >
> > > > > > > > > used iommu_attach_device() rather than iommu_attach_group()
> > > > > > > > > for non-aux mdev iommu_device.  Is there a requirement that
> > > > > > > > > the mdev parent device is in a singleton iommu group?
> > > > > > > >
> > > > > > > > I don't think there should have such limitation. Per my
> > > > > > > > understanding, vfio-mdev-pci should also be able to bind to
> > > > > > > > devices which shares iommu group with other devices. vfio-pci works
> well
> > > for such devices.
> > > > > > > > And since the two drivers share most of the codes, I think
> > > > > > > > vfio-mdev-pci should naturally support it as well.
> > > > > > >
> > > > > > > Yes, the difference though is that vfio.c knows when devices are
> > > > > > > in the same group, which mdev vfio.c only knows about the
> > > > > > > non-iommu backed group, not the group that is actually used for
> > > > > > > the iommu backing.  So we either need to enlighten vfio.c or
> > > > > > > further abstract those details in vfio_iommu_type1.c.
> > > > > >
> > > > > > Not sure if it is necessary to introduce more changes to vfio.c or
> > > > > > vfio_iommu_type1.c. If it's only for the scenario which two
> > > > > > devices share an iommu_group, I guess it could be supported by
> > > > > > using __iommu_attach_device() which has no device counting for the
> > > > > > group. But maybe I missed something here. It would be great if you
> > > > > > can elaborate a bit for it. :-)
> > > > >
> > > > > We need to use the group semantics, there's a reason
> > > > > __iommu_attach_device() is not exposed, it's an internal helper.  I
> > > > > think there's no way around that we need to somewhere track the
> > > > > actual group we're attaching to and have the smarts to re-use it for
> > > > > other devices in the same group.
> > > >
> > > > Hmmm, exposing __iommu_attach_device() is not good, let's forget it.
> > > > :-)
> > > >
> > > > > > > > > If this is a simplification, then vfio-mdev-pci should not
> > > > > > > > > bind to devices where this is violated since there's no way
> > > > > > > > > to use the device.  Can we support it though?
> > > > > > > >
> > > > > > > > yeah, I think we need to support it.
> >
> > I've already made vfio-mdev-pci driver work for non-singleton iommu
> > group. e.g. for devices in a single iommu group, I can bind the devices
> > to eithervfio-pci or vfio-mdev-pci and then passthru them to a VM. And
> > it will fail if user tries to passthru a vfio-mdev-pci device via vfio-pci
> > manner "-device vfio-pci,host=01:00.1". In other words, vfio-mdev-pci
> > device can only passthru via
> > "-device vfio-pci,sysfsdev=/sys/bus/mdev/devices/UUID". This is what
> > we expect.
> >
> > However, I encountered a problem when trying to prevent user from
> > passthru these devices to different VMs. I've tried in my side, and I
> > can passthru vfio-pci device and vfio-mdev-pci device to different
> > VMs. But actually this operation should be failed. If all the devices
> > are bound to vfio-pci, Qemu will open iommu backed group. So
> > Qemu can check if a given group has already been used by an
> > AddressSpace (a.ka. VM) in vfio_get_group() thus to prevent
> > user from passthru these devices to different VMs if the devices
> > are in the same iommu backed group. However, here for a
> > vfio-mdev-pci device, it has a new group and group ID, Qemu
> > will not be able to detect if the other devices (share iommu group
> > with vfio-mdev-pci device) are passthru to existing VMs. This is the
> > major problem for vfio-mdev-pci to support non-singleton group
> > in my side now. Even all devices are bound to vfio-mdev-pci driver,
> > Qemu is still unable to check since all the vfio-mdev-pci devices
> > have a separate mdev group.
> >
> > To fix it, may need Qemu to do more things. E.g. If it tries to use a
> > non-singleton iommu backed group, it needs to check if any mdev
> > group is created and used by an existing VM. Also it needs check if
> > iommu backed group is passthru to an existing VM when trying to
> > use a mdev group. For singleton iommu backed group and
> > aux-domain enabled physical device, still allow to passthru mdev
> > group to different VMs. To achieve these checks, Qemu may need
> > to have knowledge whether a group is iommu backed and singleton
> > or not. Do you think it is good to expose such info to userspace? or
> > any other idea? :-)
> 
> QEMU is never responsible for isolating a group, QEMU is just a
> userspace driver, it's vfio's responsibility to prevent the user from
> splitting groups in ways that are not allowed.  QEMU does not know the

yep, also my concern.

> true group association, it only knows the "virtual" group of the mdev
> device.  QEMU will create a container and add the mdev virtual group to
> the container.  In the kernel, the type1 backend should actually do an
> iommu_attach_group(), attaching the iommu_device group to the domain.
> When QEMU processes the next device, it will have a different group,
> but (assuming no vIOMMU) it will try to attach it to the same
> container, which should work because the iommu_device group backing the
> mdev virtual group is already attached to this domain.
> If we had two separate QEMU processes, each with an mdev device from a
> common group at the iommu_device level, the type1 backend should fail
> to attach the group to the container for the later caller.  I'd think
> this should fail at the iommu_attach_group() call since the group we're
> trying to attach is already attached to another domain.

Agree with you. At first, I want to fail it in similar way with vfio-pci devices.
For vfio-pci devices from a common group, vfio will fail the operation around
/dev/vfio/group_id open if user tries to assign the vfio-pci devices from common
group to multiple QEMU processes. Meanwhile, QEMU will avoid to open a
/dev/vfio/group_id multiple times, so current vfio/QEMU works well for 
non- singleton group (no vIOMMU). Unfortunately, looks like we have no way
to fail vfio-mdev-pci devices in similar mechanism as each mdev has a separate
group. So yes, I agree with you that we may fail it around the group attach
phase. Below is my draft idea:

In vfio_iommu_type1_attach_group(), we need to do the following checks.

if (mdev_group) {
	if (iommu_device group enabled aux-domain) {
		/*
		  * iommu_group enabled aux-domain means the iommu_devices
		  * in this group are aux-domain enabled. e.g. SIOV capable devices.
		  * Also, I think for aux-domain enabled group, it essentially means
		  * the group is a singleton group as SIOV capable devices require
		  * to be in a singleton group.
		  */
		 iommu_aux_attach_device();
	} else {
		/*
		  * needs to check the group->opened in vfio.c. Just like what
		  * vfio_group_fopen() does. May be a new VFIO interface required
		  * here since the group->opened is within vfio.c.
		  * vfio_iommu_device_group_opened_inc() will inc group->opened, so
		  * that other VM will fail when trying to open the group. And another
		  * VFIO interface is also required to dec group->opened when VM is
		  * down.
		  */
		if (vfio_iommu_device_group_opened_inc(iommu_device_group))
			return -EBUSY;
		iommu_attach_gorup(iommu_device_group);
	}
}

The concern here is the two new VFIO interfaces. Any thoughts on this proposal? :-)

> It's really unfortunate that we don't have the mdev inheriting the
> iommu group of the iommu_device so that userspace can really understand
> this relationship.  A separate group makes sense for the aux-domain
> case, and is (I guess) not a significant issue in the case of a
> singleton iommu_device group, but it's pretty awkward here.  Perhaps
> this is something we should correct in design of iommu backed mdevs.

Yeah, for aux-domain case, it is not significant issue as aux-domain essentially
means singleton iommu_devie group. And in early time, when designing the support
for wrap pci as a mdev, we also considered to let vfio-mdev-pci to reuse
iommu_device group. But this results in an iommu backed group includes mdev and
physical devices, which might also be strange. Do you think it is valuable to reconsider
it?

> Thanks,
> 
> Alex

Thanks,
Yi Liu