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" <m...@redhat.com> 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 <jlar...@linux.vnet.ibm.com>
> > >> ---
> > >> 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
