[libvirt] [PATCH v2 1/2] util: virhostdev: disallow assigning a pci-bridge to a guest

Laine Stump laine at laine.org
Mon Jan 23 19:46:53 UTC 2017


On 01/23/2017 11:34 AM, Martin Kletzander wrote:
> On Mon, Jan 23, 2017 at 07:06:29PM +0530, Shivaprasad G Bhat wrote:
>> Non-endpoint devices like pci-bridges cannot be passedthrough to guests.
>
> "disallow" is a mouthful, I would also use "assigned" instead of
> "passedthrough".

Correct - "passthrough" is considered a "bad word" by the VFIO 
maintainer, and shouldn't be used. VFIO's purpose is "device assignment".

>
>> Prevent such attempts.
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat at linux.vnet.ibm.com>
>> ---
>> src/util/virhostdev.c |   10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index 0673afb..b23fe1f 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
>> @@ -532,6 +532,16 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr 
>> mgr,
>>         bool strict_acs_check = !!(flags & 
>> VIR_HOSTDEV_STRICT_ACS_CHECK);
>>         bool usesVFIO = (virPCIDeviceGetStubDriver(pci) == 
>> VIR_PCI_STUB_DRIVER_VFIO);
>>         struct virHostdevIsPCINodeDeviceUsedData data = { mgr, 
>> dom_name, usesVFIO };
>> +        int hdrType = -1;
>> +
>> +        if (virPCIGetHeaderType(pci, &hdrType) < 0)
>> +            goto cleanup;
>> +
>> +        if (hdrType != VIR_PCI_HEADER_ENDPOINT) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI 
>> bridge devices "
>> +                           "cannot be assigned to guests"));
>
> Actually, you should continue the string in the same column it was
> started, just start the long strings on their own lines, it's more
> readable.

Yeah, I didn't think to say that in my review.

>
> FTFY, ACK and pushed.
>
> I wanted you to make it part of virPCIDeviceIsAssignable(), but we use
> it kind of weirdly, so I'll keep it as-is, Laine (Cc'd) will know more


Not really :-P. I'm sure I'd looked at that function at least once 
before, but didn't remember it until you pointed it out. It's been there 
for a very long time with no change (introduced in Dec. 2009 by 
jdenemar) and is a leftover from legacy KVM assignment.


> about whether we want to move it there or not.

In it's current form that wouldn't work, since 
virPCIDeviceIsAssignable() is only called when we're not using VFIO, but 
we actually want to *always* check the PCI header type, both for VFIO, 
*AND* legacy KVM. But if somebody wants to change the one call so that 
it's called unconditionally, and change the 2nd arg to say 
"strict_acs_check && !usesVFIO), then the check of device type could be 
moved to that function, which sounds like a reasonable thing, based on 
the name.




More information about the libvir-list mailing list