Re: [libvirt] [PATCH 21/24] hostdev: Stop early if unmanaged devices have not been detached

On Sat, 2016-03-12 at 09:23 -0500, John Ferlan wrote: > On 03/07/2016 12:24 PM, Andrea Bolognani wrote: >> >> Unmanaged devices, as the name suggests, are not detached >> automatically from the host by libvirt before being attached to a >> guest: it's the user's responsability to detach them manually >> beforehand. If that preliminary step has not been performed, the >> attach operation can't complete successfully. >> >> Instead of relying on the lower layers to error out with cryptic >> messages such as >> >> error: Failed to attach device from /tmp/hostdev.xml >> error: Path '/dev/vfio/12' is not accessible: No such file or directory >> >> prevent the situation altogether and provide the user with a more >> useful error message. >> --- >> src/util/virhostdev.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c >> index 03c3445..d1529c5 100644 >> --- a/src/util/virhostdev.c >> +++ b/src/util/virhostdev.c >> @@ -576,6 +576,13 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, >> mgr->inactivePCIHostdevs) < 0) > > Is this in the right place? > > Consider a few lines above: > > /* Step 1: validate that non-managed device isn't in use, ... Well, that comment is not really 100% accurate now that I look at it. What the code in step 1 is really doing is making sure that the device is assignable (virPCIDeviceIsAssignable(), haven't really investigated that function TBH so I'm just assuming it's doing something valuable ;) and that none of the devices we're about to assign to the guest have already been assigned to another guest - or, in the case of VFIO, that no devices that are in the same IOMMU group as a device we're about to assign to the guest have already been assigned to another guest. There are actually no checks on unmanaged devices, and rightly so: it's totally okay to add unmanaged devices to a guest! We just have to make sure the user has actually taken care to detach the device from the host first, hence the new check I'm adding with this commit. The comment for step 1 needs some work, though, as you point out... > Second question - how would the device get on the inactiveList > initially? Looking at virPCIDeviceListAdd calls for inactiveList. > > 'pcidevs' is the list of all devices both managed and unmanaged > > 'activeList' is populated in step 5 and virHostdevUpdateActivePCIDevices > > 'inactiveList' is populated in 'inactivedevs:' label, in step 2 of > virHostdevReAttachPCIDevices, and in virPCIDeviceDetach via > virPCIDeviceListAddCopy *if* the device is managed in step 2 of Prepare. 'pcidevs' is not the list of all devices, it's the list of (really just names of) devices that we're handling at the moment. It's our working set, basically. But yes, devices in 'pcidevs' can be both managed or unmanaged. As for your question, a device might end up in the inactive list because of: * PreparePCIDevices(), step 2, if managed. Just temporarily, it's moved to the active list in step 5 * ReattachPCIDevices(), step 4, whether managed or unmanaged. If it's managed is also removed from the inactive list in the same step, if it's unmanaged it will remain in the inactive list * PCINodeDeviceDetach() This is not including rollback paths. So it ends up in the inactive list when it's either *not yet* active, or *no longer* active. > I don't disagree this is an important step, but it's the "how" we > determine this that I'm questioning. Hopefully the above helped :)