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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
Thanks.
Erik
