Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[KONRAD Frédéric]

On 29/04/2013 16:42, Jesse Larrew wrote:
> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> > On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> > > Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> > > calculate config size based on the host features. But it forgets the
> > > VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> > > len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> > > disabled form command line. Then qemu will crash when user tries to read the
> > > config of virtio-net.
> > >
> > > Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> > > the mac address.
> > >
> > > Cc: Jesse Larrew <address@hidden>
> > > Signed-off-by: Jason Wang <address@hidden>
> > > ---
> > >   hw/net/virtio-net.c |    3 ++-
> > >   1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 70c8fce..33a70ef 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice 
> > > *vdev, int idx,
> > >   
> > >   void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> > >   {
> > > -    int i, config_size = 0;
> > > +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> > > +    int i, config_size = feature_sizes[0].end;
> > This would be cleaner:
> >         host_features |= (1 << VIRTIO_NET_F_MAC);
> >
> > no need for a comment then.
> It seems to me that the real problem here is that host_features isn't
> properly populated before virtio_net_set_config_size() is called. Looking
> at virtio_device_init(), we can see why:
>
> static int virtio_device_init(DeviceState *qdev)
> {
>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>      assert(k->init != NULL);
>      if (k->init(vdev) < 0) {
>          return -1;
>      }
>      virtio_bus_plug_device(vdev);
>      return 0;
> }
>
> virtio_net_set_config_size() is currently being called as part of the
> k->init call, but the host_features aren't properly setup until the device
> is plugged into the bus using virtio_bus_plug_device().
>
> After talking with mdroth, I think the proper way to fix this would be to
> extend VirtioDeviceClass to include a calculate_config_size() method that
> can be called at the appropriate time during device initialization like so:
>
> static int virtio_device_init(DeviceState *qdev)
> {
>      VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>      assert(k->init != NULL);
>      if (k->init(vdev) < 0) {
>          return -1;
>      }
>      virtio_bus_plug_device(vdev);
> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> +       return -1;
> +   }
>      return 0;
> }
>
> This would ensure that host_features contains the proper data before any
> devices try to make use of it to calculate the config size.
Good point, I didn't saw that.

but this was not the case with commit 14f9b664 no?

> > >       for (i = 0; feature_sizes[i].flags != 0; i++) {
> > >           if (host_features & feature_sizes[i].flags) {
> > >               config_size = MAX(feature_sizes[i].end, config_size);
> > > --
> > > 1.7.1
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> address@hidden