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

[Anthony Liguori]

"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

>
>
> -- 
> MST