Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features

[Alexander Graf]

On 02/06/2013 12:47 AM, 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>

This patch breaks virtio-s390:

------------[ cut here ]------------
kernel BUG at 
/usr/src/packages/BUILD/kernel-default-3.0.58/linux-3.0/drivers/s390/kvm/kvm_virtio.c:121!
illegal operation: 0001 [#1] SMP
Modules linked in: virtio_net(F+) scsi_dh_emc(F) scsi_dh_hp_sw(F) 
scsi_dh_rdac(F) scsi_dh_alua(F) scsi_dh(F) scsi_mod(F) dm_snapshot(F) 
dm_mod(F) ext3(F) mbcache(F) jbd(F)
Supported: No, Unsupported modules are loaded
CPU: 0 Tainted: GF             3.0.58-52-default #1
Process modprobe (pid: 160, task: 0000000005c1a238, ksp: 0000000005c1feb8)
Krnl PSW : 0704200180000000 00000000004172ca (kvm_get+0x86/0x90)
           R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:0 CC:2 PM:0 EA:3
Krnl GPRS: 0000000000000000 0000000000000000 0000000000000006 
0000000000000000
           000000000108d710 0000000000000006 0000000007dbf198 
0000000007dbf000
           0000000000001000 0000000007dbf19e 00000000008499d0 
00000000013e6000
           0000000008000000 000003c0004e80e8 000003c0004e6be8 
0000000005c1fb78
Krnl Code: 00000000004172be: a737fff9        brctg    %r3,4172b0
           00000000004172c2: a7f4ffee        brc    15,41729e
           00000000004172c6: a7f40001        brc    15,4172c8
>00000000004172ca: a7f40000        brc    15,4172ca
           00000000004172ce: d20040001000    mvc    0(1,%r4),0(%r1)
           00000000004172d4: ebaff0680024    stmg    %r10,%r15,104(%r15)
           00000000004172da: c0d00009ca03    larl    %r13,5506e0
           00000000004172e0: a7f13fc0        tmll    %r15,16320
Call Trace:
([<000003c0004e6b54>] virtnet_probe+0x50/0x4f0 [virtio_net])
 [<00000000003a7df0>] virtio_dev_probe+0x16c/0x1c0
 [<00000000003c80ce>] really_probe+0x8a/0x25c
 [<00000000003c8416>] __driver_attach+0xca/0xd0
 [<00000000003c74cc>] bus_for_each_dev+0x80/0xd4
 [<00000000003c6b82>] bus_add_driver+0x22a/0x2f4
 [<00000000003c8c0e>] driver_register+0x9e/0x1a0
 [<00000000001000ba>] do_one_initcall+0x3a/0x170
 [<000000000019e292>] SyS_init_module+0xe6/0x234
 [<00000000004f9c40>] sysc_noemu+0x16/0x1c
 [<000003fffd4a1046>] 0x3fffd4a1046
Last Breaking-Event-Address:
 [<00000000004172c6>] kvm_get+0x82/0x90

---[ end trace 22332d4912971f08 ]---


> ---
>   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))
> +
> +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)},
> +    {}
> +};
> +
>   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);
> +        }
> +    }

Because down here, host_features doesn't include any features yet. They 
only get set later by calling get_config(0) ...

>       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;

... which only works after get_config got set.


Alex

>       n->vdev.set_config = virtio_net_set_config;
>       n->vdev.get_features = virtio_net_get_features;