[libvirt PATCH 2/2] hostdev: mdev: Lookup mdevs by sysfs path rather than mdev struct

Erik Skultety eskultet at redhat.com
Fri Jan 8 07:02:42 UTC 2021


On Thu, Jan 07, 2021 at 09:32:43PM +0100, Ján Tomko wrote:
> On a Thursday in 2021, Erik Skultety wrote:
> > The lookup didn't do anything apart from comparing the sysfs paths
> > anyway since that's what makes each mdev unique.
> > The most ridiculous usage of the old logic was in
> > virHostdevReAttachMediatedDevices where in order to drop an mdev
> > hostdev from the list of active devices we first had to create a new
> > mdev and use it in the lookup call. Why couldn't we have used the
> > hostdev directly? Because the hostdev and mdev structures are
> > incompatible.
> > 
> > The way mdevs are currently removed is via a write to a specific sysfs
> > attribute. If you do it while the machine which has the mdev assigned
> > is running, the write call may block (this should return a write error
> > instead and it is likely a bug in the vendor driver) until the device

I'll reword ^this bit slightly as it turned out that this behavior conforms
to the kernel device model (which it previously didn't) and a device "remove"
apparently cannot return an error (I have this via a conversation on a private
list, so I can't link it as a source of truth).

> > is no longer bound to vfio which is when the QEMU process exits.
> > 
> > The interesting part here comes afterwards when we're cleaning up and
> > call virHostdevReAttachMediatedDevices. The domain doesn't exist
> > anymore, so the list of active hostdevs needs to be updated and the
> > respective hostdevs removed from the list, but remember we had to
> > create an mdev object in the memory in order to find it in the list
> > first which will fail because the write to sysfs had already removed
> > the mdev instance from the host system.
> > And so the next time you try to start the same domain you'll get:
> > 
> > "Requested operation is not valid: mediated device <path> is in use by
> > driver QEMU, domain <name>"
> > 
> 
> Is there a public bug/issue you could link here?

I just created one (gitlab issue) for bookkeeping and I'll link it here.

> 
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> > src/hypervisor/virhostdev.c | 10 ++++------
> > src/util/virmdev.c          | 16 ++++++++--------
> > src/util/virmdev.h          |  4 ++--
> > 3 files changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c
> > index aa3fc8738f..ea04382d0d 100644
> > --- a/src/hypervisor/virhostdev.c
> > +++ b/src/hypervisor/virhostdev.c
> > @@ -1975,7 +1975,7 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr,
> > 
> >     virObjectLock(mgr->activeMediatedHostdevs);
> >     for (i = 0; i < nhostdevs; i++) {
> > -        g_autoptr(virMediatedDevice) mdev = NULL;
> > +        g_autofree char * sysfspath = NULL;
> 
> extra space
> 
> >         virMediatedDevicePtr tmp;
> >         virDomainHostdevSubsysMediatedDevPtr mdevsrc;
> >         virDomainHostdevDefPtr hostdev = hostdevs[i];
> 
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
> 

Thanks.

Erik




More information about the libvir-list mailing list