Re: [libvirt] [PATCH v4 09/14] hostdev: Maintain a driver list of active mediated devices
On Sun, Mar 26, 2017 at 03:00:58PM -0400, Laine Stump wrote:
(I'm unable to apply this patch to the head of master with
"git am -3",
and it won't show me the conflicts (it just fails saying "fatal: sha1
information is lacking or useless (src/libvirt_private.syms), error:
could not build fake ancestor". Because of this, all further review is
based purely on examining the patch emails.)
Hmm, I also got a merge conflict (not sure if it's the same one you got) at
virHostdevReAttachMediatedDevices, but not with a fatal error. Wondering what
the problem at your end might be.
BTW, I'm assuming that you've run "make syntax-check
&& make check" as
each patch is applied, so that we're sure the functions in
I have.
libvirt_private.syms are in proper alphabetic order (among other
things). I've run it at a few stages of applying the patches, but not all.
[..]
> void
> +qemuHostdevReAttachMediatedDevices(virQEMUDriverPtr driver,
> + const char *name,
> + virDomainHostdevDefPtr *hostdevs,
> + int nhostdevs)
> +{
> + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +
> + virHostdevReAttachMediatedDevices(hostdev_mgr, QEMU_DRIVER_NAME,
> + name, hostdevs, nhostdevs);
What does "ReAttach" mean for a mediated device? Isn't this a NOP?
Oh, nevermind - I looked ahead and compared to the PCI version of the
ReAttach function. Turns out the PCI version does a *bunch* of things
other than reattaching the device to its host driver, including
restoring netdev config (which doesn't apply for mdevs and calling
virPCIDeviceReset() (which is a NOP even for PCI devices as long as
we're using VFIO, which *everybody* is these days). *BUT* the one other
thing it does is move the devices that had been in use by the domain
from the active list to the inactive list, and that's what the mdev
version of the function does.
Yeah, this was merely just to stay consistent, I hated it from the very moment
I introduced the function, but I checked with other types of devices and
thought "it just might be easier to read", but I will add a comment that
it's
there just because of consistency reasons and re-attach doesn't make any sense
for mdevs.
Someday in the future we may want to clean these up and rename the
functions appropriately, but for now having them named similarly makes
it easier to review, so forget I said anything :-)
Yep.
Erik