On Thu, Feb 07, 2013 at 03:15:42PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <m...@redhat.com> 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
