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.
--
MST
