src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + src/qemu/qemu_domain_address.c | 3 ++- src/qemu/qemu_interface.c | 1 + 4 files changed, 5 insertions(+), 1 deletion(-)
igb is a new network device which will be introduced with QEMU 8.0.0.
It is a successor of e1000e so it has PCIe interface and is understands
virtio-net headers as e1000e does.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
src/conf/domain_conf.c | 1 +
src/conf/domain_conf.h | 1 +
src/qemu/qemu_domain_address.c | 3 ++-
src/qemu/qemu_interface.c | 1 +
4 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d16a247a45..25401f742f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -587,6 +587,7 @@ VIR_ENUM_IMPL(virDomainNetModel,
"virtio",
"e1000",
"e1000e",
+ "igb",
"virtio-transitional",
"virtio-non-transitional",
"usb-net",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9e281692ff..44b070ba0c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -954,6 +954,7 @@ typedef enum {
VIR_DOMAIN_NET_MODEL_VIRTIO,
VIR_DOMAIN_NET_MODEL_E1000,
VIR_DOMAIN_NET_MODEL_E1000E,
+ VIR_DOMAIN_NET_MODEL_IGB,
VIR_DOMAIN_NET_MODEL_VIRTIO_TRANSITIONAL,
VIR_DOMAIN_NET_MODEL_VIRTIO_NON_TRANSITIONAL,
VIR_DOMAIN_NET_MODEL_USB_NET,
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b8d1969fbe..9f13f5f195 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -750,7 +750,8 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDef *dev,
if (net->model == VIR_DOMAIN_NET_MODEL_VIRTIO_TRANSITIONAL)
return pciFlags;
- if (net->model == VIR_DOMAIN_NET_MODEL_E1000E)
+ if (net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
+ net->model == VIR_DOMAIN_NET_MODEL_IGB)
return pcieFlags;
/* the only time model can be "unknown" is for type='hostdev'
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 264d5e060c..2aa4d53215 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -241,6 +241,7 @@ qemuInterfaceIsVnetCompatModel(const virDomainNetDef *net)
{
return (virDomainNetIsVirtioModel(net) ||
net->model == VIR_DOMAIN_NET_MODEL_E1000E ||
+ net->model == VIR_DOMAIN_NET_MODEL_IGB ||
net->model == VIR_DOMAIN_NET_MODEL_VMXNET3);
}
--
2.40.0On 4/10/23 07:48, Akihiko Odaki wrote: > igb is a new network device which will be introduced with QEMU 8.0.0. > It is a successor of e1000e so it has PCIe interface and is understands > virtio-net headers as e1000e does. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > src/conf/domain_conf.c | 1 + > src/conf/domain_conf.h | 1 + > src/qemu/qemu_domain_address.c | 3 ++- > src/qemu/qemu_interface.c | 1 + > 4 files changed, 5 insertions(+), 1 deletion(-) > This looks almost perfect. What is missing is: 1) documentation - might be worth including this onto the list of models, e.g. like this: diff --git i/docs/formatdomain.rst w/docs/formatdomain.rst index 27f83e254d..388c620221 100644 --- i/docs/formatdomain.rst +++ w/docs/formatdomain.rst @@ -5409,6 +5409,7 @@ Typical values for QEMU and KVM include: ne2k_isa i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio. :since:`Since 5.2.0` , ``virtio-transitional`` and ``virtio-non-transitional`` values are supported. See `Virtio transitional devices`_ for more details. +:since:`Since 9.3.0` igb is also supported. Setting NIC driver-specific options ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 2) QEMU capability, we have an arbitrary list of features that we query QEMU for. This is mostly so that we can check whether QEMU supports requested feature or not. And this new model is so new, that the minimum required QEMU version - 4.2.0 (grep QEMU_MIN_ src/qemu/qemu_capabilities.c) doesn't have it. This is not trivial though, and we'll need Peter's help to regenerate capabilities. IIUC, the IGB was introduced in QEMU commit of v7.2.0-2481-g3a977deebe but the latest capabilities data we have is from v7.2.0-2146-g2946e1af27, i.e. ~350 commits older. 3) A test case. Especially step 2) is going to be tricky. So let me suggest the following: after Peter kindly refreshes capabilities, I'll write missing patches and resend among with yours (keeping your authorship of the patch, of course). Does that work for you? Michal
On Wed, Apr 12, 2023 at 18:23:50 +0200, Michal Prívozník wrote: > On 4/10/23 07:48, Akihiko Odaki wrote: > > igb is a new network device which will be introduced with QEMU 8.0.0. > > It is a successor of e1000e so it has PCIe interface and is understands > > virtio-net headers as e1000e does. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > src/conf/domain_conf.c | 1 + > > src/conf/domain_conf.h | 1 + > > src/qemu/qemu_domain_address.c | 3 ++- > > src/qemu/qemu_interface.c | 1 + > > 4 files changed, 5 insertions(+), 1 deletion(-) [...] > 2) QEMU capability, we have an arbitrary list of features that we query > QEMU for. This is mostly so that we can check whether QEMU supports > requested feature or not. And this new model is so new, that the minimum > required QEMU version - 4.2.0 (grep QEMU_MIN_ > src/qemu/qemu_capabilities.c) doesn't have it. This is not trivial > though, and we'll need Peter's help to regenerate capabilities. > > IIUC, the IGB was introduced in QEMU commit of v7.2.0-2481-g3a977deebe > but the latest capabilities data we have is from > v7.2.0-2146-g2946e1af27, i.e. ~350 commits older. At this point I wanted to do a final update once the release is out since qemu is in 'rc3' but for now I've posted the update for you as there is a need for it: https://listman.redhat.com/archives/libvir-list/2023-April/239342.html
On 2023/04/13 1:23, Michal Prívozník wrote: > On 4/10/23 07:48, Akihiko Odaki wrote: >> igb is a new network device which will be introduced with QEMU 8.0.0. >> It is a successor of e1000e so it has PCIe interface and is understands >> virtio-net headers as e1000e does. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> src/conf/domain_conf.c | 1 + >> src/conf/domain_conf.h | 1 + >> src/qemu/qemu_domain_address.c | 3 ++- >> src/qemu/qemu_interface.c | 1 + >> 4 files changed, 5 insertions(+), 1 deletion(-) >> > > This looks almost perfect. What is missing is: > > 1) documentation - might be worth including this onto the list of > models, e.g. like this: > > diff --git i/docs/formatdomain.rst w/docs/formatdomain.rst > index 27f83e254d..388c620221 100644 > --- i/docs/formatdomain.rst > +++ w/docs/formatdomain.rst > @@ -5409,6 +5409,7 @@ Typical values for QEMU and KVM include: ne2k_isa > i82551 i82557b i82559er > ne2k_pci pcnet rtl8139 e1000 virtio. :since:`Since 5.2.0` , > ``virtio-transitional`` and ``virtio-non-transitional`` values are > supported. > See `Virtio transitional devices`_ for more details. > +:since:`Since 9.3.0` igb is also supported. > > Setting NIC driver-specific options > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > 2) QEMU capability, we have an arbitrary list of features that we query > QEMU for. This is mostly so that we can check whether QEMU supports > requested feature or not. And this new model is so new, that the minimum > required QEMU version - 4.2.0 (grep QEMU_MIN_ > src/qemu/qemu_capabilities.c) doesn't have it. This is not trivial > though, and we'll need Peter's help to regenerate capabilities. > > IIUC, the IGB was introduced in QEMU commit of v7.2.0-2481-g3a977deebe > but the latest capabilities data we have is from > v7.2.0-2146-g2946e1af27, i.e. ~350 commits older. I doubt that a capability is necessary for igb. e1000e is the predecessor of the lineage of Intel NICs, but libvirt does not define one for it. e1000 is an even older Intel NIC, and it indeed has a capability defined for, but I suspect the capability is defined just to be queried later in qemuDomainDefaultNetModel(). As we are not changing the default network model this time, it may be fine even if we do not add a capability. > > > 3) A test case. > > > Especially step 2) is going to be tricky. So let me suggest the > following: after Peter kindly refreshes capabilities, I'll write missing > patches and resend among with yours (keeping your authorship of the > patch, of course). Does that work for you? Yes, thanks in advance for writing and submitting patches. Regards, Akihiko Odaki > > > Michal >
On 4/12/23 18:51, Akihiko Odaki wrote: > On 2023/04/13 1:23, Michal Prívozník wrote: >> On 4/10/23 07:48, Akihiko Odaki wrote: >>> igb is a new network device which will be introduced with QEMU 8.0.0. >>> It is a successor of e1000e so it has PCIe interface and is understands >>> virtio-net headers as e1000e does. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> src/conf/domain_conf.c | 1 + >>> src/conf/domain_conf.h | 1 + >>> src/qemu/qemu_domain_address.c | 3 ++- >>> src/qemu/qemu_interface.c | 1 + >>> 4 files changed, 5 insertions(+), 1 deletion(-) >>> >> >> This looks almost perfect. What is missing is: >> >> 1) documentation - might be worth including this onto the list of >> models, e.g. like this: >> >> diff --git i/docs/formatdomain.rst w/docs/formatdomain.rst >> index 27f83e254d..388c620221 100644 >> --- i/docs/formatdomain.rst >> +++ w/docs/formatdomain.rst >> @@ -5409,6 +5409,7 @@ Typical values for QEMU and KVM include: ne2k_isa >> i82551 i82557b i82559er >> ne2k_pci pcnet rtl8139 e1000 virtio. :since:`Since 5.2.0` , >> ``virtio-transitional`` and ``virtio-non-transitional`` values are >> supported. >> See `Virtio transitional devices`_ for more details. >> +:since:`Since 9.3.0` igb is also supported. >> >> Setting NIC driver-specific options >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> >> 2) QEMU capability, we have an arbitrary list of features that we query >> QEMU for. This is mostly so that we can check whether QEMU supports >> requested feature or not. And this new model is so new, that the minimum >> required QEMU version - 4.2.0 (grep QEMU_MIN_ >> src/qemu/qemu_capabilities.c) doesn't have it. This is not trivial >> though, and we'll need Peter's help to regenerate capabilities. >> >> IIUC, the IGB was introduced in QEMU commit of v7.2.0-2481-g3a977deebe >> but the latest capabilities data we have is from >> v7.2.0-2146-g2946e1af27, i.e. ~350 commits older. > > I doubt that a capability is necessary for igb. e1000e is the > predecessor of the lineage of Intel NICs, but libvirt does not define > one for it. That's because e1000e device was introduced in qemu-2.7.0 (v2.7.0-rc0~157^2~11) and the minimal version that libvirt requires is 4.2.0. And there's no way to compile QEMU without the device, is there? Therefore, libvirt can safely assume the device is always present. But that's not the case for this brand new IGB device. > > e1000 is an even older Intel NIC, and it indeed has a capability defined > for, but I suspect the capability is defined just to be queried later in > qemuDomainDefaultNetModel(). As we are not changing the default network > model this time, it may be fine even if we do not add a capability. > Yeah, so this something that I've been discussing with other libvirt developers occasionally a lot recently. The only advantage of having the capability is: it'll be libvirt who reports an error instead of QEMU. And I don't see much value in that. [1]. Nevertheless, our current policy hasn't changed and thus we do need the capability. 1: There are some valuable capabilities though. For instance those, which enable libvirt to use newer style of cmd line if the capability is present, and fall back to the older style if it isn't. >> >> >> 3) A test case. >> >> >> Especially step 2) is going to be tricky. So let me suggest the >> following: after Peter kindly refreshes capabilities, I'll write missing >> patches and resend among with yours (keeping your authorship of the >> patch, of course). Does that work for you? > > Yes, thanks in advance for writing and submitting patches. Deal. Michal
On 4/13/23 08:24, Michal Prívozník wrote:
> On 4/12/23 18:51, Akihiko Odaki wrote:
>> On 2023/04/13 1:23, Michal Prívozník wrote:
>>> On 4/10/23 07:48, Akihiko Odaki wrote:
>>>> igb is a new network device which will be introduced with QEMU 8.0.0.
>>>> It is a successor of e1000e so it has PCIe interface and is understands
>>>> virtio-net headers as e1000e does.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>> src/conf/domain_conf.c | 1 +
>>>> src/conf/domain_conf.h | 1 +
>>>> src/qemu/qemu_domain_address.c | 3 ++-
>>>> src/qemu/qemu_interface.c | 1 +
>>>> 4 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>
>>> This looks almost perfect. What is missing is:
>>>
>>> 1) documentation - might be worth including this onto the list of
>>> models, e.g. like this:
>>>
>>> diff --git i/docs/formatdomain.rst w/docs/formatdomain.rst
>>> index 27f83e254d..388c620221 100644
>>> --- i/docs/formatdomain.rst
>>> +++ w/docs/formatdomain.rst
>>> @@ -5409,6 +5409,7 @@ Typical values for QEMU and KVM include: ne2k_isa
>>> i82551 i82557b i82559er
>>> ne2k_pci pcnet rtl8139 e1000 virtio. :since:`Since 5.2.0` ,
>>> ``virtio-transitional`` and ``virtio-non-transitional`` values are
>>> supported.
>>> See `Virtio transitional devices`_ for more details.
>>> +:since:`Since 9.3.0` igb is also supported.
>>>
>>> Setting NIC driver-specific options
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>>
>>> 2) QEMU capability, we have an arbitrary list of features that we query
>>> QEMU for. This is mostly so that we can check whether QEMU supports
>>> requested feature or not. And this new model is so new, that the minimum
>>> required QEMU version - 4.2.0 (grep QEMU_MIN_
>>> src/qemu/qemu_capabilities.c) doesn't have it. This is not trivial
>>> though, and we'll need Peter's help to regenerate capabilities.
>>>
>>> IIUC, the IGB was introduced in QEMU commit of v7.2.0-2481-g3a977deebe
>>> but the latest capabilities data we have is from
>>> v7.2.0-2146-g2946e1af27, i.e. ~350 commits older.
>>
>> I doubt that a capability is necessary for igb. e1000e is the
>> predecessor of the lineage of Intel NICs, but libvirt does not define
>> one for it.
>
> That's because e1000e device was introduced in qemu-2.7.0
> (v2.7.0-rc0~157^2~11) and the minimal version that libvirt requires is
> 4.2.0. And there's no way to compile QEMU without the device, is there?
> Therefore, libvirt can safely assume the device is always present. But
> that's not the case for this brand new IGB device.
Ah, spoke too soon. So apparently, for <interface/> models, we do
explicitly allow free form strings. I mean, inside of _virDomainNetDef
struct we have:
int model; /* virDomainNetModelType */
char *modelstr;
and then we have virDomainNetSetModelString() and
virDomainNetGetModelString() which either set/get model member or
modelstr member if it's unknown to libvirt. This means, we can do
validation only for 'model' member. So let's stick with what you have
and NOT do any validation nor capability.
I'm squashing in the docs hunk from above and merging.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Congratulations on your first libvirt contribution!
Michal
© 2016 - 2024 Red Hat, Inc.