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

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 19:52, Michael S. Tsirkin wrote:
> >On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
> >>On 29/04/2013 19:01, Michael S. Tsirkin wrote:
> >>
> >>     On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> >>
> >>         On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> >>
> >>             On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin 
> >> wrote:
> >>
> >>                 On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric 
> >> wrote:
> >>
> >>                     On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> >>
> >>                         On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse 
> >> Larrew wrote:
> >>
> >>                             On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> >>
> >>                                 On 29/04/2013 17:14, Jesse Larrew wrote:
> >>
> >>                                     On 04/29/2013 09:55 AM, KONRAD 
> >> Frédéric wrote:
> >>
> >>                                         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?
> >>
> >>
> >>                                     I suspect this bug was present in 
> >> 14f9b664 as well. We just hadn't triggered
> >>                                     it yet. I'll confirm this afternoon.
> >>
> >>                                 Ok, I think there is a problem with your 
> >> patch:
> >>
> >>                                     virtio_init(VIRTIO_DEVICE(n), 
> >> "virtio-net", VIRTIO_ID_NET,
> >>                                                                   
> >> n->config_size);
> >>
> >>                                 is called in virtio_net_device_init 
> >> (k->init).
> >>
> >>                                 Is there a way to resize the config after 
> >> that?
> >>
> >>
> >>                             Nope. That's definitely a bug. I can send a 
> >> patch to fix this today. Any
> >>                             objection to the method suggested above 
> >> (extending VirtioDeviceClass)?
> >>
> >>                         This needs more thought. All devices expected 
> >> everything
> >>                         is setup during the init call. config size is
> >>                         likely not the only issue.
> >>
> >>                         So we need almost all of virtio_bus_plug_device 
> >> before init,
> >>                         and then after init send the signal:
> >>
> >>                             if (klass->device_plugged != NULL) {
> >>                                 klass->device_plugged(qbus->parent);
> >>                             }
> >>
> >>                     Seems the interesting part is in 
> >> virtio_pci_device_plugged at the end:
> >>
> >>                         proxy->host_features |= 0x1 << 
> >> VIRTIO_F_NOTIFY_ON_EMPTY;
> >>                         proxy->host_features |= 0x1 << 
> >> VIRTIO_F_BAD_FEATURE;
> >>                         proxy->host_features = 
> >> virtio_bus_get_vdev_features(bus,
> >>                     proxy->host_features);
> >>
> >>                     This is and was called after set_config_size, isn't 
> >> that the issue?
> >>
> >>                 Basically devices expected everything to be setup at
> >>                 the point where init is called.
> >>                 We found one bug but let's fix it properly.
> >>                 There's no reason to delay parts of setup until after init,
> >>                 if we do, we'll trip on more uses of uninitialized data.
> >>
> >>
> >>                                                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
> >>
> >>
> >>                             --
> >>
> >>                             Jesse Larrew
> >>                             Software Engineer, KVM Team
> >>                             IBM Linux Technology Center
> >>                             Phone: (512) 973-2052 (T/L: 363-2052)
> >>                             address@hidden
> >>
> >>             Basically this is what I propose. Warning! compile-tested
> >>             only. (I also dropped an unused return value).
> >>
> >>
> >>             Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>
> >>         Which tree are you using? Is it up to date?
> >>
> >>     Heh, more changes came in.  So now, it's even simpler:
> >>
> >>     Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>
> >>     ---
> >>
> >>     diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> >>     index aab72ff..447ba16 100644
> >>     --- a/hw/virtio/virtio-bus.c
> >>     +++ b/hw/virtio/virtio-bus.c
> >>     @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } 
> >> while (0)
> >>      #endif
> >>
> >>      /* Plug the VirtIODevice */
> >>     -int virtio_bus_plug_device(VirtIODevice *vdev)
> >>     +void virtio_bus_plug_device(VirtIODevice *vdev)
> >>      {
> >>          DeviceState *qdev = DEVICE(vdev);
> >>          BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> >>     @@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
> >>          if (klass->device_plugged != NULL) {
> >>              klass->device_plugged(qbus->parent);
> >>          }
> >>     -
> >>     -    return 0;
> >>      }
> >>
> >>      /* Reset the virtio_bus */
> >>     diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>     index 0f88c25..c5228e6 100644
> >>     --- a/hw/virtio/virtio.c
> >>     +++ b/hw/virtio/virtio.c
> >>     @@ -1091,11 +1091,11 @@ static int virtio_device_init(DeviceState 
> >> *qdev)
> >>      {
> >>          VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> >>          VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> >>     +    virtio_bus_plug_device(vdev);
> >>          assert(k->init != NULL);
> >>          if (k->init(vdev) < 0) {
> >>              return -1;
> >>          }
> >>     -    virtio_bus_plug_device(vdev);
> >>          return 0;
> >>      }
> >>
> >>
> >>Not sure this is what you want to do.
> >>The device would be plugged before it is inited :/.
> >I think this is exacly what we want to do.
> >In fact, this is what other buses do, because
> >devices simply can't init properly if they
> >do not know on which bus they reside.
> >
> >E.g. with pci:
> >     do_pci_register_device (adds device on bus)
> >     init
> >
> >We can add an analog of hotplug bus callback
> >if bus wants to get notified after device initialization.
> >I don't see a need for this though.
> >Do you?
> >
> >
> >
> No, as we don't want to allow virtio-device hotplug.
> 
> but look at the plugged callback in virtio-pci:
> 
>     pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
> 
>     proxy->host_features = virtio_bus_get_vdev_features(bus,
> proxy->host_features);
> 
> are impossible before the virtio-net init.


At this point I have to admit I don't understand what's
going on any more.

virtio_net_instance_init sets config size to
some value, then virtio_net_set_config_size overrides it...
Help!

I am guessing it's this hack that backfires somehow.

-- 
MST