[PATCH kvmtool 0/3] Virtio cleanup

[PATCH kvmtool 0/3] Virtio cleanup

[Jean-Philippe Brucker]

These patches attempt to make kvmtool more compliant with what virtio
1.0 calls the "legacy device". Issues were reported when trying to run
SeaBIOS under kvmtool. I briefly searched for more discrepancy with
sections 2-4 of the spec, but couldn't find any. Except for the lack of
reset support that is, on which I'd like to work in the background.

Jean-Philippe Brucker (3):
  virtio: Save negotiated features
  virtio: Support drivers that don't negotiate VIRTIO_RING_F_EVENT_IDX
  virtio/pci: Use port I/O for configuration registers by default

 include/kvm/virtio.h | 14 ++++++++++++--
 virtio/core.c        | 17 +++++++++++++++++
 virtio/mmio.c        |  4 ++--
 virtio/pci.c         |  8 ++++----
 4 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.14.3

[PATCH kvmtool 1/3] virtio: Save negotiated features

[Jean-Philippe Brucker]

We're going to need the features bits negotiated between host and guest in
the core code. Save them in the virtio_device structure.

Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 include/kvm/virtio.h | 4 ++++
 virtio/core.c        | 9 +++++++++
 virtio/mmio.c        | 4 ++--
 virtio/pci.c         | 2 +-
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 00a791acf0f8..69856d9ef4e7 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -141,6 +141,7 @@ struct virtio_device {
 	void			*virtio;
 	struct virtio_ops	*ops;
 	u16			endian;
+	u32			features;
 };
 
 struct virtio_ops {
@@ -180,4 +181,7 @@ static inline void virtio_init_device_vq(struct virtio_device *vdev,
 	vq->endian = vdev->endian;
 }
 
+void virtio_set_guest_features(struct kvm *kvm, struct virtio_device *vdev,
+			       void *dev, u32 features);
+
 #endif /* KVM__VIRTIO_H */
diff --git a/virtio/core.c b/virtio/core.c
index d6ac289d450e..e98241f04fcd 100644
--- a/virtio/core.c
+++ b/virtio/core.c
@@ -198,6 +198,15 @@ bool virtio_queue__should_signal(struct virt_queue *vq)
 	return false;
 }
 
