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

[John Ferlan]

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