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 :)
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team