Re: [PATCH] conf: Introduce igb model for <interface>
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(a)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