qemu: do not add model when actual iface type is hostdev

[PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Paulo de Rezende Pinatti 3 years, 10 months ago

No default model should be added to the interface
entry at post parse when its actual network type is hostdev
as doing so might cause a mismatch between the interface
definition and its actual device type.
---
 src/qemu/qemu_domain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2dad823a86..33ce0ad992 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5831,6 +5831,7 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
                                 virQEMUCapsPtr qemuCaps)
 {
     if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
+        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
         !virDomainNetGetModelString(net))
         net->model = qemuDomainDefaultNetModel(def, qemuCaps);
 
-- 
2.26.2

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Paulo de Rezende Pinatti 3 years, 10 months ago

Sorry, forgot to include my signed-off-by, so:

On 16/06/20 16:32, Paulo de Rezende Pinatti wrote:
> No default model should be added to the interface
> entry at post parse when its actual network type is hostdev
> as doing so might cause a mismatch between the interface
> definition and its actual device type.
> ---
>   src/qemu/qemu_domain.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2dad823a86..33ce0ad992 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5831,6 +5831,7 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
>                                   virQEMUCapsPtr qemuCaps)
>   {
>       if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
>           !virDomainNetGetModelString(net))
>           net->model = qemuDomainDefaultNetModel(def, qemuCaps);
>   
> 

Signed-off-by: Paulo de Rezende Pinatti <ppinatti@linux.ibm.com>

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Laine Stump 3 years, 10 months ago

On 6/16/20 10:32 AM, Paulo de Rezende Pinatti wrote:
> No default model should be added to the interface
> entry at post parse when its actual network type is hostdev
> as doing so might cause a mismatch between the interface
> definition and its actual device type.

Have you encountered a real problem from this? (I have, and have been 
thinking about the issue for awhile, but only late at night when I'm not 
near my keyboard to do something about it. I'm just wondering what 
problem you've had :-))

The reason it's been on my mind is this: The default model is rtl8139. 
When libvirt is auto-assigning PCI addresses to devices at parse time, 
it decides whether to assign a network device to a conventional PCI or 
PCI Express slot based on the model, and rtl8139 is conventional PCI. So 
if you have <interface type='network'> where the network is a pool of 
hostdevs, and if you don't assign a "fake" model like "virtio" or 
"e1000e", then the hostdev device (which is 100% certainly a PCIe 
device) will be assigned to a conventional PCI slot. That works, but 
is.... "sub-optimal" :-)

I think we will still need to add a bit to 
qemuDomainDeviceCalculatePCIConnectFlags() in order to get the right 
type of slot set, but this is a good start.

Reviewed-by: Laine Stump <laine@redhat.com>

Thanks, and congratulations on your first libvirt patch!

---
>   src/qemu/qemu_domain.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2dad823a86..33ce0ad992 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -5831,6 +5831,7 @@ qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
>                                   virQEMUCapsPtr qemuCaps)
>   {
>       if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> +        virDomainNetResolveActualType(net) != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
>           !virDomainNetGetModelString(net))
>           net->model = qemuDomainDefaultNetModel(def, qemuCaps);
>

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Paulo de Rezende Pinatti 3 years, 10 months ago

