Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Michael S. Tsirkin]

On Thu, Feb 07, 2013 at 11:46:55AM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Thu, Feb 07, 2013 at 09:45:54AM -0600, Anthony Liguori wrote:
> >> "Michael S. Tsirkin" <address@hidden> writes:
> >> 
> >> This is about bug-for-bug compatibility with older QEMU.
> >
> > Sorry, with which version?
> >
> > It looks like if I run qemu 1.3 with .status=off
> > windows drivers work. If I do this on 1.4 they break.
> > I don't see much value in knowingly breaking working setups
> > like this.
> 
> The whole discussion is moot because there's no version of QEMU that we
> support backwards compatibility for that didn't have status enabled by
> default.
> 
> What we need to worry about supporting is compat machines.  Not random
> combination of feature flags that no actual user uses.
> 
> As a general rule, we need to make sure that the devices we expose look
> the same when doing -M.  That means the config space size needs to be
> the same as it was in previous machines.
> This is a pretty simple thing.
> 
> Regards,
> 
> Anthony Liguori
> >
> > -- 
> > MST

So why do we add extra code who's sole effect is breaking old
windows drivers?
I like the idea but why add code for status and mac features?
Also, would be nicer not to rely on the fact we always set MAC
flag at the moment. What if we make it optional?

We also don't really need the {} tag at the end - we have ARRAY_SIZE
macro.  Why don't we do:

static VirtIOFeature feature_sizes[] = {
   { .flags = 0, /* default size when no flags are set */
    .end = offsetof(struct virtio_net_config, max_virtqueue_pairs)},
   {.flags = 1 << VIRTIO_NET_F_MQ,
    .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
};


And:

int i, config_size = 0;

  for (i = 0; i < ARRAY_SIZE(feature_sizes); i++) {
      if (!feature_sizes[i].flags || host_features & feature_sizes[i].flags) {
          config_size = MAX(feature_sizes[i].end, config_size);
       }
   }

This handles the windows bug nicely and will work for adding
features going forward, without changing size for -M pc-1.3.


-- 
MST