From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCuvI-0001gf-KM for qemu-devel@nongnu.org; Tue, 05 Mar 2013 11:41:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UCuvH-00085o-5A for qemu-devel@nongnu.org; Tue, 05 Mar 2013 11:41:04 -0500 Received: from cantor2.suse.de ([195.135.220.15]:49972 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UCuvG-00085b-SF for qemu-devel@nongnu.org; Tue, 05 Mar 2013 11:41:03 -0500 Message-ID: <5136201C.8060608@gmail.com> Date: Tue, 05 Mar 2013 17:41:00 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1360108037-9211-1-git-send-email-jlarrew@linux.vnet.ibm.com> <1360108037-9211-3-git-send-email-jlarrew@linux.vnet.ibm.com> In-Reply-To: <1360108037-9211-3-git-send-email-jlarrew@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] hw/virtio-net.c: set config size using host features List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jesse Larrew Cc: Christian Borntraeger , aliguori@us.ibm.com, qemu-devel@nongnu.org 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 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;