[libvirt] [PATCH 3/4] qemu: hotplug: Introduce hot unplug for mediated devices

[Peter Krempa]

On Wed, Mar 28, 2018 at 11:47:47 +0200, Erik Skultety wrote:
> On Tue, Mar 27, 2018 at 11:41:45AM +0200, Peter Krempa wrote:
> > On Tue, Mar 27, 2018 at 10:57:15 +0200, Erik Skultety wrote:
> > > Mediated devices support hot-{plug,unplug} since their introduction in
> > > kernel 4.10, however this feature has been missing in libvirt since
> > > commit ec783d7c introduced a hostdev type for mdevs.
> > >
> > > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > > ---
> > >  src/qemu/qemu_hotplug.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > >
> > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > > index 4abc7393b..ff77b47bc 100644
> > > --- a/src/qemu/qemu_hotplug.c
> > > +++ b/src/qemu/qemu_hotplug.c
> > > @@ -4030,6 +4030,17 @@ qemuDomainRemoveSCSIVHostDevice(virQEMUDriverPtr driver,
> > >      qemuHostdevReAttachSCSIVHostDevices(driver, vm->def->name, &hostdev, 1);
> > >  }
> > >
> > > +
> > > +static void
> > > +qemuDomainRemoveMediatedDevice(virQEMUDriverPtr driver,
> > > +                               virDomainObjPtr vm,
> > > +                               virDomainHostdevDefPtr hostdev)
> > > +{
> > > +    qemuHostdevReAttachMediatedDevices(driver, vm->def->name, &hostdev, 1);
> > > +    qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
> >
> > Looks like you are missing teardown of the cgroups, and namespace
> > membership here.
> 
> I'm not, this is handled from qemuDomainRemoveHostDevice which is called from
> qemuDomainDetachThisHostDevice right after qemuDomainDetachMediatedDevice was
> called.

Okay, I did not notice that this was registered under the hostdev
removal function rather than the device removal function.

> > > +static int
> > > +qemuDomainDetachMediatedDevice(virQEMUDriverPtr driver,
> > > +                               virDomainObjPtr vm,
> > > +                               virDomainHostdevDefPtr detach)
> > > +{
> > > +    int ret = -1;
> > > +    qemuDomainObjPrivatePtr priv = vm->privateData;
> > > +
> > > +    if (!detach->info->alias) {
> > > +        virReportError(VIR_ERR_OPERATION_FAILED,
> > > +                       "%s", _("device cannot be detached without a device alias"));
> > > +        return -1;
> > > +    }
> > > +
> > > +    qemuDomainMarkDeviceForRemoval(vm, detach->info);
> > > +
> > > +    qemuDomainObjEnterMonitor(driver, vm);
> > > +    ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
> > > +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> > > +        ret = -1;
> >
> > You need to wait for removal here and delete it inplace if the call
> > returns soon enough.. Also call to qemuDomainResetDeviceRemoval is
> > missing.
> 
> All of this is already done as part of qemuDomainDetachThisHostDevice, point 1
> being done right at the end of the function mentioned above
> (qemuDomainWaitForDeviceRemoval), point 2 being satisfied by the last call in
> the same function.

Fair enough, both points come from the fact that I've assumed that these
are generic device deletion functions as we have for other devices and
not hostdev-specific ones, called from the hostdev removal function.

The confusion originates from the fact that the hostdev-specific workers
use the same prefix as other device deletion fucntions rather than
having something like "hostdev" in the name.

ACK to this patch if you fix the very long line as suggested.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180328/3b149d6f/attachment-0001.sig>