Virtualization - Jul 2022 - VIRTIO_NET_F_MTU not negotiated

On Thu, Jul 28, 2022 at 1:39 PM Eli Cohen <elic at nvidia.com>
wrote:
>
> > From: Jason Wang <jasowang at redhat.com>
> > Sent: Thursday, July 28, 2022 5:09 AM
> > To: Eli Cohen <elic at nvidia.com>
> > Cc: Eugenio Perez Martin <eperezma at redhat.com>; qemu-devel at
nongnu.org; Michael S. Tsirkin <mst at redhat.com>;
> > virtualization at lists.linux-foundation.org
> > Subject: Re: VIRTIO_NET_F_MTU not negotiated
> >
> > On Wed, Jul 27, 2022 at 2:52 PM Eli Cohen <elic at nvidia.com>
wrote:
> > >
> > > I found out that the reason why I could not enforce the mtu stems
from the fact that I did not configure max mtu for the net device
> > (e.g. through libvirt <mtu size="9000"/>).
> > > Libvirt does not allow this configuration for vdpa devices and
probably for a reason. The vdpa backend driver has the freedom to do
> > it using its copy of virtio_net_config.
> > >
> > > The code in qemu that is responsible to allow to consider the
device MTU restriction is here:
> > >
> > > static void virtio_net_device_realize(DeviceState *dev, Error
**errp)
> > > {
> > >     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > >     VirtIONet *n = VIRTIO_NET(dev);
> > >     NetClientState *nc;
> > >     int i;
> > >
> > >     if (n->net_conf.mtu) {
> > >         n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
> > >     }
> > >
> > > The above code can be interpreted as follows:
> > > if the command line arguments of qemu indicates that mtu should
be limited, then we would read this mtu limitation from the
> > device (that actual value is ignored).
> > >
> > > I worked around this limitation by unconditionally setting
VIRTIO_NET_F_MTU in the host features. As said, it only indicates that
> > we should read the actual limitation for the device.
> > >
> > > If this makes sense I can send a patch to fix this.
> >
> > I wonder whether it's worth to bother:
> >
> > 1) mgmt (above libvirt) should have the knowledge to prepare the
correct XML
> > 2) it's not specific to MTU, we had other features work like, for
> > example, the multiqueue?
> >
>
>
> Currently libvirt does not recognize setting the mtu through XML for vdpa
device. So you mean the fix should go to libvirt?
Probably.
> Furthermore, even if libvirt supports MTU configuration for a vdpa device,
the actual value provided will be ignored and the limitation will be taken from
what the vdpa device published in its virtio_net_config structure. That makes
the XML configuration binary.
Yes, we suffer from a similar issue for "queues=". I think we should
fix qemu by failing the initialization if the value provided by cli
doesn't match what is read from config space.

E.g when mtu=9000 was set by cli but the actual mtu is 1500.

Thanks
>
> > Thanks
>

On Thu, Jul 28, 2022 at 01:51:37PM +0800, Jason Wang
wrote:
> On Thu, Jul 28, 2022 at 1:39 PM Eli Cohen <elic at nvidia.com> wrote:
> >
> > > From: Jason Wang <jasowang at redhat.com>
> > > Sent: Thursday, July 28, 2022 5:09 AM
> > > To: Eli Cohen <elic at nvidia.com>
> > > Cc: Eugenio Perez Martin <eperezma at redhat.com>;
qemu-devel at nongnu.org; Michael S. Tsirkin <mst at redhat.com>;
> > > virtualization at lists.linux-foundation.org
> > > Subject: Re: VIRTIO_NET_F_MTU not negotiated
> > >
> > > On Wed, Jul 27, 2022 at 2:52 PM Eli Cohen <elic at
nvidia.com> wrote:
> > > >
> > > > I found out that the reason why I could not enforce the mtu
stems from the fact that I did not configure max mtu for the net device
> > > (e.g. through libvirt <mtu size="9000"/>).
> > > > Libvirt does not allow this configuration for vdpa devices
and probably for a reason. The vdpa backend driver has the freedom to do
> > > it using its copy of virtio_net_config.
> > > >
> > > > The code in qemu that is responsible to allow to consider
the device MTU restriction is here:
> > > >
> > > > static void virtio_net_device_realize(DeviceState *dev,
Error **errp)
> > > > {
> > > >     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > > >     VirtIONet *n = VIRTIO_NET(dev);
> > > >     NetClientState *nc;
> > > >     int i;
> > > >
> > > >     if (n->net_conf.mtu) {
> > > >         n->host_features |= (1ULL <<
VIRTIO_NET_F_MTU);
> > > >     }
> > > >
> > > > The above code can be interpreted as follows:
> > > > if the command line arguments of qemu indicates that mtu
should be limited, then we would read this mtu limitation from the
> > > device (that actual value is ignored).
> > > >
> > > > I worked around this limitation by unconditionally setting
VIRTIO_NET_F_MTU in the host features. As said, it only indicates that
> > > we should read the actual limitation for the device.
> > > >
> > > > If this makes sense I can send a patch to fix this.
> > >
> > > I wonder whether it's worth to bother:
> > >
> > > 1) mgmt (above libvirt) should have the knowledge to prepare the
correct XML
> > > 2) it's not specific to MTU, we had other features work like,
for
> > > example, the multiqueue?
> > >
> >
> >
> > Currently libvirt does not recognize setting the mtu through XML for
vdpa device. So you mean the fix should go to libvirt?
> 
> Probably.
> 
> > Furthermore, even if libvirt supports MTU configuration for a vdpa
device, the actual value provided will be ignored and the limitation will be
taken from what the vdpa device published in its virtio_net_config structure.
That makes the XML configuration binary.
> 
> Yes, we suffer from a similar issue for "queues=". I think we
should
> fix qemu by failing the initialization if the value provided by cli
> doesn't match what is read from config space.
> 
> E.g when mtu=9000 was set by cli but the actual mtu is 1500.
> 
> Thanks

Jason most features are passthrough now, no?
Why do you want to make this one special?


> >
> > > Thanks
> >