+void virtio_set_guest_features(struct kvm *kvm, struct virtio_device *vdev,
+			       void *dev, u32 features)
+{
+	/* TODO: fail negotiation if features & ~host_features */
+
+	vdev->features = features;
+	vdev->ops->set_guest_features(kvm, dev, features);
+}
+
 int virtio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 		struct virtio_ops *ops, enum virtio_trans trans,
 		int device_id, int subsys_id, int class)
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 005257fcb8f1..629127528bf0 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -159,8 +159,8 @@ static void virtio_mmio_config_out(struct kvm_cpu *vcpu,
 	case VIRTIO_MMIO_GUEST_FEATURES:
 		if (vmmio->hdr.guest_features_sel == 0) {
 			val = ioport__read32(data);
-			vdev->ops->set_guest_features(vmmio->kvm,
-						      vmmio->dev, val);
+			virtio_set_guest_features(vmmio->kvm, vdev,
+						  vmmio->dev, val);
 		}
 		break;
 	case VIRTIO_MMIO_GUEST_PAGE_SIZE:
diff --git a/virtio/pci.c b/virtio/pci.c
index 4ce111108100..2054c7bac472 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -267,7 +267,7 @@ static bool virtio_pci__io_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16
 	switch (offset) {
 	case VIRTIO_PCI_GUEST_FEATURES:
 		val = ioport__read32(data);
-		vdev->ops->set_guest_features(kvm, vpci->dev, val);
+		virtio_set_guest_features(kvm, vdev, vpci->dev, val);
 		break;
 	case VIRTIO_PCI_QUEUE_PFN:
 		val = ioport__read32(data);
-- 
2.14.3

[PATCH kvmtool 2/3] virtio: Support drivers that don't negotiate VIRTIO_RING_F_EVENT_IDX

[Jean-Philippe Brucker]

Bad things happen when the VIRTIO_RING_F_EVENT_IDX feature isn't
negotiated and we try to write the avail_event anyway. SeaBIOS, for
example, stores internal data where avail_event should be [1].

Technically the Virtio specification doesn't forbid the device from
writing the avail_event, and it's up to the driver to reserve space for it
("the transitional driver [...] MUST allocate the total number of bytes
for the virtqueue according to [formula containing the avail event]").

But it doesn't hurt us to avoid writing avail_event, and kvmtool needs
changes for interrupt suppression anyway, in order to comply with the
spec. Indeed Virtio 1.0 cs04 says, in 2.4.7.2 Device Requirements:
Virtqueue Interrupt Suppression:
"""
If the VIRTIO_F_EVENT_IDX feature bit is not negotiated:
* The device MUST ignore the used_event value.
* After the device writes a descriptor index into the used ring:
  - If flags is 1, the device SHOULD NOT send an interrupt.
"""

So let's do that.

[1] https://patchwork.kernel.org/patch/10038931/

Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 include/kvm/virtio.h | 10 ++++++++--
 virtio/core.c        |  8 ++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 69856d9ef4e7..ab4028ca2e2f 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -34,6 +34,7 @@ struct virt_queue {
 	u16		last_avail_idx;
 	u16		last_used_signalled;
 	u16		endian;
+	bool		use_event_idx;
 };
 
 /*
@@ -110,11 +111,15 @@ static inline struct vring_desc *virt_queue__get_desc(struct virt_queue *queue,
 
 static inline bool virt_queue__available(struct virt_queue *vq)
 {
+	u16 last_avail_idx = virtio_host_to_guest_u16(vq, vq->last_avail_idx);
+
 	if (!vq->vring.avail)
 		return 0;
 
-	vring_avail_event(&vq->vring) = virtio_host_to_guest_u16(vq, vq->last_avail_idx);
-	return virtio_guest_to_host_u16(vq, vq->vring.avail->idx) != vq->last_avail_idx;
+	if (vq->use_event_idx)
+		vring_avail_event(&vq->vring) = last_avail_idx;
+
+	return vq->vring.avail->idx != last_avail_idx;
 }
 
 void virt_queue__used_idx_advance(struct virt_queue *queue, u16 jump);
@@ -179,6 +184,7 @@ static inline void virtio_init_device_vq(struct virtio_device *vdev,
 					 struct virt_queue *vq)
 {
 	vq->endian = vdev->endian;
+	vq->use_event_idx = (vdev->features & VIRTIO_RING_F_EVENT_IDX);
 }
 
 void virtio_set_guest_features(struct kvm *kvm, struct virtio_device *vdev,
diff --git a/virtio/core.c b/virtio/core.c
index e98241f04fcd..f4d531a27544 100644
--- a/virtio/core.c
+++ b/virtio/core.c
@@ -186,6 +186,14 @@ bool virtio_queue__should_signal(struct virt_queue *vq)
 {
 	u16 old_idx, new_idx, event_idx;
 
+	if (!vq->use_event_idx) {
+		/*
+		 * When VIRTIO_RING_F_EVENT_IDX isn't negotiated, interrupt the
+		 * guest if it didn't explicitly request to be left alone.
+		 */
+		return !(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT);
+	}
+
 	old_idx		= vq->last_used_signalled;
 	new_idx		= virtio_guest_to_host_u16(vq, vq->vring.used->idx);
 	event_idx	= virtio_guest_to_host_u16(vq, vring_used_event(&vq->vring));
-- 
2.14.3

[PATCH kvmtool 3/3] virtio/pci: Use port I/O for configuration registers by default

[Jean-Philippe Brucker]

Modern virtio PCI is allowed to use both memory and I/O BARs for the
config space, but legacy devices must use I/O for BAR0, as specified by
Virtio v1.0 cs04:

4.1.5.1.1.1 Legacy Interface: A Note on Device Layout Detection
"Transitional devices MUST expose the Legacy Interface in I/O space in
BAR0."

What virtio calls "I/O space" is most certainly port I/O, as hinted by
the discussion in 4.1.4 Virtio Structure PCI Capabilities, where it
distinguishes "memory BARs" from "I/O BARs". This is also the conclusion
made by SeaBIOS [1], which only looks for port I/O in BAR0 when driving
a legacy device.

I think MMIO was made the default by a463650caad6 ("kvm tools: pci: add
MMIO interface to virtio-pci devices") to support ARM targets, but we
support PIO as well as MMIO nowadays. So let's make the legacy virtio
implementation comply with the specification and use port I/O for BAR0.

[1] https://patchwork.kernel.org/patch/10038927/

Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 virtio/pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virtio/pci.c b/virtio/pci.c
index 2054c7bac472..9a199013ac49 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -450,10 +450,10 @@ int virtio_pci__init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 		.class[2]		= (class >> 16) & 0xff,
 		.subsys_vendor_id	= cpu_to_le16(PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET),
 		.subsys_id		= cpu_to_le16(subsys_id),
-		.bar[0]			= cpu_to_le32(vpci->mmio_addr
-							| PCI_BASE_ADDRESS_SPACE_MEMORY),
-		.bar[1]			= cpu_to_le32(vpci->port_addr
+		.bar[0]			= cpu_to_le32(vpci->port_addr
 							| PCI_BASE_ADDRESS_SPACE_IO),
+		.bar[1]			= cpu_to_le32(vpci->mmio_addr
+							| PCI_BASE_ADDRESS_SPACE_MEMORY),
 		.bar[2]			= cpu_to_le32(vpci->msix_io_block
 							| PCI_BASE_ADDRESS_SPACE_MEMORY),
 		.status			= cpu_to_le16(PCI_STATUS_CAP_LIST),
-- 
2.14.3

Re: [PATCH kvmtool 0/3] Virtio cleanup

[Gerd Hoffmann]

On Fri, Nov 10, 2017 at 07:19:56PM +0000, Jean-Philippe Brucker wrote:
> These patches attempt to make kvmtool more compliant with what virtio
> 1.0 calls the "legacy device". Issues were reported when trying to run
> SeaBIOS under kvmtool. I briefly searched for more discrepancy with
> sections 2-4 of the spec, but couldn't find any. Except for the lack of
> reset support that is, on which I'd like to work in the background.

Cool, can drop the workarounds from my seabios branch now.
As expected the linux kernel still doesn't find the virtio-blk device
(due to missing reset support), but it is one step forward nevertheless.

Tested-by: Gerd Hoffmann <kraxel@redhat.com>

thanks,
  Gerd