On 17/06/20 06:01, Laine Stump wrote:
> On 6/16/20 10:32 AM, Paulo de Rezende Pinatti wrote:
>> No default model should be added to the interface
>> entry at post parse when its actual network type is hostdev
>> as doing so might cause a mismatch between the interface
>> definition and its actual device type.
> 
> 
> Have you encountered a real problem from this? (I have, and have been 
> thinking about the issue for awhile, but only late at night when I'm not 
> near my keyboard to do something about it. I'm just wondering what 
> problem you've had :-))

Yes I have. I first defined a passthrough network out of a PF and then 
created the domain with an interface definition '<source 
network=passthrough/>' and no model (basically the process outlined in 
https://wiki.libvirt.org/page/Networking#Assignment_from_a_pool_of_SRIOV_VFs_in_a_libvirt_%3Cnetwork%3E_definition) 
, but after saving it libvirt automatically added <model type='virtio'> 
(that's the default model on S390) which led to a qemu error when trying 
to start the vm (see below).

> 
> 
> The reason it's been on my mind is this: The default model is rtl8139. 
> When libvirt is auto-assigning PCI addresses to devices at parse time, 
> it decides whether to assign a network device to a conventional PCI or 
> PCI Express slot based on the model, and rtl8139 is conventional PCI. So 
> if you have <interface type='network'> where the network is a pool of 
> hostdevs, and if you don't assign a "fake" model like "virtio" or 
> "e1000e", then the hostdev device (which is 100% certainly a PCIe 
> device) will be assigned to a conventional PCI slot. That works, but 
> is.... "sub-optimal" :-)
> 

For me it didn't work at all because model virtio on S390 leads to a 
'devno' property being specified which of course is wrong for this type 
of device (i.e. '-device 
vfio-pci,host=0100:00:08.2,id=hostdev0,devno=fe.0.0006').

> 
> I think we will still need to add a bit to 
> qemuDomainDeviceCalculatePCIConnectFlags() in order to get the right 
> type of slot set, but this is a good start.
> 
> 
> Reviewed-by: Laine Stump <laine@redhat.com>
> 
> 
> Thanks, and congratulations on your first libvirt patch!
> 

Thanks! :)


> 
> 
> ---
>>   src/qemu/qemu_domain.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 2dad823a86..33ce0ad992 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -5831,6 +5831,7 @@ 
>> qemuDomainDeviceNetDefPostParse(virDomainNetDefPtr net,
>>                                   virQEMUCapsPtr qemuCaps)
>>   {
>>       if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
>> +        virDomainNetResolveActualType(net) != 
>> VIR_DOMAIN_NET_TYPE_HOSTDEV &&
>>           !virDomainNetGetModelString(net))
>>           net->model = qemuDomainDefaultNetModel(def, qemuCaps);
> 
> 

-- 
Best regards,

Paulo de Rezende Pinatti

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Laine Stump 3 years, 10 months ago

On 6/17/20 4:54 AM, Paulo de Rezende Pinatti wrote:
>
> On 17/06/20 06:01, Laine Stump wrote:
>> On 6/16/20 10:32 AM, Paulo de Rezende Pinatti wrote:
>>> No default model should be added to the interface
>>> entry at post parse when its actual network type is hostdev
>>> as doing so might cause a mismatch between the interface
>>> definition and its actual device type.
>>
>>
>> Have you encountered a real problem from this? (I have, and have been 
>> thinking about the issue for awhile, but only late at night when I'm 
>> not near my keyboard to do something about it. I'm just wondering 
>> what problem you've had :-))
>
> Yes I have. I first defined a passthrough network out of a PF and then 
> created the domain with an interface definition '<source 
> network=passthrough/>' and no model (basically the process outlined in 
> https://wiki.libvirt.org/page/Networking#Assignment_from_a_pool_of_SRIOV_VFs_in_a_libvirt_%3Cnetwork%3E_definition) 
> , but after saving it libvirt automatically added <model 
> type='virtio'> (that's the default model on S390) which led to a qemu 
> error when trying to start the vm (see below).
>
>>
>>
>> The reason it's been on my mind is this: The default model is 
>> rtl8139. When libvirt is auto-assigning PCI addresses to devices at 
>> parse time, it decides whether to assign a network device to a 
>> conventional PCI or PCI Express slot based on the model, and rtl8139 
>> is conventional PCI. So if you have <interface type='network'> where 
>> the network is a pool of hostdevs, and if you don't assign a "fake" 
>> model like "virtio" or "e1000e", then the hostdev device (which is 
>> 100% certainly a PCIe device) will be assigned to a conventional PCI 
>> slot. That works, but is.... "sub-optimal" :-)
>>
>
> For me it didn't work at all because model virtio on S390 leads to a 
> 'devno' property being specified which of course is wrong for this 
> type of device (i.e. '-device 
> vfio-pci,host=0100:00:08.2,id=hostdev0,devno=fe.0.0006').


Okay, so that's a similar problem to mine - in my case the 
address-assignment code makes the choice of "conventional PCI" instead 
of "PCI Express", which isn't great, but still works. In your case it 
mistakenly thinks "CCW" instead of "PCI". Since that's not been reported 
before (that I know of) I guess that means you're the first to use a 
network pool of hostdevs on s390!


I just sent a patch to auto-assign these devices to PCIe on platforms 
that have PCIe. I Cc'ed you so that you might apply the patch locally 
and verify that I didn't break anything for you.


>
>
>>
>> I think we will still need to add a bit to 
>> qemuDomainDeviceCalculatePCIConnectFlags() in order to get the right 
>> type of slot set, but this is a good start.


I just sent a patch to auto-assign these devices to PCIe on platforms 
that have PCIe. I Cc'ed you so that you might apply the patch locally 
and verify that I didn't break anything for you.

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Boris Fiuczynski 3 years, 10 months ago

On 6/17/20 6:01 AM, Laine Stump wrote:
> On 6/16/20 10:32 AM, Paulo de Rezende Pinatti wrote:
>> No default model should be added to the interface
>> entry at post parse when its actual network type is hostdev
>> as doing so might cause a mismatch between the interface
>> definition and its actual device type.
> 
> 
> Have you encountered a real problem from this? (I have, and have been 
> thinking about the issue for awhile, but only late at night when I'm not 
> near my keyboard to do something about it. I'm just wondering what 
> problem you've had :-))
> 
> 
> The reason it's been on my mind is this: The default model is rtl8139. 
> When libvirt is auto-assigning PCI addresses to devices at parse time, 
> it decides whether to assign a network device to a conventional PCI or 
> PCI Express slot based on the model, and rtl8139 is conventional PCI. So 
> if you have <interface type='network'> where the network is a pool of 
> hostdevs, and if you don't assign a "fake" model like "virtio" or 
> "e1000e", then the hostdev device (which is 100% certainly a PCIe 
> device) will be assigned to a conventional PCI slot. That works, but 
> is.... "sub-optimal" :-)
> 
> 
> I think we will still need to add a bit to 
> qemuDomainDeviceCalculatePCIConnectFlags() in order to get the right 
> type of slot set, but this is a good start.
> 
> 
> Reviewed-by: Laine Stump <laine@redhat.com>
> 
> 
> Thanks, and congratulations on your first libvirt patch!

Hi Laine,
do you plan to push this patch?


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Laine Stump 3 years, 10 months ago

On 6/17/20 12:55 PM, Boris Fiuczynski wrote:

> 
> Hi Laine,
> do you plan to push this patch?

Already did, about an hour ago.

Re: [PATCH] qemu: do not add model when actual iface type is hostdev

Posted by Boris Fiuczynski 3 years, 10 months ago

On 6/17/20 7:06 PM, Laine Stump wrote:
> On 6/17/20 12:55 PM, Boris Fiuczynski wrote:
> 
>>
>> Hi Laine,
>> do you plan to push this patch?
> 
> Already did, about an hour ago.
> 
Thanks and sorry I missed it.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294