On 03/15/2016 07:54 AM, Andrea Bolognani wrote: > 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 :) > Partially, let's consider an initial startup where Prepare is called (and I assume we don't call either virHostdevReattachPCIDevice or virHostdevPCINodeDeviceDetach yet). So how does this code check if the device has been manually detached from the host? Step 1 doesn't add to the inactiveList and step 2 only adds managed devices. How would that lookaside element be on the inactiveList - what put it there? Is there some magic w/ virHostdevPCINodeDeviceDetach that I'm missing? John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list