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.
How about we remove the packed attribute from config space structures?
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.
> static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
> {
> VirtIONet *n = qemu_get_nic_opaque(nc);
> @@ -104,15 +127,15 @@ static void virtio_net_get_config(VirtIODevice *vdev,
> uint8_t *config)
> stw_p(&netcfg.status, n->status);
> stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
> memcpy(netcfg.mac, n->mac, ETH_ALEN);
> - memcpy(config, &netcfg, sizeof(netcfg));
> + memcpy(config, &netcfg, n->config_size);
> }
>
> static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> {
> VirtIONet *n = to_virtio_net(vdev);
> - struct virtio_net_config netcfg;
> + struct virtio_net_config netcfg = {};
>
> - memcpy(&netcfg, config, sizeof(netcfg));
> + memcpy(&netcfg, config, n->config_size);
>
> if (!(n->vdev.guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
> memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
> @@ -1279,16 +1302,21 @@ static void
> virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> }
>
> VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
> - virtio_net_conf *net,
> - uint32_t host_features)
> + virtio_net_conf *net, uint32_t host_features)
> {
> VirtIONet *n;
> - int i;
> + int i, config_size = 0;
> +
> + for (i = 0; feature_sizes[i].flags != 0; i++) {
> + if (host_features & feature_sizes[i].flags) {
> + config_size = MAX(feature_sizes[i].end, config_size);
> + }
> + }
>
> n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> - sizeof(struct virtio_net_config),
> - sizeof(VirtIONet));
> + config_size, sizeof(VirtIONet));
>
> + n->config_size = config_size;
> n->vdev.get_config = virtio_net_get_config;
> n->vdev.set_config = virtio_net_set_config;
> n->vdev.get_features = virtio_net_get_features;
> --
> 1.7.11.7
>
