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 03:15:42PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Thu, Feb 07, 2013 at 11:46:55AM -0600, Anthony Liguori wrote:
> >
> > So why do we add extra code who's sole effect is breaking old
> > windows drivers?
> 
> A little dramatic, no?
> 
> > I like the idea but why add code for status and mac features?
> 
> Because it's correct and therefore easier to understand when it's
> applied to the remaining virtio drivers (as Jesse is working on right now).;
> 
> > Also, would be nicer not to rely on the fact we always set MAC
> > flag at the moment. What if we make it optional?
> 
> I don't follow what you mean by this.  It's not relying on that fact AFAICT.
> 
> > We also don't really need the {} tag at the end - we have ARRAY_SIZE
> > macro.  Why don't we do:
> 
> Because the second step will be to pass this array instead of
> n->config_size in virtio_common_init.
> 
> > static VirtIOFeature feature_sizes[] = {
> >    { .flags = 0, /* default size when no flags are set */
> >     .end = offsetof(struct virtio_net_config, max_virtqueue_pairs)},
> 
> Now you've got two spots to touch whenever a new config flag is added.
> That's not very nice.
> 
> >    {.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.
> 
> The Windows driver is checking for BAR0 size == 32.  Bars are always a
> power of 2.  The base config registers are 20+ bytes so the BAR0 will
> always at least be 32 bytes.  IOW, your concerns about breaking the
> Windows drivers are unfounded because BAR0 size won't change.
> 
> Regards,
> 
> Anthony Liguori

Good points. This addresses my concerns, thanks.

> >
> >
> > -- 
> > MST