* [Qemu-devel] [PATCH RFC 01/11] linux-headers/virtio_config: Update with VIRTIO_F_VERSION_1
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
@ 2014-10-07 14:39 ` Cornelia Huck
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 02/11] virtio: cull virtio_bus_set_vdev_features Cornelia Huck
` (11 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:39 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
From: Thomas Huth <thuth@linux.vnet.ibm.com>
Add the new VIRTIO_F_VERSION_1 definition to the virtio_config.h
linux header.
Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
| 3 +++
1 file changed, 3 insertions(+)
--git a/linux-headers/linux/virtio_config.h b/linux-headers/linux/virtio_config.h
index 75dc20b..16aa289 100644
--- a/linux-headers/linux/virtio_config.h
+++ b/linux-headers/linux/virtio_config.h
@@ -54,4 +54,7 @@
/* Can the device handle any descriptor layout? */
#define VIRTIO_F_ANY_LAYOUT 27
+/* v1.0 compliant. */
+#define VIRTIO_F_VERSION_1 32
+
#endif /* _LINUX_VIRTIO_CONFIG_H */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 02/11] virtio: cull virtio_bus_set_vdev_features
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 01/11] linux-headers/virtio_config: Update with VIRTIO_F_VERSION_1 Cornelia Huck
@ 2014-10-07 14:39 ` Cornelia Huck
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits Cornelia Huck
` (10 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:39 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
The only user of this function was virtio-ccw, and it should use
virtio_set_features() like everybody else: We need to make sure
that bad features are masked out properly, which this function did
not do.
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/virtio-ccw.c | 3 +--
hw/virtio/virtio-bus.c | 14 --------------
include/hw/virtio/virtio-bus.h | 3 ---
3 files changed, 1 insertion(+), 19 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index e7d3ea1..7833dd2 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -400,8 +400,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
features.features = ldl_le_phys(&address_space_memory, ccw.cda);
if (features.index < ARRAY_SIZE(dev->host_features)) {
- virtio_bus_set_vdev_features(&dev->bus, features.features);
- vdev->guest_features = features.features;
+ virtio_set_features(vdev, features.features);
} else {
/*
* If the guest supports more feature bits, assert that it
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index eb77019..a8ffa07 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -109,20 +109,6 @@ uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
return k->get_features(vdev, requested_features);
}
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
- uint32_t requested_features)
-{
- VirtIODevice *vdev = virtio_bus_get_device(bus);
- VirtioDeviceClass *k;
-
- assert(vdev != NULL);
- k = VIRTIO_DEVICE_GET_CLASS(vdev);
- if (k->set_features != NULL) {
- k->set_features(vdev, requested_features);
- }
-}
-
/* Get bad features of the plugged device. */
uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
{
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 0756545..0d2e7b4 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -84,9 +84,6 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState *bus);
/* Get the features of the plugged device. */
uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
uint32_t requested_features);
-/* Set the features of the plugged device. */
-void virtio_bus_set_vdev_features(VirtioBusState *bus,
- uint32_t requested_features);
/* Get bad features of the plugged device. */
uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus);
/* Get config of the plugged device. */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 01/11] linux-headers/virtio_config: Update with VIRTIO_F_VERSION_1 Cornelia Huck
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 02/11] virtio: cull virtio_bus_set_vdev_features Cornelia Huck
@ 2014-10-07 14:39 ` Cornelia Huck
2014-10-13 5:53 ` Rusty Russell
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 04/11] s390x/virtio-ccw: fix check for WRITE_FEAT Cornelia Huck
` (9 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:39 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
With virtio-1, we support more than 32 feature bits. Let's make
vdev->guest_features depend on the number of supported feature bits,
allowing us to grow the feature bits automatically.
We also need to enhance the internal functions dealing with getting
and setting features with an additional index field, so that all feature
bits may be accessed (in chunks of 32 bits).
vhost and migration have been ignored for now.
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/9pfs/virtio-9p-device.c | 7 ++++++-
hw/block/virtio-blk.c | 9 +++++++--
hw/char/virtio-serial-bus.c | 9 +++++++--
hw/net/virtio-net.c | 38 ++++++++++++++++++++++++++------------
hw/s390x/s390-virtio-bus.c | 9 +++++----
hw/s390x/virtio-ccw.c | 17 ++++++++++-------
hw/scsi/vhost-scsi.c | 7 +++++--
hw/scsi/virtio-scsi.c | 8 ++++----
hw/virtio/dataplane/vring.c | 10 +++++-----
hw/virtio/virtio-balloon.c | 8 ++++++--
hw/virtio/virtio-bus.c | 9 +++++----
hw/virtio/virtio-mmio.c | 9 +++++----
hw/virtio/virtio-pci.c | 13 +++++++------
hw/virtio/virtio-rng.c | 2 +-
hw/virtio/virtio.c | 29 +++++++++++++++++------------
include/hw/virtio/virtio-bus.h | 7 ++++---
include/hw/virtio/virtio.h | 19 ++++++++++++++-----
17 files changed, 134 insertions(+), 76 deletions(-)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 2572747..c29c8c8 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -21,8 +21,13 @@
#include "virtio-9p-coth.h"
#include "hw/virtio/virtio-access.h"
-static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
{
+ if (index > 0) {
+ return features;
+ }
+
features |= 1 << VIRTIO_9P_MOUNT_TAG;
return features;
}
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 45e0c8f..5abc327 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -561,10 +561,15 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
aio_context_release(bdrv_get_aio_context(s->bs));
}
-static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t virtio_blk_get_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
{
VirtIOBlock *s = VIRTIO_BLK(vdev);
+ if (index > 0) {
+ return features;
+ }
+
features |= (1 << VIRTIO_BLK_F_SEG_MAX);
features |= (1 << VIRTIO_BLK_F_GEOMETRY);
features |= (1 << VIRTIO_BLK_F_TOPOLOGY);
@@ -597,7 +602,7 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
return;
}
- features = vdev->guest_features;
+ features = vdev->guest_features[0];
/* A guest that supports VIRTIO_BLK_F_CONFIG_WCE must be able to send
* cache flushes. Thus, the "auto writethrough" behavior is never
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 3931085..0d843fe 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -75,7 +75,7 @@ static VirtIOSerialPort *find_port_by_name(char *name)
static bool use_multiport(VirtIOSerial *vser)
{
VirtIODevice *vdev = VIRTIO_DEVICE(vser);
- return vdev->guest_features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+ return vdev->guest_features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
static size_t write_to_port(VirtIOSerialPort *port,
@@ -467,10 +467,15 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
{
}
-static uint32_t get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
{
VirtIOSerial *vser;
+ if (index > 0) {
+ return features;
+ }
+
vser = VIRTIO_SERIAL(vdev);
if (vser->bus.max_nr_ports > 1) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2040eac..67f91c0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -86,7 +86,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
memcpy(&netcfg, config, n->config_size);
- if (!(vdev->guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
+ if (!(vdev->guest_features[0] >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
memcpy(n->mac, netcfg.mac, ETH_ALEN);
qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
@@ -305,7 +305,7 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
info->multicast_table = str_list;
info->vlan_table = get_vlan_table(n);
- if (!((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features)) {
+ if (!((1 << VIRTIO_NET_F_CTRL_VLAN) & vdev->guest_features[0])) {
info->vlan = RX_STATE_ALL;
} else if (!info->vlan_table) {
info->vlan = RX_STATE_NONE;
@@ -441,11 +441,16 @@ static void virtio_net_set_queues(VirtIONet *n)
static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
-static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
+static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
{
VirtIONet *n = VIRTIO_NET(vdev);
NetClientState *nc = qemu_get_queue(n->nic);
+ if (index > 0) {
+ return features;
+ }
+
features |= (1 << VIRTIO_NET_F_MAC);
if (!peer_has_vnet_hdr(n)) {
@@ -471,10 +476,14 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
return vhost_net_get_features(get_vhost_net(nc->peer), features);
}
-static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
+static uint32_t virtio_net_bad_features(VirtIODevice *vdev, unsigned int index)
{
uint32_t features = 0;
+ if (index > 0) {
+ return 0;
+ }
+
/* Linux kernel 2.6.25. It understood MAC (as everyone must),
* but also these: */
features |= (1 << VIRTIO_NET_F_MAC);
@@ -511,14 +520,19 @@ static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
static inline uint64_t virtio_net_supported_guest_offloads(VirtIONet *n)
{
VirtIODevice *vdev = VIRTIO_DEVICE(n);
- return virtio_net_guest_offloads_by_features(vdev->guest_features);
+ return virtio_net_guest_offloads_by_features(vdev->guest_features[0]);
}
-static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
+static void virtio_net_set_features(VirtIODevice *vdev, unsigned int index,
+ uint32_t features)
{
VirtIONet *n = VIRTIO_NET(vdev);
int i;
+ if (index > 0) {
+ return;
+ }
+
virtio_net_set_multiqueue(n, !!(features & (1 << VIRTIO_NET_F_MQ)));
virtio_net_set_mrg_rx_bufs(n, !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF)));
@@ -585,7 +599,7 @@ static int virtio_net_handle_offloads(VirtIONet *n, uint8_t cmd,
uint64_t offloads;
size_t s;
- if (!((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features)) {
+ if (!((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features[0])) {
return VIRTIO_NET_ERR;
}
@@ -1034,7 +1048,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
"i %zd mergeable %d offset %zd, size %zd, "
"guest hdr len %zd, host hdr len %zd guest features 0x%x",
i, n->mergeable_rx_bufs, offset, size,
- n->guest_hdr_len, n->host_hdr_len, vdev->guest_features);
+ n->guest_hdr_len, n->host_hdr_len, vdev->guest_features[0]);
exit(1);
}
@@ -1377,7 +1391,7 @@ static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f)
}
}
- if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features) {
+ if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features[0]) {
qemu_put_be64(f, n->curr_guest_offloads);
}
}
@@ -1485,7 +1499,7 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
}
}
- if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features) {
+ if ((1 << VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) & vdev->guest_features[0]) {
n->curr_guest_offloads = qemu_get_be64(f);
} else {
n->curr_guest_offloads = virtio_net_supported_guest_offloads(n);
@@ -1512,8 +1526,8 @@ static int virtio_net_load_device(VirtIODevice *vdev, QEMUFile *f,
qemu_get_subqueue(n->nic, i)->link_down = link_down;
}
- if (vdev->guest_features & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE) &&
- vdev->guest_features & (0x1 << VIRTIO_NET_F_CTRL_VQ)) {
+ if (vdev->guest_features[0] & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE) &&
+ vdev->guest_features[0] & (0x1 << VIRTIO_NET_F_CTRL_VQ)) {
n->announce_counter = SELF_ANNOUNCE_ROUNDS;
timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL));
}
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index f451ca1..1ddf133 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -128,7 +128,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
bus->dev_offs += dev_len;
- dev->host_features = virtio_bus_get_vdev_features(&dev->bus,
+ dev->host_features = virtio_bus_get_vdev_features(&dev->bus, 0,
dev->host_features);
s390_virtio_device_sync(dev);
s390_virtio_reset_idx(dev);
@@ -417,7 +417,7 @@ void s390_virtio_device_update_status(VirtIOS390Device *dev)
/* Update guest supported feature bitmap */
features = bswap32(ldl_be_phys(&address_space_memory, dev->feat_offs));
- virtio_set_features(vdev, features);
+ virtio_set_features(vdev, 0, features);
}
VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
@@ -488,10 +488,11 @@ static void virtio_s390_notify(DeviceState *d, uint16_t vector)
s390_virtio_irq(0, token);
}
-static unsigned virtio_s390_get_features(DeviceState *d)
+static unsigned virtio_s390_get_features(DeviceState *d, unsigned int index)
{
VirtIOS390Device *dev = to_virtio_s390_device(d);
- return dev->host_features;
+
+ return index == 0 ? dev->host_features : 0;
}
/**************** S390 Virtio Bus Device Descriptions *******************/
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 7833dd2..8aa79a7 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -400,7 +400,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
features.features = ldl_le_phys(&address_space_memory, ccw.cda);
if (features.index < ARRAY_SIZE(dev->host_features)) {
- virtio_set_features(vdev, features.features);
+ virtio_set_features(vdev, features.index, features.features);
} else {
/*
* If the guest supports more feature bits, assert that it
@@ -619,6 +619,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
int ret;
int num;
DeviceState *parent = DEVICE(dev);
+ int i;
sch = g_malloc0(sizeof(SubchDev));
@@ -739,9 +740,11 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
sch->id.cu_model = vdev->device_id;
- /* Only the first 32 feature bits are used. */
- dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
- dev->host_features[0]);
+ /* Set default feature bits that are offered by the host. */
+ for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
+ dev->host_features[i] =
+ virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
+ }
dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
@@ -1059,12 +1062,12 @@ static void virtio_ccw_notify(DeviceState *d, uint16_t vector)
}
}
-static unsigned virtio_ccw_get_features(DeviceState *d)
+static unsigned virtio_ccw_get_features(DeviceState *d, unsigned int index)
{
VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
- /* Only the first 32 feature bits are used. */
- return dev->host_features[0];
+ return index < ARRAY_SIZE(dev->host_features) ?
+ dev->host_features[index] : 0;
}
static void virtio_ccw_reset(DeviceState *d)
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 308b393..8e1afa0 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -92,7 +92,7 @@ static int vhost_scsi_start(VHostSCSI *s)
return ret;
}
- s->dev.acked_features = vdev->guest_features;
+ s->dev.acked_features = vdev->guest_features[0];
ret = vhost_dev_start(&s->dev, vdev);
if (ret < 0) {
error_report("Error start vhost dev");
@@ -150,11 +150,14 @@ static void vhost_scsi_stop(VHostSCSI *s)
vhost_dev_disable_notifiers(&s->dev, vdev);
}
-static uint32_t vhost_scsi_get_features(VirtIODevice *vdev,
+static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
uint32_t features)
{
VHostSCSI *s = VHOST_SCSI(vdev);
+ if (index > 0) {
+ return features;
+ }
return vhost_get_features(&s->dev, kernel_feature_bits, features);
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 203e624..088e688 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -608,7 +608,7 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
vs->cdb_size = virtio_ldl_p(vdev, &scsiconf->cdb_size);
}
-static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
+static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
uint32_t requested_features)
{
return requested_features;
@@ -729,7 +729,7 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
VirtIODevice *vdev = VIRTIO_DEVICE(s);
- if (((vdev->guest_features >> VIRTIO_SCSI_F_CHANGE) & 1) &&
+ if (((vdev->guest_features[0] >> VIRTIO_SCSI_F_CHANGE) & 1) &&
dev->type != TYPE_ROM) {
virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE,
sense.asc | (sense.ascq << 8));
@@ -741,7 +741,7 @@ static void virtio_scsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
VirtIODevice *vdev = VIRTIO_DEVICE(s);
- if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
+ if ((vdev->guest_features[0] >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
VIRTIO_SCSI_EVT_RESET_RESCAN);
}
@@ -752,7 +752,7 @@ static void virtio_scsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
VirtIODevice *vdev = VIRTIO_DEVICE(s);
- if ((vdev->guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
+ if ((vdev->guest_features[0] >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
VIRTIO_SCSI_EVT_RESET_REMOVED);
}
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 372706a..b84957f 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -103,7 +103,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
/* Disable guest->host notifies */
void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
{
- if (!(vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX))) {
+ if (!(vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX))) {
vring->vr.used->flags |= VRING_USED_F_NO_NOTIFY;
}
}
@@ -114,7 +114,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
*/
bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
{
- if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+ if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
vring_avail_event(&vring->vr) = vring->vr.avail->idx;
} else {
vring->vr.used->flags &= ~VRING_USED_F_NO_NOTIFY;
@@ -133,12 +133,12 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
* interrupts. */
smp_mb();
- if ((vdev->guest_features & VIRTIO_F_NOTIFY_ON_EMPTY) &&
+ if ((vdev->guest_features[0] & VIRTIO_F_NOTIFY_ON_EMPTY) &&
unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
return true;
}
- if (!(vdev->guest_features & VIRTIO_RING_F_EVENT_IDX)) {
+ if (!(vdev->guest_features[0] & VIRTIO_RING_F_EVENT_IDX)) {
return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
}
old = vring->signalled_used;
@@ -352,7 +352,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
goto out;
}
- if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+ if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
vring_avail_event(&vring->vr) = vring->vr.avail->idx;
}
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index b5cf7ca..5af17e2 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -69,7 +69,7 @@ static inline void reset_stats(VirtIOBalloon *dev)
static bool balloon_stats_supported(const VirtIOBalloon *s)
{
VirtIODevice *vdev = VIRTIO_DEVICE(s);
- return vdev->guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ);
+ return vdev->guest_features[0] & (1 << VIRTIO_BALLOON_F_STATS_VQ);
}
static bool balloon_stats_enabled(const VirtIOBalloon *s)
@@ -303,8 +303,12 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
}
}
-static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
+static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
+ unsigned int index, uint32_t f)
{
+ if (index > 0) {
+ return f;
+ }
f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
return f;
}
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index a8ffa07..503114d 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -97,7 +97,7 @@ size_t virtio_bus_get_vdev_config_len(VirtioBusState *bus)
}
/* Get the features of the plugged device. */
-uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
+uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus, unsigned int index,
uint32_t requested_features)
{
VirtIODevice *vdev = virtio_bus_get_device(bus);
@@ -106,11 +106,12 @@ uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
assert(vdev != NULL);
k = VIRTIO_DEVICE_GET_CLASS(vdev);
assert(k->get_features != NULL);
- return k->get_features(vdev, requested_features);
+ return k->get_features(vdev, index, requested_features);
}
/* Get bad features of the plugged device. */
-uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
+uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus,
+ unsigned int index)
{
VirtIODevice *vdev = virtio_bus_get_device(bus);
VirtioDeviceClass *k;
@@ -118,7 +119,7 @@ uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus)
assert(vdev != NULL);
k = VIRTIO_DEVICE_GET_CLASS(vdev);
if (k->bad_features != NULL) {
- return k->bad_features(vdev);
+ return k->bad_features(vdev, index);
} else {
return 0;
}
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 18c6e5b..b4282c4 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -225,7 +225,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
break;
case VIRTIO_MMIO_GUESTFEATURES:
if (!proxy->guest_features_sel) {
- virtio_set_features(vdev, value);
+ virtio_set_features(vdev, 0, value);
}
break;
case VIRTIO_MMIO_GUESTFEATURESSEL:
@@ -309,11 +309,12 @@ static void virtio_mmio_update_irq(DeviceState *opaque, uint16_t vector)
qemu_set_irq(proxy->irq, level);
}
-static unsigned int virtio_mmio_get_features(DeviceState *opaque)
+static unsigned int virtio_mmio_get_features(DeviceState *opaque,
+ unsigned int index)
{
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
- return proxy->host_features;
+ return index == 0 ? proxy->host_features : 0;
}
static int virtio_mmio_load_config(DeviceState *opaque, QEMUFile *f)
@@ -353,7 +354,7 @@ static void virtio_mmio_device_plugged(DeviceState *opaque)
VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
proxy->host_features |= (0x1 << VIRTIO_F_NOTIFY_ON_EMPTY);
- proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus,
+ proxy->host_features = virtio_bus_get_vdev_features(&proxy->bus, 0,
proxy->host_features);
}
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 390f824..b22fc64 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -275,9 +275,9 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
case VIRTIO_PCI_GUEST_FEATURES:
/* Guest does not negotiate properly? We have to assume nothing. */
if (val & (1 << VIRTIO_F_BAD_FEATURE)) {
- val = virtio_bus_get_vdev_bad_features(&proxy->bus);
+ val = virtio_bus_get_vdev_bad_features(&proxy->bus, 0);
}
- virtio_set_features(vdev, val);
+ virtio_set_features(vdev, 0, val);
break;
case VIRTIO_PCI_QUEUE_PFN:
pa = (hwaddr)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
@@ -364,7 +364,7 @@ static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
ret = proxy->host_features;
break;
case VIRTIO_PCI_GUEST_FEATURES:
- ret = vdev->guest_features;
+ ret = vdev->guest_features[0];
break;
case VIRTIO_PCI_QUEUE_PFN:
ret = virtio_queue_get_addr(vdev, vdev->queue_sel)
@@ -490,10 +490,11 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
}
}
-static unsigned virtio_pci_get_features(DeviceState *d)
+static unsigned virtio_pci_get_features(DeviceState *d, unsigned int index)
{
VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
- return proxy->host_features;
+
+ return index == 0 ? proxy->host_features : 0;
}
static int kvm_virtio_pci_vq_vector_use(VirtIOPCIProxy *proxy,
@@ -1006,7 +1007,7 @@ static void virtio_pci_device_plugged(DeviceState *d)
proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
- proxy->host_features = virtio_bus_get_vdev_features(bus,
+ proxy->host_features = virtio_bus_get_vdev_features(bus, 0,
proxy->host_features);
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index e85a979..2814017 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -99,7 +99,7 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
virtio_rng_process(vrng);
}
-static uint32_t get_features(VirtIODevice *vdev, uint32_t f)
+static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
{
return f;
}
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2c236bf..7aaa953 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -217,7 +217,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
void virtio_queue_set_notification(VirtQueue *vq, int enable)
{
vq->notification = enable;
- if (vq->vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+ if (vq->vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
vring_avail_event(vq, vring_avail_idx(vq));
} else if (enable) {
vring_used_flags_unset_bit(vq, VRING_USED_F_NO_NOTIFY);
@@ -468,7 +468,7 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
max = vq->vring.num;
i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
- if (vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX)) {
+ if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
vring_avail_event(vq, vring_avail_idx(vq));
}
@@ -592,7 +592,10 @@ void virtio_reset(void *opaque)
k->reset(vdev);
}
- vdev->guest_features = 0;
+ for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
+ vdev->guest_features[i] = 0;
+ }
+
vdev->queue_sel = 0;
vdev->status = 0;
vdev->isr = 0;
@@ -839,12 +842,12 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
/* We need to expose used array entries before checking used event. */
smp_mb();
/* Always notify when queue is empty (when feature acknowledge) */
- if (((vdev->guest_features & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) &&
+ if (((vdev->guest_features[0] & (1 << VIRTIO_F_NOTIFY_ON_EMPTY)) &&
!vq->inuse && vring_avail_idx(vq) == vq->last_avail_idx)) {
return true;
}
- if (!(vdev->guest_features & (1 << VIRTIO_RING_F_EVENT_IDX))) {
+ if (!(vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX))) {
return !(vring_avail_flags(vq) & VRING_AVAIL_F_NO_INTERRUPT);
}
@@ -924,7 +927,8 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
qemu_put_8s(f, &vdev->status);
qemu_put_8s(f, &vdev->isr);
qemu_put_be16s(f, &vdev->queue_sel);
- qemu_put_be32s(f, &vdev->guest_features);
+ /* XXX features >= 32 */
+ qemu_put_be32s(f, &vdev->guest_features[0]);
qemu_put_be32(f, vdev->config_len);
qemu_put_buffer(f, vdev->config, vdev->config_len);
@@ -958,19 +962,19 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
vmstate_save_state(f, &vmstate_virtio, vdev);
}
-int virtio_set_features(VirtIODevice *vdev, uint32_t val)
+int virtio_set_features(VirtIODevice *vdev, unsigned int index, uint32_t val)
{
BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
VirtioBusClass *vbusk = VIRTIO_BUS_GET_CLASS(qbus);
VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
- uint32_t supported_features = vbusk->get_features(qbus->parent);
+ uint32_t supported_features = vbusk->get_features(qbus->parent, index);
bool bad = (val & ~supported_features) != 0;
val &= supported_features;
if (k->set_features) {
- k->set_features(vdev, val);
+ k->set_features(vdev, index, val);
}
- vdev->guest_features = val;
+ vdev->guest_features[index] = val;
return bad ? -1 : 0;
}
@@ -1005,8 +1009,9 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
}
qemu_get_be32s(f, &features);
- if (virtio_set_features(vdev, features) < 0) {
- supported_features = k->get_features(qbus->parent);
+ /* XXX features >= 32 */
+ if (virtio_set_features(vdev, 0, features) < 0) {
+ supported_features = k->get_features(qbus->parent, 0);
error_report("Features 0x%x unsupported. Allowed features: 0x%x",
features, supported_features);
return -1;
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 0d2e7b4..03f4432 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -47,7 +47,7 @@ typedef struct VirtioBusClass {
int (*load_config)(DeviceState *d, QEMUFile *f);
int (*load_queue)(DeviceState *d, int n, QEMUFile *f);
int (*load_done)(DeviceState *d, QEMUFile *f);
- unsigned (*get_features)(DeviceState *d);
+ unsigned (*get_features)(DeviceState *d, unsigned int index);
bool (*query_guest_notifiers)(DeviceState *d);
int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
int (*set_host_notifier)(DeviceState *d, int n, bool assigned);
@@ -82,10 +82,11 @@ uint16_t virtio_bus_get_vdev_id(VirtioBusState *bus);
/* Get the config_len field of the plugged device. */
size_t virtio_bus_get_vdev_config_len(VirtioBusState *bus);
/* Get the features of the plugged device. */
-uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus,
+uint32_t virtio_bus_get_vdev_features(VirtioBusState *bus, unsigned int index,
uint32_t requested_features);
/* Get bad features of the plugged device. */
-uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus);
+uint32_t virtio_bus_get_vdev_bad_features(VirtioBusState *bus,
+ unsigned int index);
/* Get config of the plugged device. */
void virtio_bus_get_vdev_config(VirtioBusState *bus, uint8_t *config);
/* Set config of the plugged device. */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 0726d76..b408166 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -55,6 +55,12 @@
/* A guest should never accept this. It implies negotiation is broken. */
#define VIRTIO_F_BAD_FEATURE 30
+/* v1.0 compliant. */
+#define VIRTIO_F_VERSION_1 32
+
+/* The highest feature bit that we support */
+#define VIRTIO_HIGHEST_FEATURE_BIT 32
+
/* from Linux's linux/virtio_ring.h */
/* This marks a buffer as continuing via the next field. */
@@ -110,6 +116,8 @@ enum virtio_device_endian {
VIRTIO_DEVICE_ENDIAN_BIG,
};
+#define NR_VIRTIO_FEATURE_WORDS ((VIRTIO_HIGHEST_FEATURE_BIT + 32) / 32)
+
struct VirtIODevice
{
DeviceState parent_obj;
@@ -117,7 +125,7 @@ struct VirtIODevice
uint8_t status;
uint8_t isr;
uint16_t queue_sel;
- uint32_t guest_features;
+ uint32_t guest_features[NR_VIRTIO_FEATURE_WORDS];
size_t config_len;
void *config;
uint16_t config_vector;
@@ -138,9 +146,10 @@ typedef struct VirtioDeviceClass {
/* This is what a VirtioDevice must implement */
DeviceRealize realize;
DeviceUnrealize unrealize;
- uint32_t (*get_features)(VirtIODevice *vdev, uint32_t requested_features);
- uint32_t (*bad_features)(VirtIODevice *vdev);
- void (*set_features)(VirtIODevice *vdev, uint32_t val);
+ uint32_t (*get_features)(VirtIODevice *vdev, unsigned int index,
+ uint32_t requested_features);
+ uint32_t (*bad_features)(VirtIODevice *vdev, unsigned int index);
+ void (*set_features)(VirtIODevice *vdev, unsigned int index, uint32_t val);
void (*get_config)(VirtIODevice *vdev, uint8_t *config);
void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
void (*reset)(VirtIODevice *vdev);
@@ -225,7 +234,7 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
void virtio_set_status(VirtIODevice *vdev, uint8_t val);
void virtio_reset(void *opaque);
void virtio_update_irq(VirtIODevice *vdev);
-int virtio_set_features(VirtIODevice *vdev, uint32_t val);
+int virtio_set_features(VirtIODevice *vdev, unsigned int index, uint32_t val);
/* Base devices. */
typedef struct VirtIOBlkConf VirtIOBlkConf;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits Cornelia Huck
@ 2014-10-13 5:53 ` Rusty Russell
2014-10-13 10:55 ` Cornelia Huck
0 siblings, 1 reply; 36+ messages in thread
From: Rusty Russell @ 2014-10-13 5:53 UTC (permalink / raw)
To: Cornelia Huck, virtualization, qemu-devel, kvm; +Cc: thuth
Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> With virtio-1, we support more than 32 feature bits. Let's make
> vdev->guest_features depend on the number of supported feature bits,
> allowing us to grow the feature bits automatically.
It's a judgement call, but I would say that simply using uint64_t
will be sufficient for quite a while.
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits
2014-10-13 5:53 ` Rusty Russell
@ 2014-10-13 10:55 ` Cornelia Huck
0 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-13 10:55 UTC (permalink / raw)
To: Rusty Russell; +Cc: thuth, qemu-devel, kvm, virtualization
On Mon, 13 Oct 2014 16:23:58 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:
> Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > With virtio-1, we support more than 32 feature bits. Let's make
> > vdev->guest_features depend on the number of supported feature bits,
> > allowing us to grow the feature bits automatically.
>
> It's a judgement call, but I would say that simply using uint64_t
> will be sufficient for quite a while.
I prefer this as an array for two reasons:
- It matches what ccw does anyway (we read/write features in chunks of
32 bit), so this makes the backend handling for this transport very
straightforward.
- While I don't see us running out of 64 feature bits for a while, we'd
have to touch every device and transport again when we do. I'd prefer
to do this once, as we need to change code anyway.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH RFC 04/11] s390x/virtio-ccw: fix check for WRITE_FEAT
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (2 preceding siblings ...)
2014-10-07 14:39 ` [Qemu-devel] [PATCH RFC 03/11] virtio: support more feature bits Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices Cornelia Huck
` (8 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
We need to check guest feature size, not host feature size to find
out whether we should call virtio_set_features(). This check is
possible now that vdev->guest_features is an array.
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8aa79a7..69add47 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -399,7 +399,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
features.index = ldub_phys(&address_space_memory,
ccw.cda + sizeof(features.features));
features.features = ldl_le_phys(&address_space_memory, ccw.cda);
- if (features.index < ARRAY_SIZE(dev->host_features)) {
+ if (features.index < ARRAY_SIZE(vdev->guest_features)) {
virtio_set_features(vdev, features.index, features.features);
} else {
/*
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (3 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 04/11] s390x/virtio-ccw: fix check for WRITE_FEAT Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-28 15:40 ` Greg Kurz
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 06/11] virtio: allow virtio-1 queue layout Cornelia Huck
` (7 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
Introduce a helper function to indicate whether a virtio device is
operating in legacy or virtio standard mode.
It may be used to make decisions about the endianess of virtio accesses
and other virtio-1 specific changes, enabling us to support transitional
devices.
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/virtio/virtio.c | 6 +++++-
include/hw/virtio/virtio-access.h | 4 ++++
include/hw/virtio/virtio.h | 13 +++++++++++--
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7aaa953..e6ae3a0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
VirtIODevice *vdev = opaque;
assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
- return vdev->device_endian != virtio_default_endian();
+ if (virtio_device_is_legacy(vdev)) {
+ return vdev->device_endian != virtio_default_endian();
+ }
+ /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+ return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
}
static const VMStateDescription vmstate_virtio_device_endian = {
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
index 46456fd..c123ee0 100644
--- a/include/hw/virtio/virtio-access.h
+++ b/include/hw/virtio/virtio-access.h
@@ -19,6 +19,10 @@
static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
{
+ if (!virtio_device_is_legacy(vdev)) {
+ /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+ return false;
+ }
#if defined(TARGET_IS_BIENDIAN)
return virtio_is_big_endian(vdev);
#elif defined(TARGET_WORDS_BIGENDIAN)
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b408166..40e567c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -275,9 +275,18 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
void virtio_queue_notify_vq(VirtQueue *vq);
void virtio_irq(VirtQueue *vq);
+static inline bool virtio_device_is_legacy(VirtIODevice *vdev)
+{
+ return !(vdev->guest_features[1] & (1 << (VIRTIO_F_VERSION_1 - 32)));
+}
+
static inline bool virtio_is_big_endian(VirtIODevice *vdev)
{
- assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
- return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+ if (virtio_device_is_legacy(vdev)) {
+ assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+ return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+ }
+ /* Devices conforming to VIRTIO 1.0 or later are always LE. */
+ return false;
}
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices Cornelia Huck
@ 2014-10-28 15:40 ` Greg Kurz
2014-10-30 18:02 ` Cornelia Huck
0 siblings, 1 reply; 36+ messages in thread
From: Greg Kurz @ 2014-10-28 15:40 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Tue, 7 Oct 2014 16:40:01 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> Introduce a helper function to indicate whether a virtio device is
> operating in legacy or virtio standard mode.
>
> It may be used to make decisions about the endianess of virtio accesses
> and other virtio-1 specific changes, enabling us to support transitional
> devices.
>
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> hw/virtio/virtio.c | 6 +++++-
> include/hw/virtio/virtio-access.h | 4 ++++
> include/hw/virtio/virtio.h | 13 +++++++++++--
> 3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 7aaa953..e6ae3a0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
> VirtIODevice *vdev = opaque;
>
> assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> - return vdev->device_endian != virtio_default_endian();
> + if (virtio_device_is_legacy(vdev)) {
> + return vdev->device_endian != virtio_default_endian();
> + }
> + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> + return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
> }
>
Shouldn't we have some code doing the following somewhere ?
if (!virtio_device_is_legacy(vdev)) {
vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
}
also, since virtio-1 is LE only, do we expect device_endian to
be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?
Cheers.
--
Greg
> static const VMStateDescription vmstate_virtio_device_endian = {
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> index 46456fd..c123ee0 100644
> --- a/include/hw/virtio/virtio-access.h
> +++ b/include/hw/virtio/virtio-access.h
> @@ -19,6 +19,10 @@
>
> static inline bool virtio_access_is_big_endian(VirtIODevice *vdev)
> {
> + if (!virtio_device_is_legacy(vdev)) {
> + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> + return false;
> + }
> #if defined(TARGET_IS_BIENDIAN)
> return virtio_is_big_endian(vdev);
> #elif defined(TARGET_WORDS_BIGENDIAN)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index b408166..40e567c 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -275,9 +275,18 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> void virtio_queue_notify_vq(VirtQueue *vq);
> void virtio_irq(VirtQueue *vq);
>
> +static inline bool virtio_device_is_legacy(VirtIODevice *vdev)
> +{
> + return !(vdev->guest_features[1] & (1 << (VIRTIO_F_VERSION_1 - 32)));
> +}
> +
> static inline bool virtio_is_big_endian(VirtIODevice *vdev)
> {
> - assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> - return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> + if (virtio_device_is_legacy(vdev)) {
> + assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> + return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> + }
> + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> + return false;
> }
> #endif
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices
2014-10-28 15:40 ` Greg Kurz
@ 2014-10-30 18:02 ` Cornelia Huck
2014-10-30 22:29 ` Greg Kurz
0 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-30 18:02 UTC (permalink / raw)
To: Greg Kurz; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Tue, 28 Oct 2014 16:40:18 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Tue, 7 Oct 2014 16:40:01 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > Introduce a helper function to indicate whether a virtio device is
> > operating in legacy or virtio standard mode.
> >
> > It may be used to make decisions about the endianess of virtio accesses
> > and other virtio-1 specific changes, enabling us to support transitional
> > devices.
> >
> > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > hw/virtio/virtio.c | 6 +++++-
> > include/hw/virtio/virtio-access.h | 4 ++++
> > include/hw/virtio/virtio.h | 13 +++++++++++--
> > 3 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 7aaa953..e6ae3a0 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
> > VirtIODevice *vdev = opaque;
> >
> > assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> > - return vdev->device_endian != virtio_default_endian();
> > + if (virtio_device_is_legacy(vdev)) {
> > + return vdev->device_endian != virtio_default_endian();
> > + }
> > + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> > + return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
> > }
> >
>
> Shouldn't we have some code doing the following somewhere ?
>
> if (!virtio_device_is_legacy(vdev)) {
> vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> }
>
> also, since virtio-1 is LE only, do we expect device_endian to
> be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?
device_endian should not depend on whether the device is legacy or not.
virtio_is_big_endian always returns false for virtio-1 devices, though.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices
2014-10-30 18:02 ` Cornelia Huck
@ 2014-10-30 22:29 ` Greg Kurz
2014-11-03 11:44 ` Cornelia Huck
0 siblings, 1 reply; 36+ messages in thread
From: Greg Kurz @ 2014-10-30 22:29 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Thu, 30 Oct 2014 19:02:01 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 28 Oct 2014 16:40:18 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>
> > On Tue, 7 Oct 2014 16:40:01 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >
> > > Introduce a helper function to indicate whether a virtio device is
> > > operating in legacy or virtio standard mode.
> > >
> > > It may be used to make decisions about the endianess of virtio accesses
> > > and other virtio-1 specific changes, enabling us to support transitional
> > > devices.
> > >
> > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > ---
> > > hw/virtio/virtio.c | 6 +++++-
> > > include/hw/virtio/virtio-access.h | 4 ++++
> > > include/hw/virtio/virtio.h | 13 +++++++++++--
> > > 3 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 7aaa953..e6ae3a0 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
> > > VirtIODevice *vdev = opaque;
> > >
> > > assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> > > - return vdev->device_endian != virtio_default_endian();
> > > + if (virtio_device_is_legacy(vdev)) {
> > > + return vdev->device_endian != virtio_default_endian();
> > > + }
> > > + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> > > + return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
> > > }
> > >
> >
> > Shouldn't we have some code doing the following somewhere ?
> >
> > if (!virtio_device_is_legacy(vdev)) {
> > vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > }
> >
> > also, since virtio-1 is LE only, do we expect device_endian to
> > be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?
>
> device_endian should not depend on whether the device is legacy or not.
> virtio_is_big_endian always returns false for virtio-1 devices, though.
Sorry, I had missed the virtio_is_big_endian() change: it that makes
device_endian a legacy virtio only matter.
So why would we care to migrate the endian subsection when we have a
virtio-1 device ? Shouldn't virtio_device_endian_needed() return false
for virtio-1 ?
--
Greg
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices
2014-10-30 22:29 ` Greg Kurz
@ 2014-11-03 11:44 ` Cornelia Huck
0 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-11-03 11:44 UTC (permalink / raw)
To: Greg Kurz; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Thu, 30 Oct 2014 23:29:50 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Thu, 30 Oct 2014 19:02:01 +0100
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > On Tue, 28 Oct 2014 16:40:18 +0100
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >
> > > On Tue, 7 Oct 2014 16:40:01 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > >
> > > > Introduce a helper function to indicate whether a virtio device is
> > > > operating in legacy or virtio standard mode.
> > > >
> > > > It may be used to make decisions about the endianess of virtio accesses
> > > > and other virtio-1 specific changes, enabling us to support transitional
> > > > devices.
> > > >
> > > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > ---
> > > > hw/virtio/virtio.c | 6 +++++-
> > > > include/hw/virtio/virtio-access.h | 4 ++++
> > > > include/hw/virtio/virtio.h | 13 +++++++++++--
> > > > 3 files changed, 20 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 7aaa953..e6ae3a0 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -883,7 +883,11 @@ static bool virtio_device_endian_needed(void *opaque)
> > > > VirtIODevice *vdev = opaque;
> > > >
> > > > assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> > > > - return vdev->device_endian != virtio_default_endian();
> > > > + if (virtio_device_is_legacy(vdev)) {
> > > > + return vdev->device_endian != virtio_default_endian();
> > > > + }
> > > > + /* Devices conforming to VIRTIO 1.0 or later are always LE. */
> > > > + return vdev->device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
> > > > }
> > > >
> > >
> > > Shouldn't we have some code doing the following somewhere ?
> > >
> > > if (!virtio_device_is_legacy(vdev)) {
> > > vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > > }
> > >
> > > also, since virtio-1 is LE only, do we expect device_endian to
> > > be different from VIRTIO_DEVICE_ENDIAN_LITTLE ?
> >
> > device_endian should not depend on whether the device is legacy or not.
> > virtio_is_big_endian always returns false for virtio-1 devices, though.
>
> Sorry, I had missed the virtio_is_big_endian() change: it that makes
> device_endian a legacy virtio only matter.
> So why would we care to migrate the endian subsection when we have a
> virtio-1 device ? Shouldn't virtio_device_endian_needed() return false
> for virtio-1 ?
Indeeed, we can just leave device_endian at the default value for
virtio-1 devices.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH RFC 06/11] virtio: allow virtio-1 queue layout
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (4 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 05/11] virtio: introduce legacy virtio devices Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices Cornelia Huck
` (6 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
For virtio-1 devices, we allow a more complex queue layout that doesn't
require descriptor table and rings on a physically-contigous memory area:
add virtio_queue_set_rings() to allow transports to set this up.
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/virtio/virtio.c | 16 ++++++++++++++++
include/hw/virtio/virtio.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index e6ae3a0..147d227 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -96,6 +96,13 @@ static void virtqueue_init(VirtQueue *vq)
{
hwaddr pa = vq->pa;
+ if (pa == -1ULL) {
+ /*
+ * This is a virtio-1 style vq that has already been setup
+ * in virtio_queue_set.
+ */
+ return;
+ }
vq->vring.desc = pa;
vq->vring.avail = pa + vq->vring.num * sizeof(VRingDesc);
vq->vring.used = vring_align(vq->vring.avail +
@@ -719,6 +726,15 @@ hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n)
return vdev->vq[n].pa;
}
+void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
+ hwaddr avail, hwaddr used)
+{
+ vdev->vq[n].pa = -1ULL;
+ vdev->vq[n].vring.desc = desc;
+ vdev->vq[n].vring.avail = avail;
+ vdev->vq[n].vring.used = used;
+}
+
void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
{
/* Don't allow guest to flip queue between existent and
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 40e567c..f840320 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -227,6 +227,8 @@ void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
int virtio_queue_get_num(VirtIODevice *vdev, int n);
+void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
+ hwaddr avail, hwaddr used);
void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
void virtio_queue_notify(VirtIODevice *vdev, int n);
uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (5 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 06/11] virtio: allow virtio-1 queue layout Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-28 15:22 ` Greg Kurz
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 08/11] s390x/css: Add a callback for when subchannel gets disabled Cornelia Huck
` (5 subsequent siblings)
12 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
Handle endianness conversion for virtio-1 virtqueues correctly.
Note that dataplane now needs to be built per-target.
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/block/dataplane/virtio-blk.c | 3 +-
hw/scsi/virtio-scsi-dataplane.c | 2 +-
hw/virtio/Makefile.objs | 2 +-
hw/virtio/dataplane/Makefile.objs | 2 +-
hw/virtio/dataplane/vring.c | 85 +++++++++++++++++++----------------
include/hw/virtio/dataplane/vring.h | 64 ++++++++++++++++++++++++--
6 files changed, 113 insertions(+), 45 deletions(-)
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 5458f9d..eb45a3d 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -16,6 +16,7 @@
#include "qemu/iov.h"
#include "qemu/thread.h"
#include "qemu/error-report.h"
+#include "hw/virtio/virtio-access.h"
#include "hw/virtio/dataplane/vring.h"
#include "block/block.h"
#include "hw/virtio/virtio-blk.h"
@@ -75,7 +76,7 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
VirtIOBlockDataPlane *s = req->dev->dataplane;
stb_p(&req->in->status, status);
- vring_push(&req->dev->dataplane->vring, &req->elem,
+ vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
req->qiov.size + sizeof(*req->in));
/* Suppress notification to guest by BH and its scheduled
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index b778e05..3e2b706 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -81,7 +81,7 @@ VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
{
- vring_push(&req->vring->vring, &req->elem,
+ vring_push((VirtIODevice *)req->dev, &req->vring->vring, &req->elem,
req->qsgl.size + req->resp_iov.size);
event_notifier_set(&req->vring->guest_notifier);
}
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index d21c397..19b224a 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -2,7 +2,7 @@ common-obj-y += virtio-rng.o
common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
-common-obj-$(CONFIG_VIRTIO) += dataplane/
+obj-$(CONFIG_VIRTIO) += dataplane/
obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Makefile.objs
index 9a8cfc0..753a9ca 100644
--- a/hw/virtio/dataplane/Makefile.objs
+++ b/hw/virtio/dataplane/Makefile.objs
@@ -1 +1 @@
-common-obj-y += vring.o
+obj-y += vring.o
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index b84957f..4624521 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -18,6 +18,7 @@
#include "hw/hw.h"
#include "exec/memory.h"
#include "exec/address-spaces.h"
+#include "hw/virtio/virtio-access.h"
#include "hw/virtio/dataplane/vring.h"
#include "qemu/error-report.h"
@@ -83,7 +84,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
vring_init(&vring->vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
- vring->last_used_idx = vring->vr.used->idx;
+ vring->last_used_idx = vring_get_used_idx(vdev, vring);
vring->signalled_used = 0;
vring->signalled_used_valid = false;
@@ -104,7 +105,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
{
if (!(vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX))) {
- vring->vr.used->flags |= VRING_USED_F_NO_NOTIFY;
+ vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
}
}
@@ -117,10 +118,10 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
vring_avail_event(&vring->vr) = vring->vr.avail->idx;
} else {
- vring->vr.used->flags &= ~VRING_USED_F_NO_NOTIFY;
+ vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
}
smp_mb(); /* ensure update is seen before reading avail_idx */
- return !vring_more_avail(vring);
+ return !vring_more_avail(vdev, vring);
}
/* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
@@ -134,12 +135,13 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
smp_mb();
if ((vdev->guest_features[0] & VIRTIO_F_NOTIFY_ON_EMPTY) &&
- unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
+ unlikely(!vring_more_avail(vdev, vring))) {
return true;
}
if (!(vdev->guest_features[0] & VIRTIO_RING_F_EVENT_IDX)) {
- return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
+ return !(vring_get_avail_flags(vdev, vring) &
+ VRING_AVAIL_F_NO_INTERRUPT);
}
old = vring->signalled_used;
v = vring->signalled_used_valid;
@@ -154,15 +156,18 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
}
-static int get_desc(Vring *vring, VirtQueueElement *elem,
+static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
struct vring_desc *desc)
{
unsigned *num;
struct iovec *iov;
hwaddr *addr;
MemoryRegion *mr;
+ int is_write = virtio_tswap16(vdev, desc->flags) & VRING_DESC_F_WRITE;
+ uint32_t len = virtio_tswap32(vdev, desc->len);
+ uint64_t desc_addr = virtio_tswap64(vdev, desc->addr);
- if (desc->flags & VRING_DESC_F_WRITE) {
+ if (is_write) {
num = &elem->in_num;
iov = &elem->in_sg[*num];
addr = &elem->in_addr[*num];
@@ -186,44 +191,45 @@ static int get_desc(Vring *vring, VirtQueueElement *elem,
}
/* TODO handle non-contiguous memory across region boundaries */
- iov->iov_base = vring_map(&mr, desc->addr, desc->len,
- desc->flags & VRING_DESC_F_WRITE);
+ iov->iov_base = vring_map(&mr, desc_addr, len, is_write);
if (!iov->iov_base) {
error_report("Failed to map descriptor addr %#" PRIx64 " len %u",
- (uint64_t)desc->addr, desc->len);
+ (uint64_t)desc_addr, len);
return -EFAULT;
}
/* The MemoryRegion is looked up again and unref'ed later, leave the
* ref in place. */
- iov->iov_len = desc->len;
- *addr = desc->addr;
+ iov->iov_len = len;
+ *addr = desc_addr;
*num += 1;
return 0;
}
/* This is stolen from linux/drivers/vhost/vhost.c. */
-static int get_indirect(Vring *vring, VirtQueueElement *elem,
- struct vring_desc *indirect)
+static int get_indirect(VirtIODevice *vdev, Vring *vring,
+ VirtQueueElement *elem, struct vring_desc *indirect)
{
struct vring_desc desc;
unsigned int i = 0, count, found = 0;
int ret;
+ uint32_t len = virtio_tswap32(vdev, indirect->len);
+ uint64_t addr = virtio_tswap64(vdev, indirect->addr);
/* Sanity check */
- if (unlikely(indirect->len % sizeof(desc))) {
+ if (unlikely(len % sizeof(desc))) {
error_report("Invalid length in indirect descriptor: "
"len %#x not multiple of %#zx",
- indirect->len, sizeof(desc));
+ len, sizeof(desc));
vring->broken = true;
return -EFAULT;
}
- count = indirect->len / sizeof(desc);
+ count = len / sizeof(desc);
/* Buffers are chained via a 16 bit next field, so
* we can have at most 2^16 of these. */
if (unlikely(count > USHRT_MAX + 1)) {
- error_report("Indirect buffer length too big: %d", indirect->len);
+ error_report("Indirect buffer length too big: %d", len);
vring->broken = true;
return -EFAULT;
}
@@ -234,12 +240,12 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem,
/* Translate indirect descriptor */
desc_ptr = vring_map(&mr,
- indirect->addr + found * sizeof(desc),
+ addr + found * sizeof(desc),
sizeof(desc), false);
if (!desc_ptr) {
error_report("Failed to map indirect descriptor "
"addr %#" PRIx64 " len %zu",
- (uint64_t)indirect->addr + found * sizeof(desc),
+ (uint64_t)addr + found * sizeof(desc),
sizeof(desc));
vring->broken = true;
return -EFAULT;
@@ -257,19 +263,20 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem,
return -EFAULT;
}
- if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
+ if (unlikely(virtio_tswap16(vdev, desc.flags)
+ & VRING_DESC_F_INDIRECT)) {
error_report("Nested indirect descriptor");
vring->broken = true;
return -EFAULT;
}
- ret = get_desc(vring, elem, &desc);
+ ret = get_desc(vdev, vring, elem, &desc);
if (ret < 0) {
vring->broken |= (ret == -EFAULT);
return ret;
}
- i = desc.next;
- } while (desc.flags & VRING_DESC_F_NEXT);
+ i = virtio_tswap16(vdev, desc.next);
+ } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT);
return 0;
}
@@ -320,7 +327,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
/* Check it isn't doing very strange things with descriptor numbers. */
last_avail_idx = vring->last_avail_idx;
- avail_idx = vring->vr.avail->idx;
+ avail_idx = vring_get_avail_idx(vdev, vring);
barrier(); /* load indices now and not again later */
if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
@@ -341,7 +348,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- head = vring->vr.avail->ring[last_avail_idx % num];
+ head = vring_get_avail_ring(vdev, vring, last_avail_idx % num);
elem->index = head;
@@ -374,21 +381,21 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
/* Ensure descriptor is loaded before accessing fields */
barrier();
- if (desc.flags & VRING_DESC_F_INDIRECT) {
- ret = get_indirect(vring, elem, &desc);
+ if (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_INDIRECT) {
+ ret = get_indirect(vdev, vring, elem, &desc);
if (ret < 0) {
goto out;
}
continue;
}
- ret = get_desc(vring, elem, &desc);
+ ret = get_desc(vdev, vring, elem, &desc);
if (ret < 0) {
goto out;
}
- i = desc.next;
- } while (desc.flags & VRING_DESC_F_NEXT);
+ i = virtio_tswap16(vdev, desc.next);
+ } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT);
/* On success, increment avail index. */
vring->last_avail_idx++;
@@ -407,9 +414,9 @@ out:
*
* Stolen from linux/drivers/vhost/vhost.c.
*/
-void vring_push(Vring *vring, VirtQueueElement *elem, int len)
+void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
+ int len)
{
- struct vring_used_elem *used;
unsigned int head = elem->index;
uint16_t new;
@@ -422,14 +429,16 @@ void vring_push(Vring *vring, VirtQueueElement *elem, int len)
/* The virtqueue contains a ring of used buffers. Get a pointer to the
* next entry in that used ring. */
- used = &vring->vr.used->ring[vring->last_used_idx % vring->vr.num];
- used->id = head;
- used->len = len;
+ vring_set_used_ring_id(vdev, vring, vring->last_used_idx % vring->vr.num,
+ head);
+ vring_set_used_ring_len(vdev, vring, vring->last_used_idx % vring->vr.num,
+ len);
/* Make sure buffer is written before we update index. */
smp_wmb();
- new = vring->vr.used->idx = ++vring->last_used_idx;
+ new = ++vring->last_used_idx;
+ vring_set_used_idx(vdev, vring, new);
if (unlikely((int16_t)(new - vring->signalled_used) < (uint16_t)1)) {
vring->signalled_used_valid = false;
}
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index d3e086a..fde15f3 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -20,6 +20,7 @@
#include "qemu-common.h"
#include "hw/virtio/virtio_ring.h"
#include "hw/virtio/virtio.h"
+#include "hw/virtio/virtio-access.h"
typedef struct {
MemoryRegion *mr; /* memory region containing the vring */
@@ -31,15 +32,71 @@ typedef struct {
bool broken; /* was there a fatal error? */
} Vring;
+static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vring)
+{
+ return virtio_tswap16(vdev, vring->vr.used->idx);
+}
+
+static inline void vring_set_used_idx(VirtIODevice *vdev, Vring *vring,
+ uint16_t idx)
+{
+ vring->vr.used->idx = virtio_tswap16(vdev, idx);
+}
+
+static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vring)
+{
+ return virtio_tswap16(vdev, vring->vr.avail->idx);
+}
+
+static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *vring,
+ int i)
+{
+ return virtio_tswap16(vdev, vring->vr.avail->ring[i]);
+}
+
+static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vring,
+ int i, uint32_t id)
+{
+ vring->vr.used->ring[i].id = virtio_tswap32(vdev, id);
+}
+
+static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vring,
+ int i, uint32_t len)
+{
+ vring->vr.used->ring[i].len = virtio_tswap32(vdev, len);
+}
+
+static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *vring)
+{
+ return virtio_tswap16(vdev, vring->vr.used->flags);
+}
+
+static inline uint16_t vring_get_avail_flags(VirtIODevice *vdev, Vring *vring)
+{
+ return virtio_tswap16(vdev, vring->vr.avail->flags);
+}
+
+static inline void vring_set_used_flags(VirtIODevice *vdev, Vring *vring,
+ uint16_t flags)
+{
+ vring->vr.used->flags |= virtio_tswap16(vdev, flags);
+}
+
+static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vring,
+ uint16_t flags)
+{
+ vring->vr.used->flags &= virtio_tswap16(vdev, ~flags);
+}
+
static inline unsigned int vring_get_num(Vring *vring)
{
return vring->vr.num;
}
/* Are there more descriptors available? */
-static inline bool vring_more_avail(Vring *vring)
+static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
{
- return vring->vr.avail->idx != vring->last_avail_idx;
+ return vring_get_avail_idx(vdev, vring) != vring->last_avail_idx;
}
/* Fail future vring_pop() and vring_push() calls until reset */
@@ -54,6 +111,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
int vring_pop(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem);
-void vring_push(Vring *vring, VirtQueueElement *elem, int len);
+void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
+ int len);
#endif /* VRING_H */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices Cornelia Huck
@ 2014-10-28 15:22 ` Greg Kurz
2014-10-30 9:18 ` Cornelia Huck
0 siblings, 1 reply; 36+ messages in thread
From: Greg Kurz @ 2014-10-28 15:22 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Tue, 7 Oct 2014 16:40:03 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> Handle endianness conversion for virtio-1 virtqueues correctly.
>
> Note that dataplane now needs to be built per-target.
>
It also affects hw/virtio/virtio-pci.c:
In file included from include/hw/virtio/dataplane/vring.h:23:0,
from include/hw/virtio/virtio-scsi.h:21,
from hw/virtio/virtio-pci.c:24:
include/hw/virtio/virtio-access.h: In function ‘virtio_access_is_big_endian’:
include/hw/virtio/virtio-access.h:28:15: error: attempt to use poisoned "TARGET_WORDS_BIGENDIAN"
#elif defined(TARGET_WORDS_BIGENDIAN)
^
FWIW when I added endian ambivalent support to virtio, I remember *some people*
getting angry at the idea of turning common code into per-target... :)
See comment below.
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
> hw/block/dataplane/virtio-blk.c | 3 +-
> hw/scsi/virtio-scsi-dataplane.c | 2 +-
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/dataplane/Makefile.objs | 2 +-
> hw/virtio/dataplane/vring.c | 85 +++++++++++++++++++----------------
> include/hw/virtio/dataplane/vring.h | 64 ++++++++++++++++++++++++--
> 6 files changed, 113 insertions(+), 45 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 5458f9d..eb45a3d 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -16,6 +16,7 @@
> #include "qemu/iov.h"
> #include "qemu/thread.h"
> #include "qemu/error-report.h"
> +#include "hw/virtio/virtio-access.h"
> #include "hw/virtio/dataplane/vring.h"
> #include "block/block.h"
> #include "hw/virtio/virtio-blk.h"
> @@ -75,7 +76,7 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
> VirtIOBlockDataPlane *s = req->dev->dataplane;
> stb_p(&req->in->status, status);
>
> - vring_push(&req->dev->dataplane->vring, &req->elem,
> + vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem,
> req->qiov.size + sizeof(*req->in));
>
> /* Suppress notification to guest by BH and its scheduled
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index b778e05..3e2b706 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -81,7 +81,7 @@ VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
>
> void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
> {
> - vring_push(&req->vring->vring, &req->elem,
> + vring_push((VirtIODevice *)req->dev, &req->vring->vring, &req->elem,
> req->qsgl.size + req->resp_iov.size);
> event_notifier_set(&req->vring->guest_notifier);
> }
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index d21c397..19b224a 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -2,7 +2,7 @@ common-obj-y += virtio-rng.o
> common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
> common-obj-y += virtio-bus.o
> common-obj-y += virtio-mmio.o
> -common-obj-$(CONFIG_VIRTIO) += dataplane/
> +obj-$(CONFIG_VIRTIO) += dataplane/
>
> obj-y += virtio.o virtio-balloon.o
> obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Makefile.objs
> index 9a8cfc0..753a9ca 100644
> --- a/hw/virtio/dataplane/Makefile.objs
> +++ b/hw/virtio/dataplane/Makefile.objs
> @@ -1 +1 @@
> -common-obj-y += vring.o
> +obj-y += vring.o
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index b84957f..4624521 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -18,6 +18,7 @@
> #include "hw/hw.h"
> #include "exec/memory.h"
> #include "exec/address-spaces.h"
> +#include "hw/virtio/virtio-access.h"
> #include "hw/virtio/dataplane/vring.h"
> #include "qemu/error-report.h"
>
> @@ -83,7 +84,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
> vring_init(&vring->vr, virtio_queue_get_num(vdev, n), vring_ptr, 4096);
>
> vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
> - vring->last_used_idx = vring->vr.used->idx;
> + vring->last_used_idx = vring_get_used_idx(vdev, vring);
> vring->signalled_used = 0;
> vring->signalled_used_valid = false;
>
> @@ -104,7 +105,7 @@ void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
> void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
> {
> if (!(vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX))) {
> - vring->vr.used->flags |= VRING_USED_F_NO_NOTIFY;
> + vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
> }
> }
>
> @@ -117,10 +118,10 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
> if (vdev->guest_features[0] & (1 << VIRTIO_RING_F_EVENT_IDX)) {
> vring_avail_event(&vring->vr) = vring->vr.avail->idx;
> } else {
> - vring->vr.used->flags &= ~VRING_USED_F_NO_NOTIFY;
> + vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
> }
> smp_mb(); /* ensure update is seen before reading avail_idx */
> - return !vring_more_avail(vring);
> + return !vring_more_avail(vdev, vring);
> }
>
> /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
> @@ -134,12 +135,13 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
> smp_mb();
>
> if ((vdev->guest_features[0] & VIRTIO_F_NOTIFY_ON_EMPTY) &&
> - unlikely(vring->vr.avail->idx == vring->last_avail_idx)) {
> + unlikely(!vring_more_avail(vdev, vring))) {
> return true;
> }
>
> if (!(vdev->guest_features[0] & VIRTIO_RING_F_EVENT_IDX)) {
> - return !(vring->vr.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
> + return !(vring_get_avail_flags(vdev, vring) &
> + VRING_AVAIL_F_NO_INTERRUPT);
> }
> old = vring->signalled_used;
> v = vring->signalled_used_valid;
> @@ -154,15 +156,18 @@ bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
> }
>
>
> -static int get_desc(Vring *vring, VirtQueueElement *elem,
> +static int get_desc(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> struct vring_desc *desc)
> {
> unsigned *num;
> struct iovec *iov;
> hwaddr *addr;
> MemoryRegion *mr;
> + int is_write = virtio_tswap16(vdev, desc->flags) & VRING_DESC_F_WRITE;
> + uint32_t len = virtio_tswap32(vdev, desc->len);
> + uint64_t desc_addr = virtio_tswap64(vdev, desc->addr);
>
> - if (desc->flags & VRING_DESC_F_WRITE) {
> + if (is_write) {
> num = &elem->in_num;
> iov = &elem->in_sg[*num];
> addr = &elem->in_addr[*num];
> @@ -186,44 +191,45 @@ static int get_desc(Vring *vring, VirtQueueElement *elem,
> }
>
> /* TODO handle non-contiguous memory across region boundaries */
> - iov->iov_base = vring_map(&mr, desc->addr, desc->len,
> - desc->flags & VRING_DESC_F_WRITE);
> + iov->iov_base = vring_map(&mr, desc_addr, len, is_write);
> if (!iov->iov_base) {
> error_report("Failed to map descriptor addr %#" PRIx64 " len %u",
> - (uint64_t)desc->addr, desc->len);
> + (uint64_t)desc_addr, len);
> return -EFAULT;
> }
>
> /* The MemoryRegion is looked up again and unref'ed later, leave the
> * ref in place. */
> - iov->iov_len = desc->len;
> - *addr = desc->addr;
> + iov->iov_len = len;
> + *addr = desc_addr;
> *num += 1;
> return 0;
> }
>
> /* This is stolen from linux/drivers/vhost/vhost.c. */
> -static int get_indirect(Vring *vring, VirtQueueElement *elem,
> - struct vring_desc *indirect)
> +static int get_indirect(VirtIODevice *vdev, Vring *vring,
> + VirtQueueElement *elem, struct vring_desc *indirect)
> {
> struct vring_desc desc;
> unsigned int i = 0, count, found = 0;
> int ret;
> + uint32_t len = virtio_tswap32(vdev, indirect->len);
> + uint64_t addr = virtio_tswap64(vdev, indirect->addr);
>
> /* Sanity check */
> - if (unlikely(indirect->len % sizeof(desc))) {
> + if (unlikely(len % sizeof(desc))) {
> error_report("Invalid length in indirect descriptor: "
> "len %#x not multiple of %#zx",
> - indirect->len, sizeof(desc));
> + len, sizeof(desc));
> vring->broken = true;
> return -EFAULT;
> }
>
> - count = indirect->len / sizeof(desc);
> + count = len / sizeof(desc);
> /* Buffers are chained via a 16 bit next field, so
> * we can have at most 2^16 of these. */
> if (unlikely(count > USHRT_MAX + 1)) {
> - error_report("Indirect buffer length too big: %d", indirect->len);
> + error_report("Indirect buffer length too big: %d", len);
> vring->broken = true;
> return -EFAULT;
> }
> @@ -234,12 +240,12 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem,
>
> /* Translate indirect descriptor */
> desc_ptr = vring_map(&mr,
> - indirect->addr + found * sizeof(desc),
> + addr + found * sizeof(desc),
> sizeof(desc), false);
> if (!desc_ptr) {
> error_report("Failed to map indirect descriptor "
> "addr %#" PRIx64 " len %zu",
> - (uint64_t)indirect->addr + found * sizeof(desc),
> + (uint64_t)addr + found * sizeof(desc),
> sizeof(desc));
> vring->broken = true;
> return -EFAULT;
> @@ -257,19 +263,20 @@ static int get_indirect(Vring *vring, VirtQueueElement *elem,
> return -EFAULT;
> }
>
> - if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
> + if (unlikely(virtio_tswap16(vdev, desc.flags)
> + & VRING_DESC_F_INDIRECT)) {
> error_report("Nested indirect descriptor");
> vring->broken = true;
> return -EFAULT;
> }
>
> - ret = get_desc(vring, elem, &desc);
> + ret = get_desc(vdev, vring, elem, &desc);
> if (ret < 0) {
> vring->broken |= (ret == -EFAULT);
> return ret;
> }
> - i = desc.next;
> - } while (desc.flags & VRING_DESC_F_NEXT);
> + i = virtio_tswap16(vdev, desc.next);
> + } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT);
> return 0;
> }
>
> @@ -320,7 +327,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>
> /* Check it isn't doing very strange things with descriptor numbers. */
> last_avail_idx = vring->last_avail_idx;
> - avail_idx = vring->vr.avail->idx;
> + avail_idx = vring_get_avail_idx(vdev, vring);
> barrier(); /* load indices now and not again later */
>
> if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
> @@ -341,7 +348,7 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>
> /* Grab the next descriptor number they're advertising, and increment
> * the index we've seen. */
> - head = vring->vr.avail->ring[last_avail_idx % num];
> + head = vring_get_avail_ring(vdev, vring, last_avail_idx % num);
>
> elem->index = head;
>
> @@ -374,21 +381,21 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
> /* Ensure descriptor is loaded before accessing fields */
> barrier();
>
> - if (desc.flags & VRING_DESC_F_INDIRECT) {
> - ret = get_indirect(vring, elem, &desc);
> + if (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_INDIRECT) {
> + ret = get_indirect(vdev, vring, elem, &desc);
> if (ret < 0) {
> goto out;
> }
> continue;
> }
>
> - ret = get_desc(vring, elem, &desc);
> + ret = get_desc(vdev, vring, elem, &desc);
> if (ret < 0) {
> goto out;
> }
>
> - i = desc.next;
> - } while (desc.flags & VRING_DESC_F_NEXT);
> + i = virtio_tswap16(vdev, desc.next);
> + } while (virtio_tswap16(vdev, desc.flags) & VRING_DESC_F_NEXT);
>
> /* On success, increment avail index. */
> vring->last_avail_idx++;
> @@ -407,9 +414,9 @@ out:
> *
> * Stolen from linux/drivers/vhost/vhost.c.
> */
> -void vring_push(Vring *vring, VirtQueueElement *elem, int len)
> +void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> + int len)
> {
> - struct vring_used_elem *used;
> unsigned int head = elem->index;
> uint16_t new;
>
> @@ -422,14 +429,16 @@ void vring_push(Vring *vring, VirtQueueElement *elem, int len)
>
> /* The virtqueue contains a ring of used buffers. Get a pointer to the
> * next entry in that used ring. */
> - used = &vring->vr.used->ring[vring->last_used_idx % vring->vr.num];
> - used->id = head;
> - used->len = len;
> + vring_set_used_ring_id(vdev, vring, vring->last_used_idx % vring->vr.num,
> + head);
> + vring_set_used_ring_len(vdev, vring, vring->last_used_idx % vring->vr.num,
> + len);
>
> /* Make sure buffer is written before we update index. */
> smp_wmb();
>
> - new = vring->vr.used->idx = ++vring->last_used_idx;
> + new = ++vring->last_used_idx;
> + vring_set_used_idx(vdev, vring, new);
> if (unlikely((int16_t)(new - vring->signalled_used) < (uint16_t)1)) {
> vring->signalled_used_valid = false;
> }
> diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
> index d3e086a..fde15f3 100644
> --- a/
> +++ b/include/hw/virtio/dataplane/vring.h
> @@ -20,6 +20,7 @@
> #include "qemu-common.h"
> #include "hw/virtio/virtio_ring.h"
> #include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-access.h"
>
Since the following commit:
commit 244e2898b7a7735b3da114c120abe206af56a167
Author: Fam Zheng <famz@redhat.com>
Date: Wed Sep 24 15:21:41 2014 +0800
virtio-scsi: Add VirtIOSCSIVring in VirtIOSCSIReq
The include/hw/virtio/dataplane/vring.h header is indirectly included
by hw/virtio/virtio-pci.c. Why don't you move all this target dependent
helpers to another header ?
Cheers.
--
Greg
> typedef struct {
> MemoryRegion *mr; /* memory region containing the vring */
> @@ -31,15 +32,71 @@ typedef struct {
> bool broken; /* was there a fatal error? */
> } Vring;
>
> +static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vring)
> +{
> + return virtio_tswap16(vdev, vring->vr.used->idx);
> +}
> +
> +static inline void vring_set_used_idx(VirtIODevice *vdev, Vring *vring,
> + uint16_t idx)
> +{
> + vring->vr.used->idx = virtio_tswap16(vdev, idx);
> +}
> +
> +static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vring)
> +{
> + return virtio_tswap16(vdev, vring->vr.avail->idx);
> +}
> +
> +static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *vring,
> + int i)
> +{
> + return virtio_tswap16(vdev, vring->vr.avail->ring[i]);
> +}
> +
> +static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vring,
> + int i, uint32_t id)
> +{
> + vring->vr.used->ring[i].id = virtio_tswap32(vdev, id);
> +}
> +
> +static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vring,
> + int i, uint32_t len)
> +{
> + vring->vr.used->ring[i].len = virtio_tswap32(vdev, len);
> +}
> +
> +static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *vring)
> +{
> + return virtio_tswap16(vdev, vring->vr.used->flags);
> +}
> +
> +static inline uint16_t vring_get_avail_flags(VirtIODevice *vdev, Vring *vring)
> +{
> + return virtio_tswap16(vdev, vring->vr.avail->flags);
> +}
> +
> +static inline void vring_set_used_flags(VirtIODevice *vdev, Vring *vring,
> + uint16_t flags)
> +{
> + vring->vr.used->flags |= virtio_tswap16(vdev, flags);
> +}
> +
> +static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vring,
> + uint16_t flags)
> +{
> + vring->vr.used->flags &= virtio_tswap16(vdev, ~flags);
> +}
> +
> static inline unsigned int vring_get_num(Vring *vring)
> {
> return vring->vr.num;
> }
>
> /* Are there more descriptors available? */
> -static inline bool vring_more_avail(Vring *vring)
> +static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
> {
> - return vring->vr.avail->idx != vring->last_avail_idx;
> + return vring_get_avail_idx(vdev, vring) != vring->last_avail_idx;
> }
>
> /* Fail future vring_pop() and vring_push() calls until reset */
> @@ -54,6 +111,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
> bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
> bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
> int vring_pop(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem);
> -void vring_push(Vring *vring, VirtQueueElement *elem, int len);
> +void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> + int len);
>
> #endif /* VRING_H */
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices
2014-10-28 15:22 ` Greg Kurz
@ 2014-10-30 9:18 ` Cornelia Huck
0 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-30 9:18 UTC (permalink / raw)
To: Greg Kurz; +Cc: thuth, rusty, qemu-devel, kvm, virtualization
On Tue, 28 Oct 2014 16:22:54 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Tue, 7 Oct 2014 16:40:03 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > Handle endianness conversion for virtio-1 virtqueues correctly.
> >
> > Note that dataplane now needs to be built per-target.
> >
>
> It also affects hw/virtio/virtio-pci.c:
>
> In file included from include/hw/virtio/dataplane/vring.h:23:0,
> from include/hw/virtio/virtio-scsi.h:21,
> from hw/virtio/virtio-pci.c:24:
> include/hw/virtio/virtio-access.h: In function ‘virtio_access_is_big_endian’:
> include/hw/virtio/virtio-access.h:28:15: error: attempt to use poisoned "TARGET_WORDS_BIGENDIAN"
> #elif defined(TARGET_WORDS_BIGENDIAN)
> ^
>
> FWIW when I added endian ambivalent support to virtio, I remember *some people*
> getting angry at the idea of turning common code into per-target... :)
Well, it probably can't be helped for something that is
endian-sensitive like virtio :( (Although we should try to keep it as
local as possible.)
>
> See comment below.
>
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> > hw/block/dataplane/virtio-blk.c | 3 +-
> > hw/scsi/virtio-scsi-dataplane.c | 2 +-
> > hw/virtio/Makefile.objs | 2 +-
> > hw/virtio/dataplane/Makefile.objs | 2 +-
> > hw/virtio/dataplane/vring.c | 85 +++++++++++++++++++----------------
> > include/hw/virtio/dataplane/vring.h | 64 ++++++++++++++++++++++++--
> > 6 files changed, 113 insertions(+), 45 deletions(-)
> >
> > diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
> > index d3e086a..fde15f3 100644
> > --- a/
> > +++ b/include/hw/virtio/dataplane/vring.h
> > @@ -20,6 +20,7 @@
> > #include "qemu-common.h"
> > #include "hw/virtio/virtio_ring.h"
> > #include "hw/virtio/virtio.h"
> > +#include "hw/virtio/virtio-access.h"
> >
>
> Since the following commit:
>
> commit 244e2898b7a7735b3da114c120abe206af56a167
> Author: Fam Zheng <famz@redhat.com>
> Date: Wed Sep 24 15:21:41 2014 +0800
>
> virtio-scsi: Add VirtIOSCSIVring in VirtIOSCSIReq
>
> The include/hw/virtio/dataplane/vring.h header is indirectly included
> by hw/virtio/virtio-pci.c. Why don't you move all this target dependent
> helpers to another header ?
Ah, this seems to have come in after I hacked on that code - I'll take a
look at splitting off the accessors.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH RFC 08/11] s390x/css: Add a callback for when subchannel gets disabled
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (6 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 07/11] dataplane: allow virtio-1 devices Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 09/11] s390x/virtio-ccw: add virtio set-revision call Cornelia Huck
` (4 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
From: Thomas Huth <thuth@linux.vnet.ibm.com>
We need a possibility to run code when a subchannel gets disabled.
This patch adds the necessary infrastructure.
Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/css.c | 12 ++++++++++++
hw/s390x/css.h | 1 +
2 files changed, 13 insertions(+)
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index b67c039..735ec55 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -588,6 +588,7 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
{
SCSW *s = &sch->curr_status.scsw;
PMCW *p = &sch->curr_status.pmcw;
+ uint16_t oldflags;
int ret;
SCHIB schib;
@@ -610,6 +611,7 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
copy_schib_from_guest(&schib, orig_schib);
/* Only update the program-modifiable fields. */
p->intparm = schib.pmcw.intparm;
+ oldflags = p->flags;
p->flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
PMCW_FLAGS_MASK_MP);
@@ -625,6 +627,12 @@ int css_do_msch(SubchDev *sch, SCHIB *orig_schib)
(PMCW_CHARS_MASK_MBFC | PMCW_CHARS_MASK_CSENSE);
sch->curr_status.mba = schib.mba;
+ /* Has the channel been disabled? */
+ if (sch->disable_cb && (oldflags & PMCW_FLAGS_MASK_ENA) != 0
+ && (p->flags & PMCW_FLAGS_MASK_ENA) == 0) {
+ sch->disable_cb(sch);
+ }
+
ret = 0;
out:
@@ -1443,6 +1451,10 @@ void css_reset_sch(SubchDev *sch)
{
PMCW *p = &sch->curr_status.pmcw;
+ if ((p->flags & PMCW_FLAGS_MASK_ENA) != 0 && sch->disable_cb) {
+ sch->disable_cb(sch);
+ }
+
p->intparm = 0;
p->flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA |
PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME |
diff --git a/hw/s390x/css.h b/hw/s390x/css.h
index 33104ac..7fa807b 100644
--- a/hw/s390x/css.h
+++ b/hw/s390x/css.h
@@ -81,6 +81,7 @@ struct SubchDev {
uint8_t ccw_no_data_cnt;
/* transport-provided data: */
int (*ccw_cb) (SubchDev *, CCW1);
+ void (*disable_cb)(SubchDev *);
SenseId id;
void *driver_data;
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 09/11] s390x/virtio-ccw: add virtio set-revision call
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (7 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 08/11] s390x/css: Add a callback for when subchannel gets disabled Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 10/11] s390x/virtio-ccw: support virtio-1 set_vq format Cornelia Huck
` (3 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
From: Thomas Huth <thuth@linux.vnet.ibm.com>
Handle the virtio-ccw revision according to what the guest sets.
When revision 1 is selected, we have a virtio-1 standard device
with byteswapping for the virtio rings.
When a channel gets disabled, we have to revert to the legacy behavior
in case the next user of the device does not negotiate the revision 1
anymore (e.g. the boot firmware uses revision 1, but the operating
system only uses the legacy mode).
Note that revisions > 0 are still disabled; but we still extend the
feature bit size to be able to handle the VERSION_1 bit.
Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/virtio-ccw.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
hw/s390x/virtio-ccw.h | 7 ++++++-
2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 69add47..0d414f6 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -20,9 +20,11 @@
#include "hw/virtio/virtio-net.h"
#include "hw/sysbus.h"
#include "qemu/bitops.h"
+#include "hw/virtio/virtio-access.h"
#include "hw/virtio/virtio-bus.h"
#include "hw/s390x/adapter.h"
#include "hw/s390x/s390_flic.h"
+#include "linux/virtio_config.h"
#include "ioinst.h"
#include "css.h"
@@ -260,6 +262,12 @@ typedef struct VirtioThinintInfo {
uint8_t isc;
} QEMU_PACKED VirtioThinintInfo;
+typedef struct VirtioRevInfo {
+ uint16_t revision;
+ uint16_t length;
+ uint8_t data[0];
+} QEMU_PACKED VirtioRevInfo;
+
/* Specify where the virtqueues for the subchannel are in guest memory. */
static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
uint16_t index, uint16_t num)
@@ -299,6 +307,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
{
int ret;
VqInfoBlock info;
+ VirtioRevInfo revinfo;
uint8_t status;
VirtioFeatDesc features;
void *config;
@@ -373,6 +382,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
if (features.index < ARRAY_SIZE(dev->host_features)) {
features.features = dev->host_features[features.index];
+ /*
+ * Don't offer version 1 to the guest if it did not
+ * negotiate at least revision 1.
+ */
+ if (features.index == 1 && dev->revision <= 0) {
+ features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
} else {
/* Return zeroes if the guest supports more feature bits. */
features.features = 0;
@@ -400,6 +416,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
ccw.cda + sizeof(features.features));
features.features = ldl_le_phys(&address_space_memory, ccw.cda);
if (features.index < ARRAY_SIZE(vdev->guest_features)) {
+ /*
+ * The guest should not set version 1 if it didn't
+ * negotiate a revision >= 1.
+ */
+ if (features.index == 1 && dev->revision <= 0) {
+ features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
virtio_set_features(vdev, features.index, features.features);
} else {
/*
@@ -600,6 +623,25 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
}
}
break;
+ case CCW_CMD_SET_VIRTIO_REV:
+ len = sizeof(revinfo);
+ if (ccw.count < len || (check_len && ccw.count > len)) {
+ ret = -EINVAL;
+ break;
+ }
+ if (!ccw.cda) {
+ ret = -EFAULT;
+ break;
+ }
+ cpu_physical_memory_read(ccw.cda, &revinfo, len);
+ if (dev->revision >= 0 ||
+ revinfo.revision > VIRTIO_CCW_REV_MAX) {
+ ret = -ENOSYS;
+ break;
+ }
+ ret = 0;
+ dev->revision = revinfo.revision;
+ break;
default:
ret = -ENOSYS;
break;
@@ -607,6 +649,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
return ret;
}
+static void virtio_sch_disable_cb(SubchDev *sch)
+{
+ VirtioCcwDevice *dev = sch->driver_data;
+
+ dev->revision = -1;
+}
+
static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
{
unsigned int cssid = 0;
@@ -733,6 +782,7 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
css_sch_build_virtual_schib(sch, 0, VIRTIO_CCW_CHPID_TYPE);
sch->ccw_cb = virtio_ccw_cb;
+ sch->disable_cb = virtio_sch_disable_cb;
/* Build senseid data. */
memset(&sch->id, 0, sizeof(SenseId));
@@ -740,6 +790,8 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
sch->id.cu_type = VIRTIO_CCW_CU_TYPE;
sch->id.cu_model = vdev->device_id;
+ dev->revision = -1;
+
/* Set default feature bits that are offered by the host. */
for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
dev->host_features[i] =
@@ -749,6 +801,8 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
+ dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
+
css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
parent->hotplugged, 1);
return 0;
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 5a1f16e..03d5955 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -40,6 +40,7 @@
#define CCW_CMD_SET_CONF_IND 0x53
#define CCW_CMD_READ_VQ_CONF 0x32
#define CCW_CMD_SET_IND_ADAPTER 0x73
+#define CCW_CMD_SET_VIRTIO_REV 0x83
#define TYPE_VIRTIO_CCW_DEVICE "virtio-ccw-device"
#define VIRTIO_CCW_DEVICE(obj) \
@@ -69,7 +70,10 @@ typedef struct VirtIOCCWDeviceClass {
} VirtIOCCWDeviceClass;
/* Change here if we want to support more feature bits. */
-#define VIRTIO_CCW_FEATURE_SIZE 1
+#define VIRTIO_CCW_FEATURE_SIZE NR_VIRTIO_FEATURE_WORDS
+
+/* The maximum virtio revision we support. */
+#define VIRTIO_CCW_REV_MAX 0
/* Performance improves when virtqueue kick processing is decoupled from the
* vcpu thread using ioeventfd for some devices. */
@@ -89,6 +93,7 @@ struct VirtioCcwDevice {
SubchDev *sch;
char *bus_id;
uint32_t host_features[VIRTIO_CCW_FEATURE_SIZE];
+ int revision;
VirtioBusState bus;
bool ioeventfd_started;
bool ioeventfd_disabled;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 10/11] s390x/virtio-ccw: support virtio-1 set_vq format
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (8 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 09/11] s390x/virtio-ccw: add virtio set-revision call Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 11/11] s390x/virtio-ccw: enable virtio 1.0 Cornelia Huck
` (2 subsequent siblings)
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
Support the new CCW_CMD_SET_VQ format for virtio-1 devices.
While we're at it, refactor the code a bit and enforce big endian
fields (which had always been required, even for legacy).
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/virtio-ccw.c | 114 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 80 insertions(+), 34 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 0d414f6..80efe88 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -238,11 +238,20 @@ VirtualCssBus *virtual_css_bus_init(void)
}
/* Communication blocks used by several channel commands. */
-typedef struct VqInfoBlock {
+typedef struct VqInfoBlockLegacy {
uint64_t queue;
uint32_t align;
uint16_t index;
uint16_t num;
+} QEMU_PACKED VqInfoBlockLegacy;
+
+typedef struct VqInfoBlock {
+ uint64_t desc;
+ uint32_t res0;
+ uint16_t index;
+ uint16_t num;
+ uint64_t avail;
+ uint64_t used;
} QEMU_PACKED VqInfoBlock;
typedef struct VqConfigBlock {
@@ -269,17 +278,20 @@ typedef struct VirtioRevInfo {
} QEMU_PACKED VirtioRevInfo;
/* Specify where the virtqueues for the subchannel are in guest memory. */
-static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
- uint16_t index, uint16_t num)
+static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
+ VqInfoBlockLegacy *linfo)
{
VirtIODevice *vdev = virtio_ccw_get_vdev(sch);
+ uint16_t index = info ? info->index : linfo->index;
+ uint16_t num = info ? info->num : linfo->num;
+ uint64_t desc = info ? info->desc : linfo->queue;
if (index > VIRTIO_PCI_QUEUE_MAX) {
return -EINVAL;
}
/* Current code in virtio.c relies on 4K alignment. */
- if (addr && (align != 4096)) {
+ if (linfo && desc && (linfo->align != 4096)) {
return -EINVAL;
}
@@ -287,8 +299,12 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
return -EINVAL;
}
- virtio_queue_set_addr(vdev, index, addr);
- if (!addr) {
+ if (info) {
+ virtio_queue_set_rings(vdev, index, desc, info->avail, info->used);
+ } else {
+ virtio_queue_set_addr(vdev, index, desc);
+ }
+ if (!desc) {
virtio_queue_set_vector(vdev, index, 0);
} else {
/* Fail if we don't have a big enough queue. */
@@ -303,10 +319,66 @@ static int virtio_ccw_set_vqs(SubchDev *sch, uint64_t addr, uint32_t align,
return 0;
}
-static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+static int virtio_ccw_handle_set_vq(SubchDev *sch, CCW1 ccw, bool check_len,
+ bool is_legacy)
{
int ret;
VqInfoBlock info;
+ VqInfoBlockLegacy linfo;
+ size_t info_len = is_legacy ? sizeof(linfo) : sizeof(info);
+
+ if (check_len) {
+ if (ccw.count != info_len) {
+ return -EINVAL;
+ }
+ } else if (ccw.count < info_len) {
+ /* Can't execute command. */
+ return -EINVAL;
+ }
+ if (!ccw.cda) {
+ return -EFAULT;
+ }
+ if (is_legacy) {
+ linfo.queue = ldq_be_phys(&address_space_memory, ccw.cda);
+ linfo.align = ldl_be_phys(&address_space_memory,
+ ccw.cda + sizeof(linfo.queue));
+ linfo.index = lduw_be_phys(&address_space_memory,
+ ccw.cda + sizeof(linfo.queue)
+ + sizeof(linfo.align));
+ linfo.num = lduw_be_phys(&address_space_memory,
+ ccw.cda + sizeof(linfo.queue)
+ + sizeof(linfo.align)
+ + sizeof(linfo.index));
+ ret = virtio_ccw_set_vqs(sch, NULL, &linfo);
+ } else {
+ info.desc = ldq_be_phys(&address_space_memory, ccw.cda);
+ info.index = lduw_be_phys(&address_space_memory,
+ ccw.cda + sizeof(info.desc)
+ + sizeof(info.res0));
+ info.num = lduw_be_phys(&address_space_memory,
+ ccw.cda + sizeof(info.desc)
+ + sizeof(info.res0)
+ + sizeof(info.index));
+ info.avail = ldq_be_phys(&address_space_memory,
+ ccw.cda + sizeof(info.desc)
+ + sizeof(info.res0)
+ + sizeof(info.index)
+ + sizeof(info.num));
+ info.used = ldq_be_phys(&address_space_memory,
+ ccw.cda + sizeof(info.desc)
+ + sizeof(info.res0)
+ + sizeof(info.index)
+ + sizeof(info.num)
+ + sizeof(info.avail));
+ ret = virtio_ccw_set_vqs(sch, &info, NULL);
+ }
+ sch->curr_status.scsw.count = 0;
+ return ret;
+}
+
+static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
+{
+ int ret;
VirtioRevInfo revinfo;
uint8_t status;
VirtioFeatDesc features;
@@ -331,33 +403,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
/* Look at the command. */
switch (ccw.cmd_code) {
case CCW_CMD_SET_VQ:
- if (check_len) {
- if (ccw.count != sizeof(info)) {
- ret = -EINVAL;
- break;
- }
- } else if (ccw.count < sizeof(info)) {
- /* Can't execute command. */
- ret = -EINVAL;
- break;
- }
- if (!ccw.cda) {
- ret = -EFAULT;
- } else {
- info.queue = ldq_phys(&address_space_memory, ccw.cda);
- info.align = ldl_phys(&address_space_memory,
- ccw.cda + sizeof(info.queue));
- info.index = lduw_phys(&address_space_memory,
- ccw.cda + sizeof(info.queue)
- + sizeof(info.align));
- info.num = lduw_phys(&address_space_memory,
- ccw.cda + sizeof(info.queue)
- + sizeof(info.align)
- + sizeof(info.index));
- ret = virtio_ccw_set_vqs(sch, info.queue, info.align, info.index,
- info.num);
- sch->curr_status.scsw.count = 0;
- }
+ ret = virtio_ccw_handle_set_vq(sch, ccw, check_len, dev->revision < 1);
break;
case CCW_CMD_VDEV_RESET:
virtio_ccw_stop_ioeventfd(dev);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH RFC 11/11] s390x/virtio-ccw: enable virtio 1.0
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (9 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 10/11] s390x/virtio-ccw: support virtio-1 set_vq format Cornelia Huck
@ 2014-10-07 14:40 ` Cornelia Huck
2014-10-08 1:24 ` [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Andy Lutomirski
2014-10-23 21:42 ` Michael S. Tsirkin
12 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-07 14:40 UTC (permalink / raw)
To: virtualization, qemu-devel, kvm; +Cc: Cornelia Huck, rusty, thuth
virtio-ccw should now have everything in place to operate virtio 1.0
devices, so let's enable revision 1.
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
hw/s390x/virtio-ccw.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 03d5955..08edd8d 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -73,7 +73,7 @@ typedef struct VirtIOCCWDeviceClass {
#define VIRTIO_CCW_FEATURE_SIZE NR_VIRTIO_FEATURE_WORDS
/* The maximum virtio revision we support. */
-#define VIRTIO_CCW_REV_MAX 0
+#define VIRTIO_CCW_REV_MAX 1
/* Performance improves when virtqueue kick processing is decoupled from the
* vcpu thread using ioeventfd for some devices. */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (10 preceding siblings ...)
2014-10-07 14:40 ` [Qemu-devel] [PATCH RFC 11/11] s390x/virtio-ccw: enable virtio 1.0 Cornelia Huck
@ 2014-10-08 1:24 ` Andy Lutomirski
2014-10-08 9:04 ` Cornelia Huck
2014-10-23 21:42 ` Michael S. Tsirkin
12 siblings, 1 reply; 36+ messages in thread
From: Andy Lutomirski @ 2014-10-08 1:24 UTC (permalink / raw)
To: Cornelia Huck, virtualization, qemu-devel, kvm; +Cc: thuth
On 10/07/2014 07:39 AM, Cornelia Huck wrote:
> This patchset aims to get us some way to implement virtio-1 compliant
> and transitional devices in qemu. Branch available at
>
> git://github.com/cohuck/qemu virtio-1
>
> I've mainly focused on:
> - endianness handling
> - extended feature bits
> - virtio-ccw new/changed commands
At the risk of some distraction, would it be worth thinking about a
solution to the IOMMU bypassing mess as part of this?
--Andy
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-08 1:24 ` [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Andy Lutomirski
@ 2014-10-08 9:04 ` Cornelia Huck
2014-10-22 8:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-08 9:04 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: thuth, qemu-devel, kvm, virtualization
On Tue, 07 Oct 2014 18:24:22 -0700
Andy Lutomirski <luto@amacapital.net> wrote:
> On 10/07/2014 07:39 AM, Cornelia Huck wrote:
> > This patchset aims to get us some way to implement virtio-1 compliant
> > and transitional devices in qemu. Branch available at
> >
> > git://github.com/cohuck/qemu virtio-1
> >
> > I've mainly focused on:
> > - endianness handling
> > - extended feature bits
> > - virtio-ccw new/changed commands
>
> At the risk of some distraction, would it be worth thinking about a
> solution to the IOMMU bypassing mess as part of this?
I think that is a whole different issue. virtio-1 is basically done - we
just need to implement it - while the IOMMU/DMA stuff certainly needs
more discussion. Therefore, I'd like to defer to the other discussion
thread here.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-08 9:04 ` Cornelia Huck
@ 2014-10-22 8:44 ` Michael S. Tsirkin
2014-10-22 14:17 ` Jan Kiszka
0 siblings, 1 reply; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-22 8:44 UTC (permalink / raw)
To: Cornelia Huck
Cc: thuth, kvm, qemu-devel, virtualization, jan.kiszka,
Andy Lutomirski
On Wed, Oct 08, 2014 at 11:04:28AM +0200, Cornelia Huck wrote:
> On Tue, 07 Oct 2014 18:24:22 -0700
> Andy Lutomirski <luto@amacapital.net> wrote:
>
> > On 10/07/2014 07:39 AM, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > >
> > > git://github.com/cohuck/qemu virtio-1
> > >
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> >
> > At the risk of some distraction, would it be worth thinking about a
> > solution to the IOMMU bypassing mess as part of this?
>
> I think that is a whole different issue. virtio-1 is basically done - we
> just need to implement it - while the IOMMU/DMA stuff certainly needs
> more discussion. Therefore, I'd like to defer to the other discussion
> thread here.
I agree, let's do a separate thread for this.
I also think it's up to the hypervisors at this point.
People talked about using ACPI to report IOMMU bypass
to guest.
If that happens, we don't need a feature bit.
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-22 8:44 ` Michael S. Tsirkin
@ 2014-10-22 14:17 ` Jan Kiszka
2014-10-22 14:36 ` Michael S. Tsirkin
2014-10-22 20:34 ` Benjamin Herrenschmidt
0 siblings, 2 replies; 36+ messages in thread
From: Jan Kiszka @ 2014-10-22 14:17 UTC (permalink / raw)
To: Michael S. Tsirkin, Cornelia Huck
Cc: thuth, kvm, qemu-devel, virtualization, Andy Lutomirski
On 2014-10-22 10:44, Michael S. Tsirkin wrote:
> On Wed, Oct 08, 2014 at 11:04:28AM +0200, Cornelia Huck wrote:
>> On Tue, 07 Oct 2014 18:24:22 -0700
>> Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>> On 10/07/2014 07:39 AM, Cornelia Huck wrote:
>>>> This patchset aims to get us some way to implement virtio-1 compliant
>>>> and transitional devices in qemu. Branch available at
>>>>
>>>> git://github.com/cohuck/qemu virtio-1
>>>>
>>>> I've mainly focused on:
>>>> - endianness handling
>>>> - extended feature bits
>>>> - virtio-ccw new/changed commands
>>>
>>> At the risk of some distraction, would it be worth thinking about a
>>> solution to the IOMMU bypassing mess as part of this?
>>
>> I think that is a whole different issue. virtio-1 is basically done - we
>> just need to implement it - while the IOMMU/DMA stuff certainly needs
>> more discussion. Therefore, I'd like to defer to the other discussion
>> thread here.
>
> I agree, let's do a separate thread for this.
> I also think it's up to the hypervisors at this point.
> People talked about using ACPI to report IOMMU bypass
> to guest.
> If that happens, we don't need a feature bit.
I thought about this again, and I'm not sure anymore if we can use ACPI
to "black-list" the incompatible virtio devices. Reason: hotplug. To my
understanding, the ACPI DRHD tables won't change during runtime when a
device shows up or disappears. We would have to isolate virtio devices
from the rest of the system by using separate buses for it (and avoid
listing those in any DRHD table) and enforce that they only get plugged
into those buses. I suppose that is not desirable.
Maybe it's better to fix virtio /wrt IOMMUs.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-22 14:17 ` Jan Kiszka
@ 2014-10-22 14:36 ` Michael S. Tsirkin
2014-10-22 20:34 ` Benjamin Herrenschmidt
1 sibling, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-22 14:36 UTC (permalink / raw)
To: Jan Kiszka
Cc: thuth, kvm, qemu-devel, Andy Lutomirski, Cornelia Huck,
virtualization
On Wed, Oct 22, 2014 at 04:17:40PM +0200, Jan Kiszka wrote:
> On 2014-10-22 10:44, Michael S. Tsirkin wrote:
> > On Wed, Oct 08, 2014 at 11:04:28AM +0200, Cornelia Huck wrote:
> >> On Tue, 07 Oct 2014 18:24:22 -0700
> >> Andy Lutomirski <luto@amacapital.net> wrote:
> >>
> >>> On 10/07/2014 07:39 AM, Cornelia Huck wrote:
> >>>> This patchset aims to get us some way to implement virtio-1 compliant
> >>>> and transitional devices in qemu. Branch available at
> >>>>
> >>>> git://github.com/cohuck/qemu virtio-1
> >>>>
> >>>> I've mainly focused on:
> >>>> - endianness handling
> >>>> - extended feature bits
> >>>> - virtio-ccw new/changed commands
> >>>
> >>> At the risk of some distraction, would it be worth thinking about a
> >>> solution to the IOMMU bypassing mess as part of this?
> >>
> >> I think that is a whole different issue. virtio-1 is basically done - we
> >> just need to implement it - while the IOMMU/DMA stuff certainly needs
> >> more discussion. Therefore, I'd like to defer to the other discussion
> >> thread here.
> >
> > I agree, let's do a separate thread for this.
> > I also think it's up to the hypervisors at this point.
> > People talked about using ACPI to report IOMMU bypass
> > to guest.
> > If that happens, we don't need a feature bit.
>
> I thought about this again, and I'm not sure anymore if we can use ACPI
> to "black-list" the incompatible virtio devices. Reason: hotplug. To my
> understanding, the ACPI DRHD tables won't change during runtime when a
> device shows up or disappears. We would have to isolate virtio devices
> from the rest of the system by using separate buses for it (and avoid
> listing those in any DRHD table) and enforce that they only get plugged
> into those buses. I suppose that is not desirable.
That's reasonable I think.
> Maybe it's better to fix virtio /wrt IOMMUs.
>
> Jan
Yes but this seems unlikely for 2.2:
The wish to run old guests with iommu remains.
So we'll need to support iommu bypass on the host, and so as
a minimum new guest needs to detect such bypass host and fail.
Unrelated: we also need to teach vhost and dataplane virtio
to get mappings from the iommu.
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-22 14:17 ` Jan Kiszka
2014-10-22 14:36 ` Michael S. Tsirkin
@ 2014-10-22 20:34 ` Benjamin Herrenschmidt
2014-10-23 6:44 ` Jan Kiszka
2014-10-23 7:12 ` Michael S. Tsirkin
1 sibling, 2 replies; 36+ messages in thread
From: Benjamin Herrenschmidt @ 2014-10-22 20:34 UTC (permalink / raw)
To: Jan Kiszka
Cc: thuth, kvm, Michael S. Tsirkin, qemu-devel, Andy Lutomirski,
Cornelia Huck, virtualization
On Wed, 2014-10-22 at 16:17 +0200, Jan Kiszka wrote:
> I thought about this again, and I'm not sure anymore if we can use
> ACPI
> to "black-list" the incompatible virtio devices. Reason: hotplug. To
> my
> understanding, the ACPI DRHD tables won't change during runtime when a
> device shows up or disappears. We would have to isolate virtio devices
> from the rest of the system by using separate buses for it (and avoid
> listing those in any DRHD table) and enforce that they only get
> plugged
> into those buses. I suppose that is not desirable.
>
> Maybe it's better to fix virtio /wrt IOMMUs.
I always go back to my initial proposal which is to define that current
virtio always bypass any iommu (which is what it does really) and have
it expose via a new capability if that isn't the case. That means fixing
that Xen thingy to allow qemu to know what to expose I assume but that
seems to be the less bad approach.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-22 20:34 ` Benjamin Herrenschmidt
@ 2014-10-23 6:44 ` Jan Kiszka
2014-10-23 9:18 ` Michael S. Tsirkin
2014-10-23 7:12 ` Michael S. Tsirkin
1 sibling, 1 reply; 36+ messages in thread
From: Jan Kiszka @ 2014-10-23 6:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: thuth, kvm, Michael S. Tsirkin, qemu-devel, Andy Lutomirski,
Cornelia Huck, virtualization
On 2014-10-22 22:34, Benjamin Herrenschmidt wrote:
> On Wed, 2014-10-22 at 16:17 +0200, Jan Kiszka wrote:
>> I thought about this again, and I'm not sure anymore if we can use
>> ACPI
>> to "black-list" the incompatible virtio devices. Reason: hotplug. To
>> my
>> understanding, the ACPI DRHD tables won't change during runtime when a
>> device shows up or disappears. We would have to isolate virtio devices
>> from the rest of the system by using separate buses for it (and avoid
>> listing those in any DRHD table) and enforce that they only get
>> plugged
>> into those buses. I suppose that is not desirable.
>>
>> Maybe it's better to fix virtio /wrt IOMMUs.
>
> I always go back to my initial proposal which is to define that current
> virtio always bypass any iommu (which is what it does really) and have
> it expose via a new capability if that isn't the case. That means fixing
> that Xen thingy to allow qemu to know what to expose I assume but that
> seems to be the less bad approach.
Just one thing to consider: feature negotiation happens after guest
startup. If we run a virtio device under IOMMU control, what will we
have to do when the guest says it does not support such devices? Simply
reject operation?
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-23 6:44 ` Jan Kiszka
@ 2014-10-23 9:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-23 9:18 UTC (permalink / raw)
To: Jan Kiszka
Cc: thuth, kvm, qemu-devel, Andy Lutomirski, kraxel, Cornelia Huck,
virtualization
On Thu, Oct 23, 2014 at 08:44:09AM +0200, Jan Kiszka wrote:
> On 2014-10-22 22:34, Benjamin Herrenschmidt wrote:
> > On Wed, 2014-10-22 at 16:17 +0200, Jan Kiszka wrote:
> >> I thought about this again, and I'm not sure anymore if we can use
> >> ACPI
> >> to "black-list" the incompatible virtio devices. Reason: hotplug. To
> >> my
> >> understanding, the ACPI DRHD tables won't change during runtime when a
> >> device shows up or disappears. We would have to isolate virtio devices
> >> from the rest of the system by using separate buses for it (and avoid
> >> listing those in any DRHD table) and enforce that they only get
> >> plugged
> >> into those buses. I suppose that is not desirable.
> >>
> >> Maybe it's better to fix virtio /wrt IOMMUs.
> >
> > I always go back to my initial proposal which is to define that current
> > virtio always bypass any iommu (which is what it does really) and have
> > it expose via a new capability if that isn't the case. That means fixing
> > that Xen thingy to allow qemu to know what to expose I assume but that
> > seems to be the less bad approach.
>
> Just one thing to consider: feature negotiation happens after guest
> startup. If we run a virtio device under IOMMU control, what will we
> have to do when the guest says it does not support such devices? Simply
> reject operation?
>
> Jan
We could restrict this to the new set of device IDs that
got introduced in virtio 1.0.
That actually has a mechanism for devices to gracefully
reject a combination of features if we still need it.
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-22 20:34 ` Benjamin Herrenschmidt
2014-10-23 6:44 ` Jan Kiszka
@ 2014-10-23 7:12 ` Michael S. Tsirkin
1 sibling, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-23 7:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: thuth, kvm, Rusty Russell, Jan Kiszka, qemu-devel,
Andy Lutomirski, Cornelia Huck, virtualization
On Thu, Oct 23, 2014 at 07:34:16AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2014-10-22 at 16:17 +0200, Jan Kiszka wrote:
> > I thought about this again, and I'm not sure anymore if we can use
> > ACPI
> > to "black-list" the incompatible virtio devices. Reason: hotplug. To
> > my
> > understanding, the ACPI DRHD tables won't change during runtime when a
> > device shows up or disappears. We would have to isolate virtio devices
> > from the rest of the system by using separate buses for it (and avoid
> > listing those in any DRHD table) and enforce that they only get
> > plugged
> > into those buses. I suppose that is not desirable.
> >
> > Maybe it's better to fix virtio /wrt IOMMUs.
>
> I always go back to my initial proposal which is to define that current
> virtio always bypass any iommu (which is what it does really) and have
> it expose via a new capability if that isn't the case. That means fixing
> that Xen thingy to allow qemu to know what to expose I assume but that
> seems to be the less bad approach.
>
> Cheers,
> Ben.
>
OK so how does this work?
If you want to run an existing guest, you use the old device.
And presumably you blacklist virtio for nested virt then,
unless a new capability is present?
--
MST
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-07 14:39 [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Cornelia Huck
` (11 preceding siblings ...)
2014-10-08 1:24 ` [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support Andy Lutomirski
@ 2014-10-23 21:42 ` Michael S. Tsirkin
2014-10-24 8:38 ` Cornelia Huck
12 siblings, 1 reply; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-23 21:42 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, qemu-devel, kvm, virtualization
On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> This patchset aims to get us some way to implement virtio-1 compliant
> and transitional devices in qemu. Branch available at
>
> git://github.com/cohuck/qemu virtio-1
>
> I've mainly focused on:
> - endianness handling
> - extended feature bits
> - virtio-ccw new/changed commands
So issues identified so far:
- devices not converted yet should not advertize 1.0
- blk has some features removed under 1.0,
specifically CONFIG_WCE.
- net header is different for 1.0
> Thanks go to Thomas for some preliminary work in this area.
>
> I've been able to start guests both with and without the virtio-1 patches
> in the linux guest patchset, with virtio-net and virtio-blk devices (with
> and without dataplane). virtio-ccw only :) vhost, migration and loads of
> other things have been ignored for now.
>
> I'd like to know whether I walk into the right direction, so let's consider
> this as a starting point.
>
> Cornelia Huck (8):
> virtio: cull virtio_bus_set_vdev_features
> virtio: support more feature bits
> s390x/virtio-ccw: fix check for WRITE_FEAT
> virtio: introduce legacy virtio devices
> virtio: allow virtio-1 queue layout
> dataplane: allow virtio-1 devices
> s390x/virtio-ccw: support virtio-1 set_vq format
> s390x/virtio-ccw: enable virtio 1.0
>
> Thomas Huth (3):
> linux-headers/virtio_config: Update with VIRTIO_F_VERSION_1
> s390x/css: Add a callback for when subchannel gets disabled
> s390x/virtio-ccw: add virtio set-revision call
>
> hw/9pfs/virtio-9p-device.c | 7 +-
> hw/block/dataplane/virtio-blk.c | 3 +-
> hw/block/virtio-blk.c | 9 +-
> hw/char/virtio-serial-bus.c | 9 +-
> hw/net/virtio-net.c | 38 ++++---
> hw/s390x/css.c | 12 +++
> hw/s390x/css.h | 1 +
> hw/s390x/s390-virtio-bus.c | 9 +-
> hw/s390x/virtio-ccw.c | 188 +++++++++++++++++++++++++++--------
> hw/s390x/virtio-ccw.h | 7 +-
> hw/scsi/vhost-scsi.c | 7 +-
> hw/scsi/virtio-scsi-dataplane.c | 2 +-
> hw/scsi/virtio-scsi.c | 8 +-
> hw/virtio/Makefile.objs | 2 +-
> hw/virtio/dataplane/Makefile.objs | 2 +-
> hw/virtio/dataplane/vring.c | 95 ++++++++++--------
> hw/virtio/virtio-balloon.c | 8 +-
> hw/virtio/virtio-bus.c | 23 +----
> hw/virtio/virtio-mmio.c | 9 +-
> hw/virtio/virtio-pci.c | 13 +--
> hw/virtio/virtio-rng.c | 2 +-
> hw/virtio/virtio.c | 51 +++++++---
> include/hw/virtio/dataplane/vring.h | 64 +++++++++++-
> include/hw/virtio/virtio-access.h | 4 +
> include/hw/virtio/virtio-bus.h | 10 +-
> include/hw/virtio/virtio.h | 34 +++++--
> linux-headers/linux/virtio_config.h | 3 +
> 27 files changed, 442 insertions(+), 178 deletions(-)
>
> --
> 1.7.9.5
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-23 21:42 ` Michael S. Tsirkin
@ 2014-10-24 8:38 ` Cornelia Huck
2014-10-24 12:37 ` Cornelia Huck
` (2 more replies)
0 siblings, 3 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-24 8:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: thuth, qemu-devel, kvm, virtualization
On Fri, 24 Oct 2014 00:42:20 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > This patchset aims to get us some way to implement virtio-1 compliant
> > and transitional devices in qemu. Branch available at
> >
> > git://github.com/cohuck/qemu virtio-1
> >
> > I've mainly focused on:
> > - endianness handling
> > - extended feature bits
> > - virtio-ccw new/changed commands
>
> So issues identified so far:
Thanks for taking a look.
> - devices not converted yet should not advertize 1.0
Neither should an uncoverted transport. So we either can
- have transport set the bit and rely on devices ->get_features
callback to mask it out
(virtio-ccw has to change the calling order for get_features, btw.)
- have device set the bit and the transport mask it out later. Feels a
bit weird, as virtio-1 is a transport feature bit.
I'm tending towards the first option; smth like this (on top of my
branch):
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index c29c8c8..f6501ea 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -24,6 +24,9 @@
static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
uint32_t features)
{
+ if (index == 1) {
+ features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
if (index > 0) {
return features;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 0d843fe..07a7a6f 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
{
VirtIOSerial *vser;
+ if (index == 1) {
+ features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
if (index > 0) {
return features;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 67f91c0..4b75105 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
VirtIONet *n = VIRTIO_NET(vdev);
NetClientState *nc = qemu_get_queue(n->nic);
+ if (index == 1 && get_vhost_net(nc->peer)) {
+ features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
if (index > 0) {
return features;
}
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 80efe88..07fbf40 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
dev->revision = -1;
/* Set default feature bits that are offered by the host. */
+ dev->host_features[0] = 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
+ dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
+
+ dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
+
for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
dev->host_features[i] =
virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
}
- dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
- dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
-
- dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
-
css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
parent->hotplugged, 1);
return 0;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 8e1afa0..8a8fdb9 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
{
VHostSCSI *s = VHOST_SCSI(vdev);
+ if (index == 1) {
+ features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
if (index > 0) {
return features;
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 088e688..378783f 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
uint32_t requested_features)
{
+ if (index == 1) {
+ requested_features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
return requested_features;
}
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5af17e2..bd52845 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
unsigned int index, uint32_t f)
{
+ if (index == 1) {
+ f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
if (index > 0) {
return f;
}
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 2814017..bf6101d 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
{
+ if (index == 1) {
+ f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
+ }
return f;
}
> - blk has some features removed under 1.0,
> specifically CONFIG_WCE.
> - net header is different for 1.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-24 8:38 ` Cornelia Huck
@ 2014-10-24 12:37 ` Cornelia Huck
2014-10-24 14:05 ` Michael S. Tsirkin
2014-10-24 14:17 ` Michael S. Tsirkin
2014-10-28 4:43 ` Michael S. Tsirkin
2 siblings, 1 reply; 36+ messages in thread
From: Cornelia Huck @ 2014-10-24 12:37 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: thuth, qemu-devel, kvm, virtualization
On Fri, 24 Oct 2014 10:38:39 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > >
> > > git://github.com/cohuck/qemu virtio-1
> > >
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> >
> > So issues identified so far:
>
> Thanks for taking a look.
>
> > - devices not converted yet should not advertize 1.0
>
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
> callback to mask it out
> (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
> bit weird, as virtio-1 is a transport feature bit.
>
> I'm tending towards the first option; smth like this (on top of my
> branch):
(...)
OK, I played around with this patch on top and the vhost-next branch as
guest. It seems to work reasonably well so far: a virtio-blk device
used virtio-1, a virtio-balloon device legacy.
One thing I noticed, though, is that I may need to think about
virtio-ccw revision vs. virtio version again. As a device can refuse
virtio-1 after the driver negotiated revision 1, we're operating a
legacy device with (some) standard ccws. Probably not a big deal, as
(a) both driver and device already have indicated that they support
revision 1 which those ccws are tied to and (b) some legacy
devices/drivers already support standard ccws (adapter interrupt
support). I might want to clarify the standard a bit, let me think
about that over the weekend.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-24 12:37 ` Cornelia Huck
@ 2014-10-24 14:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-24 14:05 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, qemu-devel, kvm, virtualization
On Fri, Oct 24, 2014 at 02:37:08PM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 10:38:39 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > On Fri, 24 Oct 2014 00:42:20 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > > This patchset aims to get us some way to implement virtio-1 compliant
> > > > and transitional devices in qemu. Branch available at
> > > >
> > > > git://github.com/cohuck/qemu virtio-1
> > > >
> > > > I've mainly focused on:
> > > > - endianness handling
> > > > - extended feature bits
> > > > - virtio-ccw new/changed commands
> > >
> > > So issues identified so far:
> >
> > Thanks for taking a look.
> >
> > > - devices not converted yet should not advertize 1.0
> >
> > Neither should an uncoverted transport. So we either can
> > - have transport set the bit and rely on devices ->get_features
> > callback to mask it out
> > (virtio-ccw has to change the calling order for get_features, btw.)
> > - have device set the bit and the transport mask it out later. Feels a
> > bit weird, as virtio-1 is a transport feature bit.
> >
> > I'm tending towards the first option; smth like this (on top of my
> > branch):
>
> (...)
>
> OK, I played around with this patch on top and the vhost-next branch as
> guest. It seems to work reasonably well so far: a virtio-blk device
> used virtio-1, a virtio-balloon device legacy.
>
> One thing I noticed, though, is that I may need to think about
> virtio-ccw revision vs. virtio version again. As a device can refuse
> virtio-1 after the driver negotiated revision 1, we're operating a
> legacy device with (some) standard ccws. Probably not a big deal, as
> (a) both driver and device already have indicated that they support
> revision 1 which those ccws are tied to and (b) some legacy
> devices/drivers already support standard ccws (adapter interrupt
> support). I might want to clarify the standard a bit, let me think
> about that over the weekend.
Thanks!
Please note I have updated the branch several times - I'm not going to
send patches until next week. I updated vhost net by now.
Just for the kernel, host side, we need to update tun, macvtap,
af_packet, vhost scsi and vhost test.
Guest side, we need to keep going over devices and convert them one by
one.
And of course qemu, including support for old and new tun/macvtap.
--
MST
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-24 8:38 ` Cornelia Huck
2014-10-24 12:37 ` Cornelia Huck
@ 2014-10-24 14:17 ` Michael S. Tsirkin
2014-10-28 4:43 ` Michael S. Tsirkin
2 siblings, 0 replies; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-24 14:17 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, qemu-devel, kvm, virtualization
On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > >
> > > git://github.com/cohuck/qemu virtio-1
> > >
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> >
> > So issues identified so far:
>
> Thanks for taking a look.
>
> > - devices not converted yet should not advertize 1.0
>
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
> callback to mask it out
> (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
> bit weird, as virtio-1 is a transport feature bit.
>
> I'm tending towards the first option; smth like this (on top of my
> branch):
I would rather *converted* devices had extra code
than unconverted ones.
Reduces the chance we forget one by mistake.
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index c29c8c8..f6501ea 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -24,6 +24,9 @@
> static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
> uint32_t features)
> {
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 0d843fe..07a7a6f 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
> {
> VirtIOSerial *vser;
>
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 67f91c0..4b75105 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
> VirtIONet *n = VIRTIO_NET(vdev);
> NetClientState *nc = qemu_get_queue(n->nic);
>
> + if (index == 1 && get_vhost_net(nc->peer)) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 80efe88..07fbf40 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> dev->revision = -1;
>
> /* Set default feature bits that are offered by the host. */
> + dev->host_features[0] = 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> + dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> +
> + dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> +
> for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
> dev->host_features[i] =
> virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
> }
>
> - dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> - dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> -
> - dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> -
> css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> parent->hotplugged, 1);
> return 0;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 8e1afa0..8a8fdb9 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
> {
> VHostSCSI *s = VHOST_SCSI(vdev);
>
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 088e688..378783f 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
> static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
> uint32_t requested_features)
> {
> + if (index == 1) {
> + requested_features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> return requested_features;
> }
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af17e2..bd52845 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
> unsigned int index, uint32_t f)
> {
> + if (index == 1) {
> + f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return f;
> }
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 2814017..bf6101d 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>
> static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
> {
> + if (index == 1) {
> + f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> return f;
> }
>
>
>
> > - blk has some features removed under 1.0,
> > specifically CONFIG_WCE.
> > - net header is different for 1.0
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-24 8:38 ` Cornelia Huck
2014-10-24 12:37 ` Cornelia Huck
2014-10-24 14:17 ` Michael S. Tsirkin
@ 2014-10-28 4:43 ` Michael S. Tsirkin
2014-10-30 16:52 ` Cornelia Huck
2 siblings, 1 reply; 36+ messages in thread
From: Michael S. Tsirkin @ 2014-10-28 4:43 UTC (permalink / raw)
To: Cornelia Huck; +Cc: thuth, qemu-devel, kvm, virtualization
On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > >
> > > git://github.com/cohuck/qemu virtio-1
> > >
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> >
> > So issues identified so far:
>
> Thanks for taking a look.
>
> > - devices not converted yet should not advertize 1.0
>
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
> callback to mask it out
> (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
> bit weird, as virtio-1 is a transport feature bit.
I thought more about it, I think the right thing
would be for unconverted transports to clear
high bits on ack and get features.
So bit 32 is set, but not exposed to guests.
In fact at least for PCI, we have a 32 bit field for
features in 0.9 so it's automatic.
Didn't check mmio yet.
> I'm tending towards the first option; smth like this (on top of my
> branch):
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index c29c8c8..f6501ea 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -24,6 +24,9 @@
> static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
> uint32_t features)
> {
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 0d843fe..07a7a6f 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
> {
> VirtIOSerial *vser;
>
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 67f91c0..4b75105 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
> VirtIONet *n = VIRTIO_NET(vdev);
> NetClientState *nc = qemu_get_queue(n->nic);
>
> + if (index == 1 && get_vhost_net(nc->peer)) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 80efe88..07fbf40 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
> dev->revision = -1;
>
> /* Set default feature bits that are offered by the host. */
> + dev->host_features[0] = 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> + dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> +
> + dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> +
> for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
> dev->host_features[i] =
> virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
> }
>
> - dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> - dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> -
> - dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> -
> css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> parent->hotplugged, 1);
> return 0;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 8e1afa0..8a8fdb9 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
> {
> VHostSCSI *s = VHOST_SCSI(vdev);
>
> + if (index == 1) {
> + features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return features;
> }
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 088e688..378783f 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
> static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
> uint32_t requested_features)
> {
> + if (index == 1) {
> + requested_features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> return requested_features;
> }
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af17e2..bd52845 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
> unsigned int index, uint32_t f)
> {
> + if (index == 1) {
> + f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> if (index > 0) {
> return f;
> }
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 2814017..bf6101d 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>
> static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
> {
> + if (index == 1) {
> + f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> + }
> return f;
> }
>
>
>
> > - blk has some features removed under 1.0,
> > specifically CONFIG_WCE.
> > - net header is different for 1.0
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH RFC 00/11] qemu: towards virtio-1 host support
2014-10-28 4:43 ` Michael S. Tsirkin
@ 2014-10-30 16:52 ` Cornelia Huck
0 siblings, 0 replies; 36+ messages in thread
From: Cornelia Huck @ 2014-10-30 16:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: thuth, qemu-devel, kvm, virtualization
On Tue, 28 Oct 2014 06:43:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> > On Fri, 24 Oct 2014 00:42:20 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > > This patchset aims to get us some way to implement virtio-1 compliant
> > > > and transitional devices in qemu. Branch available at
> > > >
> > > > git://github.com/cohuck/qemu virtio-1
> > > >
> > > > I've mainly focused on:
> > > > - endianness handling
> > > > - extended feature bits
> > > > - virtio-ccw new/changed commands
> > >
> > > So issues identified so far:
> >
> > Thanks for taking a look.
> >
> > > - devices not converted yet should not advertize 1.0
> >
> > Neither should an uncoverted transport. So we either can
> > - have transport set the bit and rely on devices ->get_features
> > callback to mask it out
> > (virtio-ccw has to change the calling order for get_features, btw.)
> > - have device set the bit and the transport mask it out later. Feels a
> > bit weird, as virtio-1 is a transport feature bit.
>
>
> I thought more about it, I think the right thing
> would be for unconverted transports to clear
> high bits on ack and get features.
This should work out of the box with my patches (virtio-pci and
virtio-mmio return 0 for high feature bits).
>
> So bit 32 is set, but not exposed to guests.
> In fact at least for PCI, we have a 32 bit field for
> features in 0.9 so it's automatic.
> Didn't check mmio yet.
We still to make sure the bit is not set for unconverted devices,
though. But you're probably right that having the device set the bit is
less error-prone.
^ permalink raw reply [flat|nested] 36+ messages in thread