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 05:56:16PM +0200, Michael S. Tsirkin wrote:
> 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.

Well there is some value. It will catch anyone trying to
hard-code BAR size checks in the driver in the future :)

> -- 
> MST