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 09:45:54AM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Tue, Feb 05, 2013 at 05:47:16PM -0600, Jesse Larrew wrote:
> >> Currently, the config size for virtio devices is hard coded. When a new
> >> feature is added that changes the config size, drivers that assume a static
> >> config size will break. For purposes of backward compatibility, there needs
> >> to be a way to inform drivers of the config size needed to accommodate the
> >> set of features enabled.
> >> 
> >> Signed-off-by: Jesse Larrew <address@hidden>
> >> ---
> >>  hw/virtio-net.c | 44 ++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 36 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index f1c2884..8f521b3 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -73,8 +73,31 @@ typedef struct VirtIONet
> >>      int multiqueue;
> >>      uint16_t max_queues;
> >>      uint16_t curr_queues;
> >> +    int config_size;
> >>  } VirtIONet;
> >>  
> >> +/*
> >> + * Calculate the number of bytes up to and including the given 'field' of
> >> + * 'container'.
> >> + */
> >> +#define endof(container, field) \
> >> +    ((intptr_t)(&(((container *)0)->field)+1))
> >
> > Hmm I'm worried whether it's legal to take a pointer to a
> > field in a packed structure.
> 
> It absolutely is.  Packed just means remove padding.  It doesn't imply
> that there would be any kind of weird layout.  Ultimately, a pointer to
> a member needs to behave the same way that a pointer to that type
> declared in an unpacked structure would behave since the knowledge of
> the fact that it's in a packed structure is quickly lost.
> 
> > How about we remove the packed attribute from config space structures?
> 
> It's definitely not needed here AFAICT.
> 
> Regards,
> 
> Anthony Liguori
> 
> > It's not really necessary since fields are laid out reasonably.
> >
> >
> >> +typedef struct VirtIOFeature {
> >> +    uint32_t flags;
> >> +    size_t end;
> >> +} VirtIOFeature;
> >> +
> >> +static VirtIOFeature feature_sizes[] = {
> >> +    {.flags = 1 << VIRTIO_NET_F_MAC,
> >> +     .end = endof(struct virtio_net_config, mac)},
> >> +    {.flags = 1 << VIRTIO_NET_F_STATUS,
> >> +     .end = endof(struct virtio_net_config, status)},
> >> +    {.flags = 1 << VIRTIO_NET_F_MQ,
> >> +     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> >> +    {}
> >> +};
> >> +
> >
> > This is not good. This will break old windows drivers
> > if mac/status are off, since they hardcode 32 BAR size.
> 
> Then old windows drivers are broken on older QEMU instances that never
> had those fields in the first place.
> 
> 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.

-- 
MST