[PATCH v3 0/5] ARM: KVM: Enable the ioeventfd capability of KVM on ARM

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 0/5] ARM: KVM: Enable the ioeventfd capability of KVM on ARM
@ 2015-01-24 11:59 ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

The IOEVENTFD KVM capability is a prerequisite for vhost support.

This series enables the ioeventfd KVM capability on ARM.

The implementation routes MMIO access in the IO abort handler to the KVM IO bus.
If there is already a registered ioeventfd handler for this address, the file
descriptor will be triggered.

We extended the KVM IO bus API to expose the VCPU struct pointer. Now the VGIC
MMIO access is done through this API. For this to operate the VGIC registers a
kvm_io_device which represents the whole dist MMIO region.

The code was tested on Dual Cortex-A15 Exynos5250 (ARM Chromebook).
ARM64 build was verified, but not run on actual HW.

Changes since v2:
 - rebased on top of kvmarm/next
 - moved vgic_register_kvm_io_dev to kvm_vgic_map_resources

Changes since v1:
 - fixed x86 compilation
 - GICv2/GICv3 dist base selection
 - added vgic_unregister_kvm_io_dev to free the iodev resources
 - enable eventfd on ARM64

---

Nikolay Nikolaev (5):
      KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
      KVM: ARM: on IO mem abort - route the call to KVM MMIO bus
      KVM: ARM VGIC add kvm_io_bus_ frontend
      ARM/ARM64: enable linking against eventfd
      ARM: enable KVM_CAP_IOEVENTFD


 arch/arm/kvm/Kconfig       |    1 
 arch/arm/kvm/Makefile      |    2 -
 arch/arm/kvm/arm.c         |    3 +
 arch/arm/kvm/mmio.c        |   32 +++++++++++
 arch/arm64/kvm/Kconfig     |    1 
 arch/arm64/kvm/Makefile    |    2 -
 arch/powerpc/kvm/mpic.c    |   10 ++--
 arch/powerpc/kvm/powerpc.c |    4 +
 arch/s390/kvm/diag.c       |    2 -
 arch/x86/kvm/i8254.c       |   14 +++--
 arch/x86/kvm/i8259.c       |   12 ++--
 arch/x86/kvm/ioapic.c      |    8 +--
 arch/x86/kvm/lapic.c       |    4 +
 arch/x86/kvm/vmx.c         |    2 -
 arch/x86/kvm/x86.c         |   13 +++--
 include/kvm/arm_vgic.h     |    3 -
 include/linux/kvm_host.h   |   10 ++--
 virt/kvm/arm/vgic.c        |  123 ++++++++++++++++++++++++++++++++++++++++----
 virt/kvm/coalesced_mmio.c  |    5 +-
 virt/kvm/eventfd.c         |    4 +
 virt/kvm/iodev.h           |   23 +++++---
 virt/kvm/kvm_main.c        |   32 ++++++-----
 22 files changed, 231 insertions(+), 79 deletions(-)

--
Signature

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 0/5] ARM: KVM: Enable the ioeventfd capability of KVM on ARM
@ 2015-01-24 11:59 ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: linux-arm-kernel

The IOEVENTFD KVM capability is a prerequisite for vhost support.

This series enables the ioeventfd KVM capability on ARM.

The implementation routes MMIO access in the IO abort handler to the KVM IO bus.
If there is already a registered ioeventfd handler for this address, the file
descriptor will be triggered.

We extended the KVM IO bus API to expose the VCPU struct pointer. Now the VGIC
MMIO access is done through this API. For this to operate the VGIC registers a
kvm_io_device which represents the whole dist MMIO region.

The code was tested on Dual Cortex-A15 Exynos5250 (ARM Chromebook).
ARM64 build was verified, but not run on actual HW.

Changes since v2:
 - rebased on top of kvmarm/next
 - moved vgic_register_kvm_io_dev to kvm_vgic_map_resources

Changes since v1:
 - fixed x86 compilation
 - GICv2/GICv3 dist base selection
 - added vgic_unregister_kvm_io_dev to free the iodev resources
 - enable eventfd on ARM64

---

Nikolay Nikolaev (5):
      KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
      KVM: ARM: on IO mem abort - route the call to KVM MMIO bus
      KVM: ARM VGIC add kvm_io_bus_ frontend
      ARM/ARM64: enable linking against eventfd
      ARM: enable KVM_CAP_IOEVENTFD


 arch/arm/kvm/Kconfig       |    1 
 arch/arm/kvm/Makefile      |    2 -
 arch/arm/kvm/arm.c         |    3 +
 arch/arm/kvm/mmio.c        |   32 +++++++++++
 arch/arm64/kvm/Kconfig     |    1 
 arch/arm64/kvm/Makefile    |    2 -
 arch/powerpc/kvm/mpic.c    |   10 ++--
 arch/powerpc/kvm/powerpc.c |    4 +
 arch/s390/kvm/diag.c       |    2 -
 arch/x86/kvm/i8254.c       |   14 +++--
 arch/x86/kvm/i8259.c       |   12 ++--
 arch/x86/kvm/ioapic.c      |    8 +--
 arch/x86/kvm/lapic.c       |    4 +
 arch/x86/kvm/vmx.c         |    2 -
 arch/x86/kvm/x86.c         |   13 +++--
 include/kvm/arm_vgic.h     |    3 -
 include/linux/kvm_host.h   |   10 ++--
 virt/kvm/arm/vgic.c        |  123 ++++++++++++++++++++++++++++++++++++++++----
 virt/kvm/coalesced_mmio.c  |    5 +-
 virt/kvm/eventfd.c         |    4 +
 virt/kvm/iodev.h           |   23 +++++---
 virt/kvm/kvm_main.c        |   32 ++++++-----
 22 files changed, 231 insertions(+), 79 deletions(-)

--
Signature

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 1/5] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
  2015-01-24 11:59 ` Nikolay Nikolaev
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

This is needed in e.g. ARM vGIC emulation, where the MMIO handling
depends on the VCPU that does the access.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/powerpc/kvm/mpic.c    |   10 ++++++----
 arch/powerpc/kvm/powerpc.c |    4 ++--
 arch/s390/kvm/diag.c       |    2 +-
 arch/x86/kvm/i8254.c       |   14 +++++++++-----
 arch/x86/kvm/i8259.c       |   12 ++++++------
 arch/x86/kvm/ioapic.c      |    8 ++++----
 arch/x86/kvm/lapic.c       |    4 ++--
 arch/x86/kvm/vmx.c         |    2 +-
 arch/x86/kvm/x86.c         |   13 +++++++------
 include/linux/kvm_host.h   |   10 +++++-----
 virt/kvm/coalesced_mmio.c  |    5 +++--
 virt/kvm/eventfd.c         |    4 ++--
 virt/kvm/iodev.h           |   23 +++++++++++++++--------
 virt/kvm/kvm_main.c        |   32 ++++++++++++++++----------------
 14 files changed, 79 insertions(+), 64 deletions(-)

diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
index 39b3a8f..8542f07 100644
--- a/arch/powerpc/kvm/mpic.c
+++ b/arch/powerpc/kvm/mpic.c
@@ -1374,8 +1374,9 @@ static int kvm_mpic_write_internal(struct openpic *opp, gpa_t addr, u32 val)
 	return -ENXIO;
 }
 
-static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
-			 int len, void *ptr)
+static int kvm_mpic_read(struct kvm_vcpu *vcpu,
+			 struct kvm_io_device *this,
+			 gpa_t addr, int len, void *ptr)
 {
 	struct openpic *opp = container_of(this, struct openpic, mmio);
 	int ret;
@@ -1415,8 +1416,9 @@ static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
 	return ret;
 }
 
-static int kvm_mpic_write(struct kvm_io_device *this, gpa_t addr,
-			  int len, const void *ptr)
+static int kvm_mpic_write(struct kvm_vcpu *vcpu,
+			  struct kvm_io_device *this,
+			  gpa_t addr, int len, const void *ptr)
 {
 	struct openpic *opp = container_of(this, struct openpic, mmio);
 	int ret;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index c45eaab..0aac251 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -808,7 +808,7 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 
-	ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+	ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
 			      bytes, &run->mmio.data);
 
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
@@ -881,7 +881,7 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 
-	ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+	ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
 			       bytes, &run->mmio.data);
 
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 9254aff..329ec75 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -213,7 +213,7 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	 * - gpr 3 contains the virtqueue index (passed as datamatch)
 	 * - gpr 4 contains the index on the bus (optionally)
 	 */
-	ret = kvm_io_bus_write_cookie(vcpu->kvm, KVM_VIRTIO_CCW_NOTIFY_BUS,
+	ret = kvm_io_bus_write_cookie(vcpu, KVM_VIRTIO_CCW_NOTIFY_BUS,
 				      vcpu->run->s.regs.gprs[2] & 0xffffffff,
 				      8, &vcpu->run->s.regs.gprs[3],
 				      vcpu->run->s.regs.gprs[4]);
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 298781d..4dce6f8 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -443,7 +443,8 @@ static inline int pit_in_range(gpa_t addr)
 		(addr < KVM_PIT_BASE_ADDRESS + KVM_PIT_MEM_LENGTH));
 }
 
-static int pit_ioport_write(struct kvm_io_device *this,
+static int pit_ioport_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this,
 			    gpa_t addr, int len, const void *data)
 {
 	struct kvm_pit *pit = dev_to_pit(this);
@@ -519,7 +520,8 @@ static int pit_ioport_write(struct kvm_io_device *this,
 	return 0;
 }
 
-static int pit_ioport_read(struct kvm_io_device *this,
+static int pit_ioport_read(struct kvm_vcpu *vcpu,
+			   struct kvm_io_device *this,
 			   gpa_t addr, int len, void *data)
 {
 	struct kvm_pit *pit = dev_to_pit(this);
@@ -589,7 +591,8 @@ static int pit_ioport_read(struct kvm_io_device *this,
 	return 0;
 }
 
-static int speaker_ioport_write(struct kvm_io_device *this,
+static int speaker_ioport_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this,
 				gpa_t addr, int len, const void *data)
 {
 	struct kvm_pit *pit = speaker_to_pit(this);
@@ -606,8 +609,9 @@ static int speaker_ioport_write(struct kvm_io_device *this,
 	return 0;
 }
 
-static int speaker_ioport_read(struct kvm_io_device *this,
-			       gpa_t addr, int len, void *data)
+static int speaker_ioport_read(struct kvm_vcpu *vcpu,
+				   struct kvm_io_device *this,
+				   gpa_t addr, int len, void *data)
 {
 	struct kvm_pit *pit = speaker_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index cc31f7c..8ff4eaa 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -528,42 +528,42 @@ static int picdev_read(struct kvm_pic *s,
 	return 0;
 }
 
-static int picdev_master_write(struct kvm_io_device *dev,
+static int picdev_master_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			       gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_master),
 			    addr, len, val);
 }
 
-static int picdev_master_read(struct kvm_io_device *dev,
+static int picdev_master_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			      gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_master),
 			    addr, len, val);
 }
 
-static int picdev_slave_write(struct kvm_io_device *dev,
+static int picdev_slave_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			      gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_slave),
 			    addr, len, val);
 }
 
-static int picdev_slave_read(struct kvm_io_device *dev,
+static int picdev_slave_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			     gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_slave),
 			    addr, len, val);
 }
 
-static int picdev_eclr_write(struct kvm_io_device *dev,
+static int picdev_eclr_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			     gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_eclr),
 			    addr, len, val);
 }
 
-static int picdev_eclr_read(struct kvm_io_device *dev,
+static int picdev_eclr_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			    gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_eclr),
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index b1947e0..8bf2e49 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -498,8 +498,8 @@ static inline int ioapic_in_range(struct kvm_ioapic *ioapic, gpa_t addr)
 		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));
 }
 
-static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
-			    void *val)
+static int ioapic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+				gpa_t addr, int len, void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 result;
@@ -541,8 +541,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 	return 0;
 }
 
-static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
-			     const void *val)
+static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+				 gpa_t addr, int len, const void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 data;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index a688fbf..5e0ce37 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1043,7 +1043,7 @@ static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
 	    addr < apic->base_address + LAPIC_MMIO_LENGTH;
 }
 
-static int apic_mmio_read(struct kvm_io_device *this,
+static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
 			   gpa_t address, int len, void *data)
 {
 	struct kvm_lapic *apic = to_lapic(this);
@@ -1363,7 +1363,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	return ret;
 }
 
-static int apic_mmio_write(struct kvm_io_device *this,
+static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
 			    gpa_t address, int len, const void *data)
 {
 	struct kvm_lapic *apic = to_lapic(this);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ce35071..31f8d39 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5623,7 +5623,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
 	gpa_t gpa;
 
 	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
-	if (!kvm_io_bus_write(vcpu->kvm, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
+	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
 		skip_emulated_instruction(vcpu);
 		return 1;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 556dfb4..1f522b5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4097,8 +4097,8 @@ static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
 	do {
 		n = min(len, 8);
 		if (!(vcpu->arch.apic &&
-		      !kvm_iodevice_write(&vcpu->arch.apic->dev, addr, n, v))
-		    && kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
+		      !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev, addr, n, v))
+		    && kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, n, v))
 			break;
 		handled += n;
 		addr += n;
@@ -4117,8 +4117,9 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v)
 	do {
 		n = min(len, 8);
 		if (!(vcpu->arch.apic &&
-		      !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v))
-		    && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
+		      !kvm_iodevice_read(vcpu, &vcpu->arch.apic->dev,
+					 addr, n, v))
+		    && kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, n, v))
 			break;
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v);
 		handled += n;
@@ -4610,10 +4611,10 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 	int r;
 
 	if (vcpu->arch.pio.in)
-		r = kvm_io_bus_read(vcpu->kvm, KVM_PIO_BUS, vcpu->arch.pio.port,
+		r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
 				    vcpu->arch.pio.size, pd);
 	else
-		r = kvm_io_bus_write(vcpu->kvm, KVM_PIO_BUS,
+		r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
 				     vcpu->arch.pio.port, vcpu->arch.pio.size,
 				     pd);
 	return r;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0ef2daa..fddaf0d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -169,12 +169,12 @@ enum kvm_bus {
 	KVM_NR_BUSES
 };
 
-int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		     int len, const void *val);
-int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
-			    int len, const void *val, long cookie);
-int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int len,
-		    void *val);
+int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
+			    gpa_t addr, int len, const void *val, long cookie);
+int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
+		    int len, void *val);
 int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 			    int len, struct kvm_io_device *dev);
 int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 00d8642..c831a40 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -60,8 +60,9 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev)
 	return 1;
 }
 
-static int coalesced_mmio_write(struct kvm_io_device *this,
-				gpa_t addr, int len, const void *val)
+static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this, gpa_t addr,
+				int len, const void *val)
 {
 	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 148b239..99994c8 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -712,8 +712,8 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
 
 /* MMIO/PIO writes trigger an event if the addr/val match */
 static int
-ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
-		const void *val)
+ioeventfd_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
+		int len, const void *val)
 {
 	struct _ioeventfd *p = to_ioeventfd(this);
 
diff --git a/virt/kvm/iodev.h b/virt/kvm/iodev.h
index 12fd3ca..9ef709c 100644
--- a/virt/kvm/iodev.h
+++ b/virt/kvm/iodev.h
@@ -20,6 +20,7 @@
 #include <asm/errno.h>
 
 struct kvm_io_device;
+struct kvm_vcpu;
 
 /**
  * kvm_io_device_ops are called under kvm slots_lock.
@@ -27,11 +28,13 @@ struct kvm_io_device;
  * or non-zero to have it passed to the next device.
  **/
 struct kvm_io_device_ops {
-	int (*read)(struct kvm_io_device *this,
+	int (*read)(struct kvm_vcpu *vcpu,
+		    struct kvm_io_device *this,
 		    gpa_t addr,
 		    int len,
 		    void *val);
-	int (*write)(struct kvm_io_device *this,
+	int (*write)(struct kvm_vcpu *vcpu,
+		     struct kvm_io_device *this,
 		     gpa_t addr,
 		     int len,
 		     const void *val);
@@ -49,16 +52,20 @@ static inline void kvm_iodevice_init(struct kvm_io_device *dev,
 	dev->ops = ops;
 }
 
-static inline int kvm_iodevice_read(struct kvm_io_device *dev,
-				    gpa_t addr, int l, void *v)
+static inline int kvm_iodevice_read(struct kvm_vcpu *vcpu,
+				    struct kvm_io_device *dev, gpa_t addr,
+				    int l, void *v)
 {
-	return dev->ops->read ? dev->ops->read(dev, addr, l, v) : -EOPNOTSUPP;
+	return dev->ops->read ? dev->ops->read(vcpu, dev, addr, l, v)
+				: -EOPNOTSUPP;
 }
 
-static inline int kvm_iodevice_write(struct kvm_io_device *dev,
-				     gpa_t addr, int l, const void *v)
+static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu,
+				     struct kvm_io_device *dev, gpa_t addr,
+				     int l, const void *v)
 {
-	return dev->ops->write ? dev->ops->write(dev, addr, l, v) : -EOPNOTSUPP;
+	return dev->ops->write ? dev->ops->write(vcpu, dev, addr, l, v)
+				 : -EOPNOTSUPP;
 }
 
 static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 246cf29..60856d3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3004,7 +3004,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
 	return off;
 }
 
-static int __kvm_io_bus_write(struct kvm_io_bus *bus,
+static int __kvm_io_bus_write(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
 			      struct kvm_io_range *range, const void *val)
 {
 	int idx;
@@ -3015,7 +3015,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
 
 	while (idx < bus->dev_count &&
 		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
-		if (!kvm_iodevice_write(bus->range[idx].dev, range->addr,
+		if (!kvm_iodevice_write(vcpu, bus->range[idx].dev, range->addr,
 					range->len, val))
 			return idx;
 		idx++;
@@ -3025,7 +3025,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
 }
 
 /* kvm_io_bus_write - called under kvm->slots_lock */
-int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		     int len, const void *val)
 {
 	struct kvm_io_bus *bus;
@@ -3037,14 +3037,14 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
-	r = __kvm_io_bus_write(bus, &range, val);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
+	r = __kvm_io_bus_write(vcpu, bus, &range, val);
 	return r < 0 ? r : 0;
 }
 
 /* kvm_io_bus_write_cookie - called under kvm->slots_lock */
-int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
-			    int len, const void *val, long cookie)
+int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
+			    gpa_t addr, int len, const void *val, long cookie)
 {
 	struct kvm_io_bus *bus;
 	struct kvm_io_range range;
@@ -3054,12 +3054,12 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
 
 	/* First try the device referenced by cookie. */
 	if ((cookie >= 0) && (cookie < bus->dev_count) &&
 	    (kvm_io_bus_cmp(&range, &bus->range[cookie]) == 0))
-		if (!kvm_iodevice_write(bus->range[cookie].dev, addr, len,
+		if (!kvm_iodevice_write(vcpu, bus->range[cookie].dev, addr, len,
 					val))
 			return cookie;
 
@@ -3067,11 +3067,11 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 	 * cookie contained garbage; fall back to search and return the
 	 * correct cookie value.
 	 */
-	return __kvm_io_bus_write(bus, &range, val);
+	return __kvm_io_bus_write(vcpu, bus, &range, val);
 }
 
-static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
-			     void *val)
+static int __kvm_io_bus_read(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
+			     struct kvm_io_range *range, void *val)
 {
 	int idx;
 
@@ -3081,7 +3081,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
 
 	while (idx < bus->dev_count &&
 		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
-		if (!kvm_iodevice_read(bus->range[idx].dev, range->addr,
+		if (!kvm_iodevice_read(vcpu, bus->range[idx].dev, range->addr,
 				       range->len, val))
 			return idx;
 		idx++;
@@ -3092,7 +3092,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
 EXPORT_SYMBOL_GPL(kvm_io_bus_write);
 
 /* kvm_io_bus_read - called under kvm->slots_lock */
-int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		    int len, void *val)
 {
 	struct kvm_io_bus *bus;
@@ -3104,8 +3104,8 @@ int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
-	r = __kvm_io_bus_read(bus, &range, val);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
+	r = __kvm_io_bus_read(vcpu, bus, &range, val);
 	return r < 0 ? r : 0;
 }

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 1/5] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: linux-arm-kernel

This is needed in e.g. ARM vGIC emulation, where the MMIO handling
depends on the VCPU that does the access.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/powerpc/kvm/mpic.c    |   10 ++++++----
 arch/powerpc/kvm/powerpc.c |    4 ++--
 arch/s390/kvm/diag.c       |    2 +-
 arch/x86/kvm/i8254.c       |   14 +++++++++-----
 arch/x86/kvm/i8259.c       |   12 ++++++------
 arch/x86/kvm/ioapic.c      |    8 ++++----
 arch/x86/kvm/lapic.c       |    4 ++--
 arch/x86/kvm/vmx.c         |    2 +-
 arch/x86/kvm/x86.c         |   13 +++++++------
 include/linux/kvm_host.h   |   10 +++++-----
 virt/kvm/coalesced_mmio.c  |    5 +++--
 virt/kvm/eventfd.c         |    4 ++--
 virt/kvm/iodev.h           |   23 +++++++++++++++--------
 virt/kvm/kvm_main.c        |   32 ++++++++++++++++----------------
 14 files changed, 79 insertions(+), 64 deletions(-)

diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
index 39b3a8f..8542f07 100644
--- a/arch/powerpc/kvm/mpic.c
+++ b/arch/powerpc/kvm/mpic.c
@@ -1374,8 +1374,9 @@ static int kvm_mpic_write_internal(struct openpic *opp, gpa_t addr, u32 val)
 	return -ENXIO;
 }
 
-static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
-			 int len, void *ptr)
+static int kvm_mpic_read(struct kvm_vcpu *vcpu,
+			 struct kvm_io_device *this,
+			 gpa_t addr, int len, void *ptr)
 {
 	struct openpic *opp = container_of(this, struct openpic, mmio);
 	int ret;
@@ -1415,8 +1416,9 @@ static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
 	return ret;
 }
 
-static int kvm_mpic_write(struct kvm_io_device *this, gpa_t addr,
-			  int len, const void *ptr)
+static int kvm_mpic_write(struct kvm_vcpu *vcpu,
+			  struct kvm_io_device *this,
+			  gpa_t addr, int len, const void *ptr)
 {
 	struct openpic *opp = container_of(this, struct openpic, mmio);
 	int ret;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index c45eaab..0aac251 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -808,7 +808,7 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 
-	ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+	ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
 			      bytes, &run->mmio.data);
 
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
@@ -881,7 +881,7 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 
-	ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
+	ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
 			       bytes, &run->mmio.data);
 
 	srcu_read_unlock(&vcpu->kvm->srcu, idx);
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 9254aff..329ec75 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -213,7 +213,7 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
 	 * - gpr 3 contains the virtqueue index (passed as datamatch)
 	 * - gpr 4 contains the index on the bus (optionally)
 	 */
-	ret = kvm_io_bus_write_cookie(vcpu->kvm, KVM_VIRTIO_CCW_NOTIFY_BUS,
+	ret = kvm_io_bus_write_cookie(vcpu, KVM_VIRTIO_CCW_NOTIFY_BUS,
 				      vcpu->run->s.regs.gprs[2] & 0xffffffff,
 				      8, &vcpu->run->s.regs.gprs[3],
 				      vcpu->run->s.regs.gprs[4]);
diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index 298781d..4dce6f8 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -443,7 +443,8 @@ static inline int pit_in_range(gpa_t addr)
 		(addr < KVM_PIT_BASE_ADDRESS + KVM_PIT_MEM_LENGTH));
 }
 
-static int pit_ioport_write(struct kvm_io_device *this,
+static int pit_ioport_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this,
 			    gpa_t addr, int len, const void *data)
 {
 	struct kvm_pit *pit = dev_to_pit(this);
@@ -519,7 +520,8 @@ static int pit_ioport_write(struct kvm_io_device *this,
 	return 0;
 }
 
-static int pit_ioport_read(struct kvm_io_device *this,
+static int pit_ioport_read(struct kvm_vcpu *vcpu,
+			   struct kvm_io_device *this,
 			   gpa_t addr, int len, void *data)
 {
 	struct kvm_pit *pit = dev_to_pit(this);
@@ -589,7 +591,8 @@ static int pit_ioport_read(struct kvm_io_device *this,
 	return 0;
 }
 
-static int speaker_ioport_write(struct kvm_io_device *this,
+static int speaker_ioport_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this,
 				gpa_t addr, int len, const void *data)
 {
 	struct kvm_pit *pit = speaker_to_pit(this);
@@ -606,8 +609,9 @@ static int speaker_ioport_write(struct kvm_io_device *this,
 	return 0;
 }
 
-static int speaker_ioport_read(struct kvm_io_device *this,
-			       gpa_t addr, int len, void *data)
+static int speaker_ioport_read(struct kvm_vcpu *vcpu,
+				   struct kvm_io_device *this,
+				   gpa_t addr, int len, void *data)
 {
 	struct kvm_pit *pit = speaker_to_pit(this);
 	struct kvm_kpit_state *pit_state = &pit->pit_state;
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index cc31f7c..8ff4eaa 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -528,42 +528,42 @@ static int picdev_read(struct kvm_pic *s,
 	return 0;
 }
 
-static int picdev_master_write(struct kvm_io_device *dev,
+static int picdev_master_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			       gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_master),
 			    addr, len, val);
 }
 
-static int picdev_master_read(struct kvm_io_device *dev,
+static int picdev_master_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			      gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_master),
 			    addr, len, val);
 }
 
-static int picdev_slave_write(struct kvm_io_device *dev,
+static int picdev_slave_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			      gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_slave),
 			    addr, len, val);
 }
 
-static int picdev_slave_read(struct kvm_io_device *dev,
+static int picdev_slave_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			     gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_slave),
 			    addr, len, val);
 }
 
-static int picdev_eclr_write(struct kvm_io_device *dev,
+static int picdev_eclr_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			     gpa_t addr, int len, const void *val)
 {
 	return picdev_write(container_of(dev, struct kvm_pic, dev_eclr),
 			    addr, len, val);
 }
 
-static int picdev_eclr_read(struct kvm_io_device *dev,
+static int picdev_eclr_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
 			    gpa_t addr, int len, void *val)
 {
 	return picdev_read(container_of(dev, struct kvm_pic, dev_eclr),
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
index b1947e0..8bf2e49 100644
--- a/arch/x86/kvm/ioapic.c
+++ b/arch/x86/kvm/ioapic.c
@@ -498,8 +498,8 @@ static inline int ioapic_in_range(struct kvm_ioapic *ioapic, gpa_t addr)
 		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));
 }
 
-static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
-			    void *val)
+static int ioapic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+				gpa_t addr, int len, void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 result;
@@ -541,8 +541,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 	return 0;
 }
 
-static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
-			     const void *val)
+static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+				 gpa_t addr, int len, const void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
 	u32 data;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index a688fbf..5e0ce37 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1043,7 +1043,7 @@ static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
 	    addr < apic->base_address + LAPIC_MMIO_LENGTH;
 }
 
-static int apic_mmio_read(struct kvm_io_device *this,
+static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
 			   gpa_t address, int len, void *data)
 {
 	struct kvm_lapic *apic = to_lapic(this);
@@ -1363,7 +1363,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	return ret;
 }
 
-static int apic_mmio_write(struct kvm_io_device *this,
+static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
 			    gpa_t address, int len, const void *data)
 {
 	struct kvm_lapic *apic = to_lapic(this);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ce35071..31f8d39 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5623,7 +5623,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
 	gpa_t gpa;
 
 	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
-	if (!kvm_io_bus_write(vcpu->kvm, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
+	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
 		skip_emulated_instruction(vcpu);
 		return 1;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 556dfb4..1f522b5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4097,8 +4097,8 @@ static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
 	do {
 		n = min(len, 8);
 		if (!(vcpu->arch.apic &&
-		      !kvm_iodevice_write(&vcpu->arch.apic->dev, addr, n, v))
-		    && kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
+		      !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev, addr, n, v))
+		    && kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, n, v))
 			break;
 		handled += n;
 		addr += n;
@@ -4117,8 +4117,9 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v)
 	do {
 		n = min(len, 8);
 		if (!(vcpu->arch.apic &&
-		      !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v))
-		    && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
+		      !kvm_iodevice_read(vcpu, &vcpu->arch.apic->dev,
+					 addr, n, v))
+		    && kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, n, v))
 			break;
 		trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v);
 		handled += n;
@@ -4610,10 +4611,10 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
 	int r;
 
 	if (vcpu->arch.pio.in)
-		r = kvm_io_bus_read(vcpu->kvm, KVM_PIO_BUS, vcpu->arch.pio.port,
+		r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
 				    vcpu->arch.pio.size, pd);
 	else
-		r = kvm_io_bus_write(vcpu->kvm, KVM_PIO_BUS,
+		r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
 				     vcpu->arch.pio.port, vcpu->arch.pio.size,
 				     pd);
 	return r;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0ef2daa..fddaf0d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -169,12 +169,12 @@ enum kvm_bus {
 	KVM_NR_BUSES
 };
 
-int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		     int len, const void *val);
-int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
-			    int len, const void *val, long cookie);
-int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int len,
-		    void *val);
+int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
+			    gpa_t addr, int len, const void *val, long cookie);
+int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
+		    int len, void *val);
 int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 			    int len, struct kvm_io_device *dev);
 int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 00d8642..c831a40 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -60,8 +60,9 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev)
 	return 1;
 }
 
-static int coalesced_mmio_write(struct kvm_io_device *this,
-				gpa_t addr, int len, const void *val)
+static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
+				struct kvm_io_device *this, gpa_t addr,
+				int len, const void *val)
 {
 	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
 	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 148b239..99994c8 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -712,8 +712,8 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
 
 /* MMIO/PIO writes trigger an event if the addr/val match */
 static int
-ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
-		const void *val)
+ioeventfd_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
+		int len, const void *val)
 {
 	struct _ioeventfd *p = to_ioeventfd(this);
 
diff --git a/virt/kvm/iodev.h b/virt/kvm/iodev.h
index 12fd3ca..9ef709c 100644
--- a/virt/kvm/iodev.h
+++ b/virt/kvm/iodev.h
@@ -20,6 +20,7 @@
 #include <asm/errno.h>
 
 struct kvm_io_device;
+struct kvm_vcpu;
 
 /**
  * kvm_io_device_ops are called under kvm slots_lock.
@@ -27,11 +28,13 @@ struct kvm_io_device;
  * or non-zero to have it passed to the next device.
  **/
 struct kvm_io_device_ops {
-	int (*read)(struct kvm_io_device *this,
+	int (*read)(struct kvm_vcpu *vcpu,
+		    struct kvm_io_device *this,
 		    gpa_t addr,
 		    int len,
 		    void *val);
-	int (*write)(struct kvm_io_device *this,
+	int (*write)(struct kvm_vcpu *vcpu,
+		     struct kvm_io_device *this,
 		     gpa_t addr,
 		     int len,
 		     const void *val);
@@ -49,16 +52,20 @@ static inline void kvm_iodevice_init(struct kvm_io_device *dev,
 	dev->ops = ops;
 }
 
-static inline int kvm_iodevice_read(struct kvm_io_device *dev,
-				    gpa_t addr, int l, void *v)
+static inline int kvm_iodevice_read(struct kvm_vcpu *vcpu,
+				    struct kvm_io_device *dev, gpa_t addr,
+				    int l, void *v)
 {
-	return dev->ops->read ? dev->ops->read(dev, addr, l, v) : -EOPNOTSUPP;
+	return dev->ops->read ? dev->ops->read(vcpu, dev, addr, l, v)
+				: -EOPNOTSUPP;
 }
 
-static inline int kvm_iodevice_write(struct kvm_io_device *dev,
-				     gpa_t addr, int l, const void *v)
+static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu,
+				     struct kvm_io_device *dev, gpa_t addr,
+				     int l, const void *v)
 {
-	return dev->ops->write ? dev->ops->write(dev, addr, l, v) : -EOPNOTSUPP;
+	return dev->ops->write ? dev->ops->write(vcpu, dev, addr, l, v)
+				 : -EOPNOTSUPP;
 }
 
 static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 246cf29..60856d3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3004,7 +3004,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
 	return off;
 }
 
-static int __kvm_io_bus_write(struct kvm_io_bus *bus,
+static int __kvm_io_bus_write(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
 			      struct kvm_io_range *range, const void *val)
 {
 	int idx;
@@ -3015,7 +3015,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
 
 	while (idx < bus->dev_count &&
 		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
-		if (!kvm_iodevice_write(bus->range[idx].dev, range->addr,
+		if (!kvm_iodevice_write(vcpu, bus->range[idx].dev, range->addr,
 					range->len, val))
 			return idx;
 		idx++;
@@ -3025,7 +3025,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
 }
 
 /* kvm_io_bus_write - called under kvm->slots_lock */
-int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		     int len, const void *val)
 {
 	struct kvm_io_bus *bus;
@@ -3037,14 +3037,14 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
-	r = __kvm_io_bus_write(bus, &range, val);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
+	r = __kvm_io_bus_write(vcpu, bus, &range, val);
 	return r < 0 ? r : 0;
 }
 
 /* kvm_io_bus_write_cookie - called under kvm->slots_lock */
-int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
-			    int len, const void *val, long cookie)
+int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
+			    gpa_t addr, int len, const void *val, long cookie)
 {
 	struct kvm_io_bus *bus;
 	struct kvm_io_range range;
@@ -3054,12 +3054,12 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
 
 	/* First try the device referenced by cookie. */
 	if ((cookie >= 0) && (cookie < bus->dev_count) &&
 	    (kvm_io_bus_cmp(&range, &bus->range[cookie]) == 0))
-		if (!kvm_iodevice_write(bus->range[cookie].dev, addr, len,
+		if (!kvm_iodevice_write(vcpu, bus->range[cookie].dev, addr, len,
 					val))
 			return cookie;
 
@@ -3067,11 +3067,11 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 	 * cookie contained garbage; fall back to search and return the
 	 * correct cookie value.
 	 */
-	return __kvm_io_bus_write(bus, &range, val);
+	return __kvm_io_bus_write(vcpu, bus, &range, val);
 }
 
-static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
-			     void *val)
+static int __kvm_io_bus_read(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
+			     struct kvm_io_range *range, void *val)
 {
 	int idx;
 
@@ -3081,7 +3081,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
 
 	while (idx < bus->dev_count &&
 		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
-		if (!kvm_iodevice_read(bus->range[idx].dev, range->addr,
+		if (!kvm_iodevice_read(vcpu, bus->range[idx].dev, range->addr,
 				       range->len, val))
 			return idx;
 		idx++;
@@ -3092,7 +3092,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
 EXPORT_SYMBOL_GPL(kvm_io_bus_write);
 
 /* kvm_io_bus_read - called under kvm->slots_lock */
-int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
+int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
 		    int len, void *val)
 {
 	struct kvm_io_bus *bus;
@@ -3104,8 +3104,8 @@ int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
 		.len = len,
 	};
 
-	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
-	r = __kvm_io_bus_read(bus, &range, val);
+	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
+	r = __kvm_io_bus_read(vcpu, bus, &range, val);
 	return r < 0 ? r : 0;
 }
 

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 2/5] KVM: ARM: on IO mem abort - route the call to KVM MMIO bus
  2015-01-24 11:59 ` Nikolay Nikolaev
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

On IO memory abort, try to handle the MMIO access thorugh the KVM
registered read/write callbacks. This is done by invoking the relevant
kvm_io_bus_* API.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/mmio.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 5d3bfc0..d852137 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -162,6 +162,36 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	return 0;
 }
 
+/**
+ * handle_kernel_mmio - handle an in-kernel MMIO access
+ * @vcpu:	pointer to the vcpu performing the access
+ * @run:	pointer to the kvm_run structure
+ * @mmio:	pointer to the data describing the access
+ *
+ * returns true if the MMIO access has been performed in kernel space,
+ * and false if it needs to be emulated in user space.
+ */
+static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
+		struct kvm_exit_mmio *mmio)
+{
+	int ret;
+
+	if (mmio->is_write) {
+		ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+
+	} else {
+		ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+	}
+	if (!ret) {
+		kvm_prepare_mmio(run, mmio);
+		kvm_handle_mmio_return(vcpu, run);
+	}
+
+	return !ret;
+}
+
 int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		 phys_addr_t fault_ipa)
 {
@@ -203,6 +233,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	if (vgic_handle_mmio(vcpu, run, &mmio))
 		return 1;
 
+	if (handle_kernel_mmio(vcpu, run, &mmio))
+		return 1;
+
 	kvm_prepare_mmio(run, &mmio);
 	return 0;
 }


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 2/5] KVM: ARM: on IO mem abort - route the call to KVM MMIO bus
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: linux-arm-kernel

On IO memory abort, try to handle the MMIO access thorugh the KVM
registered read/write callbacks. This is done by invoking the relevant
kvm_io_bus_* API.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/mmio.c |   33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index 5d3bfc0..d852137 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -162,6 +162,36 @@ static int decode_hsr(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	return 0;
 }
 
+/**
+ * handle_kernel_mmio - handle an in-kernel MMIO access
+ * @vcpu:	pointer to the vcpu performing the access
+ * @run:	pointer to the kvm_run structure
+ * @mmio:	pointer to the data describing the access
+ *
+ * returns true if the MMIO access has been performed in kernel space,
+ * and false if it needs to be emulated in user space.
+ */
+static bool handle_kernel_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
+		struct kvm_exit_mmio *mmio)
+{
+	int ret;
+
+	if (mmio->is_write) {
+		ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+
+	} else {
+		ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, mmio->phys_addr,
+				mmio->len, &mmio->data);
+	}
+	if (!ret) {
+		kvm_prepare_mmio(run, mmio);
+		kvm_handle_mmio_return(vcpu, run);
+	}
+
+	return !ret;
+}
+
 int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		 phys_addr_t fault_ipa)
 {
@@ -203,6 +233,9 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	if (vgic_handle_mmio(vcpu, run, &mmio))
 		return 1;
 
+	if (handle_kernel_mmio(vcpu, run, &mmio))
+		return 1;
+
 	kvm_prepare_mmio(run, &mmio);
 	return 0;
 }

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-24 11:59 ` Nikolay Nikolaev
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
a single MMIO handling path - that is through the kvm_io_bus_ API.

Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
Both read and write calls are redirected to vgic_io_dev_access where
kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.


Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/mmio.c    |    3 -
 include/kvm/arm_vgic.h |    3 -
 virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 114 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index d852137..8dc2fde 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 			       fault_ipa, 0);
 	}
 
-	if (vgic_handle_mmio(vcpu, run, &mmio))
-		return 1;
-
 	if (handle_kernel_mmio(vcpu, run, &mmio))
 		return 1;
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7c55dd5..60639b1 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -237,6 +237,7 @@ struct vgic_dist {
 	unsigned long		*irq_pending_on_cpu;
 
 	struct vgic_vm_ops	vm_ops;
+	struct kvm_io_device	*io_dev;
 #endif
 };
 
@@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level);
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
-bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
-		      struct kvm_exit_mmio *mmio);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 0cc6ab6..195d2ba 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -31,6 +31,9 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/kvm.h>
+
+#include "iodev.h"
 
 /*
  * How the whole thing works (courtesy of Christoffer Dall):
@@ -77,6 +80,7 @@
 
 #include "vgic.h"
 
+static int vgic_register_kvm_io_dev(struct kvm *kvm);
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
 static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
@@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 int kvm_vgic_map_resources(struct kvm *kvm)
 {
+	vgic_register_kvm_io_dev(kvm);
 	return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
 }
 
@@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
 }
 
 /**
- * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
+ * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
  * @vcpu:      pointer to the vcpu performing the access
- * @run:       pointer to the kvm_run structure
- * @mmio:      pointer to the data describing the access
+ * @this:      pointer to the kvm_io_device structure
+ * @addr:      the MMIO address being accessed
+ * @len:       the length of the accessed data
+ * @val:       pointer to the value being written,
+ *             or where the read operation will store its result
+ * @is_write:  flag to show whether a write access is performed
  *
- * returns true if the MMIO access has been performed in kernel space,
- * and false if it needs to be emulated in user space.
+ * returns 0 if the MMIO access has been performed in kernel space,
+ * and 1 if it needs to be emulated in user space.
  * Calls the actual handling routine for the selected VGIC model.
  */
-bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
-		      struct kvm_exit_mmio *mmio)
+static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			    gpa_t addr, int len, void *val, bool is_write)
 {
-	if (!irqchip_in_kernel(vcpu->kvm))
-		return false;
+	struct kvm_exit_mmio mmio;
+	bool ret;
+
+	mmio = (struct kvm_exit_mmio) {
+		.phys_addr = addr,
+		.len = len,
+		.is_write = is_write,
+	};
+
+	if (is_write)
+		memcpy(mmio.data, val, len);
 
 	/*
 	 * This will currently call either vgic_v2_handle_mmio() or
 	 * vgic_v3_handle_mmio(), which in turn will call
 	 * vgic_handle_mmio_range() defined above.
 	 */
-	return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
+	ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
+
+	if (!is_write)
+		memcpy(val, mmio.data, len);
+
+	return ret ? 0 : 1;
+}
+
+static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			  gpa_t addr, int len, void *val)
+{
+	return vgic_io_dev_access(vcpu, this, addr, len, val, false);
+}
+
+static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			   gpa_t addr, int len, const void *val)
+{
+	return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
+}
+
+static const struct kvm_io_device_ops vgic_io_dev_ops = {
+	.read       = vgic_io_dev_read,
+	.write      = vgic_io_dev_write,
+};
+
+static int vgic_register_kvm_io_dev(struct kvm *kvm)
+{
+	int len = 0;
+	int ret;
+
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	unsigned long base = dist->vgic_dist_base;
+	u32 type = kvm->arch.vgic.vgic_model;
+	struct kvm_io_device *dev;
+
+	if (IS_VGIC_ADDR_UNDEF(base)) {
+		kvm_err("Need to set vgic distributor address first\n");
+		return -ENXIO;
+	}
+
+	dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	switch (type) {
+	case KVM_DEV_TYPE_ARM_VGIC_V2:
+		len = KVM_VGIC_V2_DIST_SIZE;
+		break;
+#ifdef CONFIG_ARM_GIC_V3
+	case KVM_DEV_TYPE_ARM_VGIC_V3:
+		len = KVM_VGIC_V3_DIST_SIZE;
+		break;
+#endif
+	}
+
+	kvm_iodevice_init(dev, &vgic_io_dev_ops);
+
+	mutex_lock(&kvm->slots_lock);
+
+	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
+			base, len, dev);
+	if (ret < 0)
+		goto out_unlock;
+	mutex_unlock(&kvm->slots_lock);
+
+	kvm->arch.vgic.io_dev = dev;
+
+	return 0;
+
+out_unlock:
+	mutex_unlock(&kvm->slots_lock);
+	kfree(dev);
+	return ret;
+}
+
+static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
+{
+	struct vgic_dist *dist = &kvm->arch.vgic;
+
+	if (dist) {
+		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
+		kfree(dist->io_dev);
+		dist->io_dev = NULL;
+	}
 }
 
 static int vgic_nr_shared_irqs(struct vgic_dist *dist)
@@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	int i;
 
+	vgic_unregister_kvm_io_dev(kvm);
+
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_vgic_vcpu_destroy(vcpu);

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: linux-arm-kernel

In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
a single MMIO handling path - that is through the kvm_io_bus_ API.

Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
Both read and write calls are redirected to vgic_io_dev_access where
kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.


Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/mmio.c    |    3 -
 include/kvm/arm_vgic.h |    3 -
 virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 114 insertions(+), 15 deletions(-)

diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
index d852137..8dc2fde 100644
--- a/arch/arm/kvm/mmio.c
+++ b/arch/arm/kvm/mmio.c
@@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
 			       fault_ipa, 0);
 	}
 
-	if (vgic_handle_mmio(vcpu, run, &mmio))
-		return 1;
-
 	if (handle_kernel_mmio(vcpu, run, &mmio))
 		return 1;
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 7c55dd5..60639b1 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -237,6 +237,7 @@ struct vgic_dist {
 	unsigned long		*irq_pending_on_cpu;
 
 	struct vgic_vm_ops	vm_ops;
+	struct kvm_io_device	*io_dev;
 #endif
 };
 
@@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level);
 void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
-bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
-		      struct kvm_exit_mmio *mmio);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 0cc6ab6..195d2ba 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -31,6 +31,9 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_arm.h>
 #include <asm/kvm_mmu.h>
+#include <asm/kvm.h>
+
+#include "iodev.h"
 
 /*
  * How the whole thing works (courtesy of Christoffer Dall):
@@ -77,6 +80,7 @@
 
 #include "vgic.h"
 
+static int vgic_register_kvm_io_dev(struct kvm *kvm);
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
 static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
@@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
 
 int kvm_vgic_map_resources(struct kvm *kvm)
 {
+	vgic_register_kvm_io_dev(kvm);
 	return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
 }
 
@@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
 }
 
 /**
- * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
+ * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
  * @vcpu:      pointer to the vcpu performing the access
- * @run:       pointer to the kvm_run structure
- * @mmio:      pointer to the data describing the access
+ * @this:      pointer to the kvm_io_device structure
+ * @addr:      the MMIO address being accessed
+ * @len:       the length of the accessed data
+ * @val:       pointer to the value being written,
+ *             or where the read operation will store its result
+ * @is_write:  flag to show whether a write access is performed
  *
- * returns true if the MMIO access has been performed in kernel space,
- * and false if it needs to be emulated in user space.
+ * returns 0 if the MMIO access has been performed in kernel space,
+ * and 1 if it needs to be emulated in user space.
  * Calls the actual handling routine for the selected VGIC model.
  */
-bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
-		      struct kvm_exit_mmio *mmio)
+static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			    gpa_t addr, int len, void *val, bool is_write)
 {
-	if (!irqchip_in_kernel(vcpu->kvm))
-		return false;
+	struct kvm_exit_mmio mmio;
+	bool ret;
+
+	mmio = (struct kvm_exit_mmio) {
+		.phys_addr = addr,
+		.len = len,
+		.is_write = is_write,
+	};
+
+	if (is_write)
+		memcpy(mmio.data, val, len);
 
 	/*
 	 * This will currently call either vgic_v2_handle_mmio() or
 	 * vgic_v3_handle_mmio(), which in turn will call
 	 * vgic_handle_mmio_range() defined above.
 	 */
-	return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
+	ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
+
+	if (!is_write)
+		memcpy(val, mmio.data, len);
+
+	return ret ? 0 : 1;
+}
+
+static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			  gpa_t addr, int len, void *val)
+{
+	return vgic_io_dev_access(vcpu, this, addr, len, val, false);
+}
+
+static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
+			   gpa_t addr, int len, const void *val)
+{
+	return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
+}
+
+static const struct kvm_io_device_ops vgic_io_dev_ops = {
+	.read       = vgic_io_dev_read,
+	.write      = vgic_io_dev_write,
+};
+
+static int vgic_register_kvm_io_dev(struct kvm *kvm)
+{
+	int len = 0;
+	int ret;
+
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	unsigned long base = dist->vgic_dist_base;
+	u32 type = kvm->arch.vgic.vgic_model;
+	struct kvm_io_device *dev;
+
+	if (IS_VGIC_ADDR_UNDEF(base)) {
+		kvm_err("Need to set vgic distributor address first\n");
+		return -ENXIO;
+	}
+
+	dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	switch (type) {
+	case KVM_DEV_TYPE_ARM_VGIC_V2:
+		len = KVM_VGIC_V2_DIST_SIZE;
+		break;
+#ifdef CONFIG_ARM_GIC_V3
+	case KVM_DEV_TYPE_ARM_VGIC_V3:
+		len = KVM_VGIC_V3_DIST_SIZE;
+		break;
+#endif
+	}
+
+	kvm_iodevice_init(dev, &vgic_io_dev_ops);
+
+	mutex_lock(&kvm->slots_lock);
+
+	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
+			base, len, dev);
+	if (ret < 0)
+		goto out_unlock;
+	mutex_unlock(&kvm->slots_lock);
+
+	kvm->arch.vgic.io_dev = dev;
+
+	return 0;
+
+out_unlock:
+	mutex_unlock(&kvm->slots_lock);
+	kfree(dev);
+	return ret;
+}
+
+static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
+{
+	struct vgic_dist *dist = &kvm->arch.vgic;
+
+	if (dist) {
+		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
+		kfree(dist->io_dev);
+		dist->io_dev = NULL;
+	}
 }
 
 static int vgic_nr_shared_irqs(struct vgic_dist *dist)
@@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
 	struct kvm_vcpu *vcpu;
 	int i;
 
+	vgic_unregister_kvm_io_dev(kvm);
+
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_vgic_vcpu_destroy(vcpu);
 

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 4/5] ARM/ARM64: enable linking against eventfd
  2015-01-24 11:59 ` Nikolay Nikolaev
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

This enables compilation of the eventfd feature on ARM/ARM64.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/Kconfig    |    1 +
 arch/arm/kvm/Makefile   |    2 +-
 arch/arm64/kvm/Kconfig  |    1 +
 arch/arm64/kvm/Makefile |    2 +-
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index a8d1ace..ca23ded 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -20,6 +20,7 @@ config KVM
 	bool "Kernel-based Virtual Machine (KVM) support"
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
+	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select HAVE_KVM_ARCH_TLB_FLUSH_ALL
 	select KVM_MMIO
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index 443b8be..539c1a5 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)
 
 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 3ce389b..2cc985c 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -21,6 +21,7 @@ config KVM
 	select MMU_NOTIFIER
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
+	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select HAVE_KVM_ARCH_TLB_FLUSH_ALL
 	select KVM_MMIO
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 4e6e09e..0dffb5f 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -11,7 +11,7 @@ ARM=../../../arch/arm/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += kvm.o
 
-kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 4/5] ARM/ARM64: enable linking against eventfd
@ 2015-01-24 11:59   ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 11:59 UTC (permalink / raw)
  To: linux-arm-kernel

This enables compilation of the eventfd feature on ARM/ARM64.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/Kconfig    |    1 +
 arch/arm/kvm/Makefile   |    2 +-
 arch/arm64/kvm/Kconfig  |    1 +
 arch/arm64/kvm/Makefile |    2 +-
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
index a8d1ace..ca23ded 100644
--- a/arch/arm/kvm/Kconfig
+++ b/arch/arm/kvm/Kconfig
@@ -20,6 +20,7 @@ config KVM
 	bool "Kernel-based Virtual Machine (KVM) support"
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
+	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select HAVE_KVM_ARCH_TLB_FLUSH_ALL
 	select KVM_MMIO
diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index 443b8be..539c1a5 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)
 
 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o
 
 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 3ce389b..2cc985c 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -21,6 +21,7 @@ config KVM
 	select MMU_NOTIFIER
 	select PREEMPT_NOTIFIERS
 	select ANON_INODES
+	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_CPU_RELAX_INTERCEPT
 	select HAVE_KVM_ARCH_TLB_FLUSH_ALL
 	select KVM_MMIO
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 4e6e09e..0dffb5f 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -11,7 +11,7 @@ ARM=../../../arch/arm/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += kvm.o
 
-kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 5/5] ARM: enable KVM_CAP_IOEVENTFD
  2015-01-24 11:59 ` Nikolay Nikolaev
@ 2015-01-24 12:00   ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 12:00 UTC (permalink / raw)
  To: kvm, eric.auger, marc.zyngier, andre.przywara, kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel

KVM on arm will support the eventfd extension.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/arm.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 6fbfa5f..ec5ebef 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -197,6 +197,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
 		break;
+	case KVM_CAP_IOEVENTFD:
+		r = 1;
+		break;
 	default:
 		r = kvm_arch_dev_ioctl_check_extension(ext);
 		break;


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 5/5] ARM: enable KVM_CAP_IOEVENTFD
@ 2015-01-24 12:00   ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-24 12:00 UTC (permalink / raw)
  To: linux-arm-kernel

KVM on arm will support the eventfd extension.

Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 arch/arm/kvm/arm.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 6fbfa5f..ec5ebef 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -197,6 +197,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
 		break;
+	case KVM_CAP_IOEVENTFD:
+		r = 1;
+		break;
 	default:
 		r = kvm_arch_dev_ioctl_check_extension(ext);
 		break;

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/5] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
  2015-01-24 11:59   ` Nikolay Nikolaev
@ 2015-01-24 19:08     ` Paolo Bonzini
  -1 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2015-01-24 19:08 UTC (permalink / raw)
  To: Nikolay Nikolaev, kvm, eric.auger, marc.zyngier, andre.przywara,
	kvmarm, christoffer.dall
  Cc: tech, linux-arm-kernel



On 24/01/2015 12:59, Nikolay Nikolaev wrote:
> This is needed in e.g. ARM vGIC emulation, where the MMIO handling
> depends on the VCPU that does the access.
> 
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  arch/powerpc/kvm/mpic.c    |   10 ++++++----
>  arch/powerpc/kvm/powerpc.c |    4 ++--
>  arch/s390/kvm/diag.c       |    2 +-
>  arch/x86/kvm/i8254.c       |   14 +++++++++-----
>  arch/x86/kvm/i8259.c       |   12 ++++++------
>  arch/x86/kvm/ioapic.c      |    8 ++++----
>  arch/x86/kvm/lapic.c       |    4 ++--
>  arch/x86/kvm/vmx.c         |    2 +-
>  arch/x86/kvm/x86.c         |   13 +++++++------
>  include/linux/kvm_host.h   |   10 +++++-----
>  virt/kvm/coalesced_mmio.c  |    5 +++--
>  virt/kvm/eventfd.c         |    4 ++--
>  virt/kvm/iodev.h           |   23 +++++++++++++++--------
>  virt/kvm/kvm_main.c        |   32 ++++++++++++++++----------------
>  14 files changed, 79 insertions(+), 64 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
> index 39b3a8f..8542f07 100644
> --- a/arch/powerpc/kvm/mpic.c
> +++ b/arch/powerpc/kvm/mpic.c
> @@ -1374,8 +1374,9 @@ static int kvm_mpic_write_internal(struct openpic *opp, gpa_t addr, u32 val)
>  	return -ENXIO;
>  }
>  
> -static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
> -			 int len, void *ptr)
> +static int kvm_mpic_read(struct kvm_vcpu *vcpu,
> +			 struct kvm_io_device *this,
> +			 gpa_t addr, int len, void *ptr)
>  {
>  	struct openpic *opp = container_of(this, struct openpic, mmio);
>  	int ret;
> @@ -1415,8 +1416,9 @@ static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
>  	return ret;
>  }
>  
> -static int kvm_mpic_write(struct kvm_io_device *this, gpa_t addr,
> -			  int len, const void *ptr)
> +static int kvm_mpic_write(struct kvm_vcpu *vcpu,
> +			  struct kvm_io_device *this,
> +			  gpa_t addr, int len, const void *ptr)
>  {
>  	struct openpic *opp = container_of(this, struct openpic, mmio);
>  	int ret;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index c45eaab..0aac251 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -808,7 +808,7 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
> -	ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
> +	ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
>  			      bytes, &run->mmio.data);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> @@ -881,7 +881,7 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
> -	ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
> +	ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
>  			       bytes, &run->mmio.data);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 9254aff..329ec75 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -213,7 +213,7 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
>  	 * - gpr 3 contains the virtqueue index (passed as datamatch)
>  	 * - gpr 4 contains the index on the bus (optionally)
>  	 */
> -	ret = kvm_io_bus_write_cookie(vcpu->kvm, KVM_VIRTIO_CCW_NOTIFY_BUS,
> +	ret = kvm_io_bus_write_cookie(vcpu, KVM_VIRTIO_CCW_NOTIFY_BUS,
>  				      vcpu->run->s.regs.gprs[2] & 0xffffffff,
>  				      8, &vcpu->run->s.regs.gprs[3],
>  				      vcpu->run->s.regs.gprs[4]);
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 298781d..4dce6f8 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -443,7 +443,8 @@ static inline int pit_in_range(gpa_t addr)
>  		(addr < KVM_PIT_BASE_ADDRESS + KVM_PIT_MEM_LENGTH));
>  }
>  
> -static int pit_ioport_write(struct kvm_io_device *this,
> +static int pit_ioport_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this,
>  			    gpa_t addr, int len, const void *data)
>  {
>  	struct kvm_pit *pit = dev_to_pit(this);
> @@ -519,7 +520,8 @@ static int pit_ioport_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int pit_ioport_read(struct kvm_io_device *this,
> +static int pit_ioport_read(struct kvm_vcpu *vcpu,
> +			   struct kvm_io_device *this,
>  			   gpa_t addr, int len, void *data)
>  {
>  	struct kvm_pit *pit = dev_to_pit(this);
> @@ -589,7 +591,8 @@ static int pit_ioport_read(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int speaker_ioport_write(struct kvm_io_device *this,
> +static int speaker_ioport_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this,
>  				gpa_t addr, int len, const void *data)
>  {
>  	struct kvm_pit *pit = speaker_to_pit(this);
> @@ -606,8 +609,9 @@ static int speaker_ioport_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int speaker_ioport_read(struct kvm_io_device *this,
> -			       gpa_t addr, int len, void *data)
> +static int speaker_ioport_read(struct kvm_vcpu *vcpu,
> +				   struct kvm_io_device *this,
> +				   gpa_t addr, int len, void *data)
>  {
>  	struct kvm_pit *pit = speaker_to_pit(this);
>  	struct kvm_kpit_state *pit_state = &pit->pit_state;
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index cc31f7c..8ff4eaa 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -528,42 +528,42 @@ static int picdev_read(struct kvm_pic *s,
>  	return 0;
>  }
>  
> -static int picdev_master_write(struct kvm_io_device *dev,
> +static int picdev_master_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			       gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_master),
>  			    addr, len, val);
>  }
>  
> -static int picdev_master_read(struct kvm_io_device *dev,
> +static int picdev_master_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			      gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_master),
>  			    addr, len, val);
>  }
>  
> -static int picdev_slave_write(struct kvm_io_device *dev,
> +static int picdev_slave_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			      gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_slave),
>  			    addr, len, val);
>  }
>  
> -static int picdev_slave_read(struct kvm_io_device *dev,
> +static int picdev_slave_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			     gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_slave),
>  			    addr, len, val);
>  }
>  
> -static int picdev_eclr_write(struct kvm_io_device *dev,
> +static int picdev_eclr_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			     gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_eclr),
>  			    addr, len, val);
>  }
>  
> -static int picdev_eclr_read(struct kvm_io_device *dev,
> +static int picdev_eclr_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			    gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_eclr),
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index b1947e0..8bf2e49 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -498,8 +498,8 @@ static inline int ioapic_in_range(struct kvm_ioapic *ioapic, gpa_t addr)
>  		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));
>  }
>  
> -static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> -			    void *val)
> +static int ioapic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +				gpa_t addr, int len, void *val)
>  {
>  	struct kvm_ioapic *ioapic = to_ioapic(this);
>  	u32 result;
> @@ -541,8 +541,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
>  	return 0;
>  }
>  
> -static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
> -			     const void *val)
> +static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +				 gpa_t addr, int len, const void *val)
>  {
>  	struct kvm_ioapic *ioapic = to_ioapic(this);
>  	u32 data;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index a688fbf..5e0ce37 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1043,7 +1043,7 @@ static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
>  	    addr < apic->base_address + LAPIC_MMIO_LENGTH;
>  }
>  
> -static int apic_mmio_read(struct kvm_io_device *this,
> +static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>  			   gpa_t address, int len, void *data)
>  {
>  	struct kvm_lapic *apic = to_lapic(this);
> @@ -1363,7 +1363,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  	return ret;
>  }
>  
> -static int apic_mmio_write(struct kvm_io_device *this,
> +static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>  			    gpa_t address, int len, const void *data)
>  {
>  	struct kvm_lapic *apic = to_lapic(this);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ce35071..31f8d39 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5623,7 +5623,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
>  	gpa_t gpa;
>  
>  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> -	if (!kvm_io_bus_write(vcpu->kvm, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
> +	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
>  		skip_emulated_instruction(vcpu);
>  		return 1;
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 556dfb4..1f522b5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4097,8 +4097,8 @@ static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
>  	do {
>  		n = min(len, 8);
>  		if (!(vcpu->arch.apic &&
> -		      !kvm_iodevice_write(&vcpu->arch.apic->dev, addr, n, v))
> -		    && kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
> +		      !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev, addr, n, v))
> +		    && kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, n, v))
>  			break;
>  		handled += n;
>  		addr += n;
> @@ -4117,8 +4117,9 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v)
>  	do {
>  		n = min(len, 8);
>  		if (!(vcpu->arch.apic &&
> -		      !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v))
> -		    && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
> +		      !kvm_iodevice_read(vcpu, &vcpu->arch.apic->dev,
> +					 addr, n, v))
> +		    && kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, n, v))
>  			break;
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v);
>  		handled += n;
> @@ -4610,10 +4611,10 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
>  	int r;
>  
>  	if (vcpu->arch.pio.in)
> -		r = kvm_io_bus_read(vcpu->kvm, KVM_PIO_BUS, vcpu->arch.pio.port,
> +		r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
>  				    vcpu->arch.pio.size, pd);
>  	else
> -		r = kvm_io_bus_write(vcpu->kvm, KVM_PIO_BUS,
> +		r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
>  				     vcpu->arch.pio.port, vcpu->arch.pio.size,
>  				     pd);
>  	return r;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0ef2daa..fddaf0d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -169,12 +169,12 @@ enum kvm_bus {
>  	KVM_NR_BUSES
>  };
>  
> -int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		     int len, const void *val);
> -int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> -			    int len, const void *val, long cookie);
> -int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int len,
> -		    void *val);
> +int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
> +			    gpa_t addr, int len, const void *val, long cookie);
> +int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
> +		    int len, void *val);
>  int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  			    int len, struct kvm_io_device *dev);
>  int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 00d8642..c831a40 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -60,8 +60,9 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev)
>  	return 1;
>  }
>  
> -static int coalesced_mmio_write(struct kvm_io_device *this,
> -				gpa_t addr, int len, const void *val)
> +static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this, gpa_t addr,
> +				int len, const void *val)
>  {
>  	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
>  	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 148b239..99994c8 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -712,8 +712,8 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
>  
>  /* MMIO/PIO writes trigger an event if the addr/val match */
>  static int
> -ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
> -		const void *val)
> +ioeventfd_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
> +		int len, const void *val)
>  {
>  	struct _ioeventfd *p = to_ioeventfd(this);
>  
> diff --git a/virt/kvm/iodev.h b/virt/kvm/iodev.h
> index 12fd3ca..9ef709c 100644
> --- a/virt/kvm/iodev.h
> +++ b/virt/kvm/iodev.h
> @@ -20,6 +20,7 @@
>  #include <asm/errno.h>
>  
>  struct kvm_io_device;
> +struct kvm_vcpu;
>  
>  /**
>   * kvm_io_device_ops are called under kvm slots_lock.
> @@ -27,11 +28,13 @@ struct kvm_io_device;
>   * or non-zero to have it passed to the next device.
>   **/
>  struct kvm_io_device_ops {
> -	int (*read)(struct kvm_io_device *this,
> +	int (*read)(struct kvm_vcpu *vcpu,
> +		    struct kvm_io_device *this,
>  		    gpa_t addr,
>  		    int len,
>  		    void *val);
> -	int (*write)(struct kvm_io_device *this,
> +	int (*write)(struct kvm_vcpu *vcpu,
> +		     struct kvm_io_device *this,
>  		     gpa_t addr,
>  		     int len,
>  		     const void *val);
> @@ -49,16 +52,20 @@ static inline void kvm_iodevice_init(struct kvm_io_device *dev,
>  	dev->ops = ops;
>  }
>  
> -static inline int kvm_iodevice_read(struct kvm_io_device *dev,
> -				    gpa_t addr, int l, void *v)
> +static inline int kvm_iodevice_read(struct kvm_vcpu *vcpu,
> +				    struct kvm_io_device *dev, gpa_t addr,
> +				    int l, void *v)
>  {
> -	return dev->ops->read ? dev->ops->read(dev, addr, l, v) : -EOPNOTSUPP;
> +	return dev->ops->read ? dev->ops->read(vcpu, dev, addr, l, v)
> +				: -EOPNOTSUPP;
>  }
>  
> -static inline int kvm_iodevice_write(struct kvm_io_device *dev,
> -				     gpa_t addr, int l, const void *v)
> +static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu,
> +				     struct kvm_io_device *dev, gpa_t addr,
> +				     int l, const void *v)
>  {
> -	return dev->ops->write ? dev->ops->write(dev, addr, l, v) : -EOPNOTSUPP;
> +	return dev->ops->write ? dev->ops->write(vcpu, dev, addr, l, v)
> +				 : -EOPNOTSUPP;
>  }
>  
>  static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 246cf29..60856d3 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3004,7 +3004,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
>  	return off;
>  }
>  
> -static int __kvm_io_bus_write(struct kvm_io_bus *bus,
> +static int __kvm_io_bus_write(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
>  			      struct kvm_io_range *range, const void *val)
>  {
>  	int idx;
> @@ -3015,7 +3015,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
>  
>  	while (idx < bus->dev_count &&
>  		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
> -		if (!kvm_iodevice_write(bus->range[idx].dev, range->addr,
> +		if (!kvm_iodevice_write(vcpu, bus->range[idx].dev, range->addr,
>  					range->len, val))
>  			return idx;
>  		idx++;
> @@ -3025,7 +3025,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
>  }
>  
>  /* kvm_io_bus_write - called under kvm->slots_lock */
> -int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		     int len, const void *val)
>  {
>  	struct kvm_io_bus *bus;
> @@ -3037,14 +3037,14 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> -	r = __kvm_io_bus_write(bus, &range, val);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
> +	r = __kvm_io_bus_write(vcpu, bus, &range, val);
>  	return r < 0 ? r : 0;
>  }
>  
>  /* kvm_io_bus_write_cookie - called under kvm->slots_lock */
> -int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> -			    int len, const void *val, long cookie)
> +int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
> +			    gpa_t addr, int len, const void *val, long cookie)
>  {
>  	struct kvm_io_bus *bus;
>  	struct kvm_io_range range;
> @@ -3054,12 +3054,12 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
>  
>  	/* First try the device referenced by cookie. */
>  	if ((cookie >= 0) && (cookie < bus->dev_count) &&
>  	    (kvm_io_bus_cmp(&range, &bus->range[cookie]) == 0))
> -		if (!kvm_iodevice_write(bus->range[cookie].dev, addr, len,
> +		if (!kvm_iodevice_write(vcpu, bus->range[cookie].dev, addr, len,
>  					val))
>  			return cookie;
>  
> @@ -3067,11 +3067,11 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  	 * cookie contained garbage; fall back to search and return the
>  	 * correct cookie value.
>  	 */
> -	return __kvm_io_bus_write(bus, &range, val);
> +	return __kvm_io_bus_write(vcpu, bus, &range, val);
>  }
>  
> -static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
> -			     void *val)
> +static int __kvm_io_bus_read(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
> +			     struct kvm_io_range *range, void *val)
>  {
>  	int idx;
>  
> @@ -3081,7 +3081,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
>  
>  	while (idx < bus->dev_count &&
>  		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
> -		if (!kvm_iodevice_read(bus->range[idx].dev, range->addr,
> +		if (!kvm_iodevice_read(vcpu, bus->range[idx].dev, range->addr,
>  				       range->len, val))
>  			return idx;
>  		idx++;
> @@ -3092,7 +3092,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
>  EXPORT_SYMBOL_GPL(kvm_io_bus_write);
>  
>  /* kvm_io_bus_read - called under kvm->slots_lock */
> -int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		    int len, void *val)
>  {
>  	struct kvm_io_bus *bus;
> @@ -3104,8 +3104,8 @@ int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> -	r = __kvm_io_bus_read(bus, &range, val);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
> +	r = __kvm_io_bus_read(vcpu, bus, &range, val);
>  	return r < 0 ? r : 0;
>  }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 1/5] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks.
@ 2015-01-24 19:08     ` Paolo Bonzini
  0 siblings, 0 replies; 26+ messages in thread
From: Paolo Bonzini @ 2015-01-24 19:08 UTC (permalink / raw)
  To: linux-arm-kernel



On 24/01/2015 12:59, Nikolay Nikolaev wrote:
> This is needed in e.g. ARM vGIC emulation, where the MMIO handling
> depends on the VCPU that does the access.
> 
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  arch/powerpc/kvm/mpic.c    |   10 ++++++----
>  arch/powerpc/kvm/powerpc.c |    4 ++--
>  arch/s390/kvm/diag.c       |    2 +-
>  arch/x86/kvm/i8254.c       |   14 +++++++++-----
>  arch/x86/kvm/i8259.c       |   12 ++++++------
>  arch/x86/kvm/ioapic.c      |    8 ++++----
>  arch/x86/kvm/lapic.c       |    4 ++--
>  arch/x86/kvm/vmx.c         |    2 +-
>  arch/x86/kvm/x86.c         |   13 +++++++------
>  include/linux/kvm_host.h   |   10 +++++-----
>  virt/kvm/coalesced_mmio.c  |    5 +++--
>  virt/kvm/eventfd.c         |    4 ++--
>  virt/kvm/iodev.h           |   23 +++++++++++++++--------
>  virt/kvm/kvm_main.c        |   32 ++++++++++++++++----------------
>  14 files changed, 79 insertions(+), 64 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
> index 39b3a8f..8542f07 100644
> --- a/arch/powerpc/kvm/mpic.c
> +++ b/arch/powerpc/kvm/mpic.c
> @@ -1374,8 +1374,9 @@ static int kvm_mpic_write_internal(struct openpic *opp, gpa_t addr, u32 val)
>  	return -ENXIO;
>  }
>  
> -static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
> -			 int len, void *ptr)
> +static int kvm_mpic_read(struct kvm_vcpu *vcpu,
> +			 struct kvm_io_device *this,
> +			 gpa_t addr, int len, void *ptr)
>  {
>  	struct openpic *opp = container_of(this, struct openpic, mmio);
>  	int ret;
> @@ -1415,8 +1416,9 @@ static int kvm_mpic_read(struct kvm_io_device *this, gpa_t addr,
>  	return ret;
>  }
>  
> -static int kvm_mpic_write(struct kvm_io_device *this, gpa_t addr,
> -			  int len, const void *ptr)
> +static int kvm_mpic_write(struct kvm_vcpu *vcpu,
> +			  struct kvm_io_device *this,
> +			  gpa_t addr, int len, const void *ptr)
>  {
>  	struct openpic *opp = container_of(this, struct openpic, mmio);
>  	int ret;
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index c45eaab..0aac251 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -808,7 +808,7 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
> -	ret = kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
> +	ret = kvm_io_bus_read(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
>  			      bytes, &run->mmio.data);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> @@ -881,7 +881,7 @@ int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  
>  	idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
> -	ret = kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, run->mmio.phys_addr,
> +	ret = kvm_io_bus_write(vcpu, KVM_MMIO_BUS, run->mmio.phys_addr,
>  			       bytes, &run->mmio.data);
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 9254aff..329ec75 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -213,7 +213,7 @@ static int __diag_virtio_hypercall(struct kvm_vcpu *vcpu)
>  	 * - gpr 3 contains the virtqueue index (passed as datamatch)
>  	 * - gpr 4 contains the index on the bus (optionally)
>  	 */
> -	ret = kvm_io_bus_write_cookie(vcpu->kvm, KVM_VIRTIO_CCW_NOTIFY_BUS,
> +	ret = kvm_io_bus_write_cookie(vcpu, KVM_VIRTIO_CCW_NOTIFY_BUS,
>  				      vcpu->run->s.regs.gprs[2] & 0xffffffff,
>  				      8, &vcpu->run->s.regs.gprs[3],
>  				      vcpu->run->s.regs.gprs[4]);
> diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> index 298781d..4dce6f8 100644
> --- a/arch/x86/kvm/i8254.c
> +++ b/arch/x86/kvm/i8254.c
> @@ -443,7 +443,8 @@ static inline int pit_in_range(gpa_t addr)
>  		(addr < KVM_PIT_BASE_ADDRESS + KVM_PIT_MEM_LENGTH));
>  }
>  
> -static int pit_ioport_write(struct kvm_io_device *this,
> +static int pit_ioport_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this,
>  			    gpa_t addr, int len, const void *data)
>  {
>  	struct kvm_pit *pit = dev_to_pit(this);
> @@ -519,7 +520,8 @@ static int pit_ioport_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int pit_ioport_read(struct kvm_io_device *this,
> +static int pit_ioport_read(struct kvm_vcpu *vcpu,
> +			   struct kvm_io_device *this,
>  			   gpa_t addr, int len, void *data)
>  {
>  	struct kvm_pit *pit = dev_to_pit(this);
> @@ -589,7 +591,8 @@ static int pit_ioport_read(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int speaker_ioport_write(struct kvm_io_device *this,
> +static int speaker_ioport_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this,
>  				gpa_t addr, int len, const void *data)
>  {
>  	struct kvm_pit *pit = speaker_to_pit(this);
> @@ -606,8 +609,9 @@ static int speaker_ioport_write(struct kvm_io_device *this,
>  	return 0;
>  }
>  
> -static int speaker_ioport_read(struct kvm_io_device *this,
> -			       gpa_t addr, int len, void *data)
> +static int speaker_ioport_read(struct kvm_vcpu *vcpu,
> +				   struct kvm_io_device *this,
> +				   gpa_t addr, int len, void *data)
>  {
>  	struct kvm_pit *pit = speaker_to_pit(this);
>  	struct kvm_kpit_state *pit_state = &pit->pit_state;
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index cc31f7c..8ff4eaa 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -528,42 +528,42 @@ static int picdev_read(struct kvm_pic *s,
>  	return 0;
>  }
>  
> -static int picdev_master_write(struct kvm_io_device *dev,
> +static int picdev_master_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			       gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_master),
>  			    addr, len, val);
>  }
>  
> -static int picdev_master_read(struct kvm_io_device *dev,
> +static int picdev_master_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			      gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_master),
>  			    addr, len, val);
>  }
>  
> -static int picdev_slave_write(struct kvm_io_device *dev,
> +static int picdev_slave_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			      gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_slave),
>  			    addr, len, val);
>  }
>  
> -static int picdev_slave_read(struct kvm_io_device *dev,
> +static int picdev_slave_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			     gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_slave),
>  			    addr, len, val);
>  }
>  
> -static int picdev_eclr_write(struct kvm_io_device *dev,
> +static int picdev_eclr_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			     gpa_t addr, int len, const void *val)
>  {
>  	return picdev_write(container_of(dev, struct kvm_pic, dev_eclr),
>  			    addr, len, val);
>  }
>  
> -static int picdev_eclr_read(struct kvm_io_device *dev,
> +static int picdev_eclr_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
>  			    gpa_t addr, int len, void *val)
>  {
>  	return picdev_read(container_of(dev, struct kvm_pic, dev_eclr),
> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
> index b1947e0..8bf2e49 100644
> --- a/arch/x86/kvm/ioapic.c
> +++ b/arch/x86/kvm/ioapic.c
> @@ -498,8 +498,8 @@ static inline int ioapic_in_range(struct kvm_ioapic *ioapic, gpa_t addr)
>  		 (addr < ioapic->base_address + IOAPIC_MEM_LENGTH)));
>  }
>  
> -static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> -			    void *val)
> +static int ioapic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +				gpa_t addr, int len, void *val)
>  {
>  	struct kvm_ioapic *ioapic = to_ioapic(this);
>  	u32 result;
> @@ -541,8 +541,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
>  	return 0;
>  }
>  
> -static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
> -			     const void *val)
> +static int ioapic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +				 gpa_t addr, int len, const void *val)
>  {
>  	struct kvm_ioapic *ioapic = to_ioapic(this);
>  	u32 data;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index a688fbf..5e0ce37 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1043,7 +1043,7 @@ static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
>  	    addr < apic->base_address + LAPIC_MMIO_LENGTH;
>  }
>  
> -static int apic_mmio_read(struct kvm_io_device *this,
> +static int apic_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>  			   gpa_t address, int len, void *data)
>  {
>  	struct kvm_lapic *apic = to_lapic(this);
> @@ -1363,7 +1363,7 @@ static int apic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  	return ret;
>  }
>  
> -static int apic_mmio_write(struct kvm_io_device *this,
> +static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>  			    gpa_t address, int len, const void *data)
>  {
>  	struct kvm_lapic *apic = to_lapic(this);
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ce35071..31f8d39 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5623,7 +5623,7 @@ static int handle_ept_misconfig(struct kvm_vcpu *vcpu)
>  	gpa_t gpa;
>  
>  	gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> -	if (!kvm_io_bus_write(vcpu->kvm, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
> +	if (!kvm_io_bus_write(vcpu, KVM_FAST_MMIO_BUS, gpa, 0, NULL)) {
>  		skip_emulated_instruction(vcpu);
>  		return 1;
>  	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 556dfb4..1f522b5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4097,8 +4097,8 @@ static int vcpu_mmio_write(struct kvm_vcpu *vcpu, gpa_t addr, int len,
>  	do {
>  		n = min(len, 8);
>  		if (!(vcpu->arch.apic &&
> -		      !kvm_iodevice_write(&vcpu->arch.apic->dev, addr, n, v))
> -		    && kvm_io_bus_write(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
> +		      !kvm_iodevice_write(vcpu, &vcpu->arch.apic->dev, addr, n, v))
> +		    && kvm_io_bus_write(vcpu, KVM_MMIO_BUS, addr, n, v))
>  			break;
>  		handled += n;
>  		addr += n;
> @@ -4117,8 +4117,9 @@ static int vcpu_mmio_read(struct kvm_vcpu *vcpu, gpa_t addr, int len, void *v)
>  	do {
>  		n = min(len, 8);
>  		if (!(vcpu->arch.apic &&
> -		      !kvm_iodevice_read(&vcpu->arch.apic->dev, addr, n, v))
> -		    && kvm_io_bus_read(vcpu->kvm, KVM_MMIO_BUS, addr, n, v))
> +		      !kvm_iodevice_read(vcpu, &vcpu->arch.apic->dev,
> +					 addr, n, v))
> +		    && kvm_io_bus_read(vcpu, KVM_MMIO_BUS, addr, n, v))
>  			break;
>  		trace_kvm_mmio(KVM_TRACE_MMIO_READ, n, addr, *(u64 *)v);
>  		handled += n;
> @@ -4610,10 +4611,10 @@ static int kernel_pio(struct kvm_vcpu *vcpu, void *pd)
>  	int r;
>  
>  	if (vcpu->arch.pio.in)
> -		r = kvm_io_bus_read(vcpu->kvm, KVM_PIO_BUS, vcpu->arch.pio.port,
> +		r = kvm_io_bus_read(vcpu, KVM_PIO_BUS, vcpu->arch.pio.port,
>  				    vcpu->arch.pio.size, pd);
>  	else
> -		r = kvm_io_bus_write(vcpu->kvm, KVM_PIO_BUS,
> +		r = kvm_io_bus_write(vcpu, KVM_PIO_BUS,
>  				     vcpu->arch.pio.port, vcpu->arch.pio.size,
>  				     pd);
>  	return r;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0ef2daa..fddaf0d 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -169,12 +169,12 @@ enum kvm_bus {
>  	KVM_NR_BUSES
>  };
>  
> -int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		     int len, const void *val);
> -int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> -			    int len, const void *val, long cookie);
> -int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, int len,
> -		    void *val);
> +int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
> +			    gpa_t addr, int len, const void *val, long cookie);
> +int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
> +		    int len, void *val);
>  int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  			    int len, struct kvm_io_device *dev);
>  int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx,
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 00d8642..c831a40 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -60,8 +60,9 @@ static int coalesced_mmio_has_room(struct kvm_coalesced_mmio_dev *dev)
>  	return 1;
>  }
>  
> -static int coalesced_mmio_write(struct kvm_io_device *this,
> -				gpa_t addr, int len, const void *val)
> +static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
> +				struct kvm_io_device *this, gpa_t addr,
> +				int len, const void *val)
>  {
>  	struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
>  	struct kvm_coalesced_mmio_ring *ring = dev->kvm->coalesced_mmio_ring;
> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
> index 148b239..99994c8 100644
> --- a/virt/kvm/eventfd.c
> +++ b/virt/kvm/eventfd.c
> @@ -712,8 +712,8 @@ ioeventfd_in_range(struct _ioeventfd *p, gpa_t addr, int len, const void *val)
>  
>  /* MMIO/PIO writes trigger an event if the addr/val match */
>  static int
> -ioeventfd_write(struct kvm_io_device *this, gpa_t addr, int len,
> -		const void *val)
> +ioeventfd_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this, gpa_t addr,
> +		int len, const void *val)
>  {
>  	struct _ioeventfd *p = to_ioeventfd(this);
>  
> diff --git a/virt/kvm/iodev.h b/virt/kvm/iodev.h
> index 12fd3ca..9ef709c 100644
> --- a/virt/kvm/iodev.h
> +++ b/virt/kvm/iodev.h
> @@ -20,6 +20,7 @@
>  #include <asm/errno.h>
>  
>  struct kvm_io_device;
> +struct kvm_vcpu;
>  
>  /**
>   * kvm_io_device_ops are called under kvm slots_lock.
> @@ -27,11 +28,13 @@ struct kvm_io_device;
>   * or non-zero to have it passed to the next device.
>   **/
>  struct kvm_io_device_ops {
> -	int (*read)(struct kvm_io_device *this,
> +	int (*read)(struct kvm_vcpu *vcpu,
> +		    struct kvm_io_device *this,
>  		    gpa_t addr,
>  		    int len,
>  		    void *val);
> -	int (*write)(struct kvm_io_device *this,
> +	int (*write)(struct kvm_vcpu *vcpu,
> +		     struct kvm_io_device *this,
>  		     gpa_t addr,
>  		     int len,
>  		     const void *val);
> @@ -49,16 +52,20 @@ static inline void kvm_iodevice_init(struct kvm_io_device *dev,
>  	dev->ops = ops;
>  }
>  
> -static inline int kvm_iodevice_read(struct kvm_io_device *dev,
> -				    gpa_t addr, int l, void *v)
> +static inline int kvm_iodevice_read(struct kvm_vcpu *vcpu,
> +				    struct kvm_io_device *dev, gpa_t addr,
> +				    int l, void *v)
>  {
> -	return dev->ops->read ? dev->ops->read(dev, addr, l, v) : -EOPNOTSUPP;
> +	return dev->ops->read ? dev->ops->read(vcpu, dev, addr, l, v)
> +				: -EOPNOTSUPP;
>  }
>  
> -static inline int kvm_iodevice_write(struct kvm_io_device *dev,
> -				     gpa_t addr, int l, const void *v)
> +static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu,
> +				     struct kvm_io_device *dev, gpa_t addr,
> +				     int l, const void *v)
>  {
> -	return dev->ops->write ? dev->ops->write(dev, addr, l, v) : -EOPNOTSUPP;
> +	return dev->ops->write ? dev->ops->write(vcpu, dev, addr, l, v)
> +				 : -EOPNOTSUPP;
>  }
>  
>  static inline void kvm_iodevice_destructor(struct kvm_io_device *dev)
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 246cf29..60856d3 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3004,7 +3004,7 @@ static int kvm_io_bus_get_first_dev(struct kvm_io_bus *bus,
>  	return off;
>  }
>  
> -static int __kvm_io_bus_write(struct kvm_io_bus *bus,
> +static int __kvm_io_bus_write(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
>  			      struct kvm_io_range *range, const void *val)
>  {
>  	int idx;
> @@ -3015,7 +3015,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
>  
>  	while (idx < bus->dev_count &&
>  		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
> -		if (!kvm_iodevice_write(bus->range[idx].dev, range->addr,
> +		if (!kvm_iodevice_write(vcpu, bus->range[idx].dev, range->addr,
>  					range->len, val))
>  			return idx;
>  		idx++;
> @@ -3025,7 +3025,7 @@ static int __kvm_io_bus_write(struct kvm_io_bus *bus,
>  }
>  
>  /* kvm_io_bus_write - called under kvm->slots_lock */
> -int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_write(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		     int len, const void *val)
>  {
>  	struct kvm_io_bus *bus;
> @@ -3037,14 +3037,14 @@ int kvm_io_bus_write(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> -	r = __kvm_io_bus_write(bus, &range, val);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
> +	r = __kvm_io_bus_write(vcpu, bus, &range, val);
>  	return r < 0 ? r : 0;
>  }
>  
>  /* kvm_io_bus_write_cookie - called under kvm->slots_lock */
> -int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> -			    int len, const void *val, long cookie)
> +int kvm_io_bus_write_cookie(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx,
> +			    gpa_t addr, int len, const void *val, long cookie)
>  {
>  	struct kvm_io_bus *bus;
>  	struct kvm_io_range range;
> @@ -3054,12 +3054,12 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
>  
>  	/* First try the device referenced by cookie. */
>  	if ((cookie >= 0) && (cookie < bus->dev_count) &&
>  	    (kvm_io_bus_cmp(&range, &bus->range[cookie]) == 0))
> -		if (!kvm_iodevice_write(bus->range[cookie].dev, addr, len,
> +		if (!kvm_iodevice_write(vcpu, bus->range[cookie].dev, addr, len,
>  					val))
>  			return cookie;
>  
> @@ -3067,11 +3067,11 @@ int kvm_io_bus_write_cookie(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  	 * cookie contained garbage; fall back to search and return the
>  	 * correct cookie value.
>  	 */
> -	return __kvm_io_bus_write(bus, &range, val);
> +	return __kvm_io_bus_write(vcpu, bus, &range, val);
>  }
>  
> -static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
> -			     void *val)
> +static int __kvm_io_bus_read(struct kvm_vcpu *vcpu, struct kvm_io_bus *bus,
> +			     struct kvm_io_range *range, void *val)
>  {
>  	int idx;
>  
> @@ -3081,7 +3081,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
>  
>  	while (idx < bus->dev_count &&
>  		kvm_io_bus_cmp(range, &bus->range[idx]) == 0) {
> -		if (!kvm_iodevice_read(bus->range[idx].dev, range->addr,
> +		if (!kvm_iodevice_read(vcpu, bus->range[idx].dev, range->addr,
>  				       range->len, val))
>  			return idx;
>  		idx++;
> @@ -3092,7 +3092,7 @@ static int __kvm_io_bus_read(struct kvm_io_bus *bus, struct kvm_io_range *range,
>  EXPORT_SYMBOL_GPL(kvm_io_bus_write);
>  
>  /* kvm_io_bus_read - called under kvm->slots_lock */
> -int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
> +int kvm_io_bus_read(struct kvm_vcpu *vcpu, enum kvm_bus bus_idx, gpa_t addr,
>  		    int len, void *val)
>  {
>  	struct kvm_io_bus *bus;
> @@ -3104,8 +3104,8 @@ int kvm_io_bus_read(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr,
>  		.len = len,
>  	};
>  
> -	bus = srcu_dereference(kvm->buses[bus_idx], &kvm->srcu);
> -	r = __kvm_io_bus_read(bus, &range, val);
> +	bus = srcu_dereference(vcpu->kvm->buses[bus_idx], &kvm->srcu);
> +	r = __kvm_io_bus_read(vcpu, bus, &range, val);
>  	return r < 0 ? r : 0;
>  }
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-24 11:59   ` Nikolay Nikolaev
@ 2015-01-27 13:31     ` Andre Przywara
  -1 siblings, 0 replies; 26+ messages in thread
From: Andre Przywara @ 2015-01-27 13:31 UTC (permalink / raw)
  To: Nikolay Nikolaev, kvm, eric.auger, marc.zyngier, kvmarm,
	christoffer.dall
  Cc: tech, linux-arm-kernel

Hi Nikolay,

On 24/01/15 11:59, Nikolay Nikolaev wrote:
> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> a single MMIO handling path - that is through the kvm_io_bus_ API.
> 
> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> Both read and write calls are redirected to vgic_io_dev_access where
> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> 
> 
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  arch/arm/kvm/mmio.c    |    3 -
>  include/kvm/arm_vgic.h |    3 -
>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 114 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index d852137..8dc2fde 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  			       fault_ipa, 0);
>  	}
>  
> -	if (vgic_handle_mmio(vcpu, run, &mmio))
> -		return 1;
> -

Why is this (whole patch) actually needed? Is that just to make it nicer
by pulling everything under one umbrella?
For enabling ioeventfd you actually don't need this patch, right?
(I am asking because this breaks GICv3 emulation, see below)

>  	if (handle_kernel_mmio(vcpu, run, &mmio))
>  		return 1;
>  
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7c55dd5..60639b1 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -237,6 +237,7 @@ struct vgic_dist {
>  	unsigned long		*irq_pending_on_cpu;
>  
>  	struct vgic_vm_ops	vm_ops;
> +	struct kvm_io_device	*io_dev;
>  #endif
>  };
>  
> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -		      struct kvm_exit_mmio *mmio);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 0cc6ab6..195d2ba 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -31,6 +31,9 @@
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_mmu.h>
> +#include <asm/kvm.h>
> +
> +#include "iodev.h"
>  
>  /*
>   * How the whole thing works (courtesy of Christoffer Dall):
> @@ -77,6 +80,7 @@
>  
>  #include "vgic.h"
>  
> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  
>  int kvm_vgic_map_resources(struct kvm *kvm)
>  {
> +	vgic_register_kvm_io_dev(kvm);
>  	return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>  }
>  
> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  }
>  
>  /**
> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>   * @vcpu:      pointer to the vcpu performing the access
> - * @run:       pointer to the kvm_run structure
> - * @mmio:      pointer to the data describing the access
> + * @this:      pointer to the kvm_io_device structure
> + * @addr:      the MMIO address being accessed
> + * @len:       the length of the accessed data
> + * @val:       pointer to the value being written,
> + *             or where the read operation will store its result
> + * @is_write:  flag to show whether a write access is performed
>   *
> - * returns true if the MMIO access has been performed in kernel space,
> - * and false if it needs to be emulated in user space.
> + * returns 0 if the MMIO access has been performed in kernel space,
> + * and 1 if it needs to be emulated in user space.
>   * Calls the actual handling routine for the selected VGIC model.
>   */
> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -		      struct kvm_exit_mmio *mmio)
> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			    gpa_t addr, int len, void *val, bool is_write)
>  {
> -	if (!irqchip_in_kernel(vcpu->kvm))
> -		return false;
> +	struct kvm_exit_mmio mmio;
> +	bool ret;
> +
> +	mmio = (struct kvm_exit_mmio) {
> +		.phys_addr = addr,
> +		.len = len,
> +		.is_write = is_write,
> +	};
> +
> +	if (is_write)
> +		memcpy(mmio.data, val, len);
>  
>  	/*
>  	 * This will currently call either vgic_v2_handle_mmio() or
>  	 * vgic_v3_handle_mmio(), which in turn will call
>  	 * vgic_handle_mmio_range() defined above.
>  	 */
> -	return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
> +	ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
> +
> +	if (!is_write)
> +		memcpy(val, mmio.data, len);
> +
> +	return ret ? 0 : 1;
> +}
> +
> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			  gpa_t addr, int len, void *val)
> +{
> +	return vgic_io_dev_access(vcpu, this, addr, len, val, false);
> +}
> +
> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			   gpa_t addr, int len, const void *val)
> +{
> +	return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
> +}
> +
> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
> +	.read       = vgic_io_dev_read,
> +	.write      = vgic_io_dev_write,
> +};
> +
> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
> +{
> +	int len = 0;
> +	int ret;
> +
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +	unsigned long base = dist->vgic_dist_base;
> +	u32 type = kvm->arch.vgic.vgic_model;
> +	struct kvm_io_device *dev;
> +
> +	if (IS_VGIC_ADDR_UNDEF(base)) {
> +		kvm_err("Need to set vgic distributor address first\n");
> +		return -ENXIO;
> +	}
> +
> +	dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	switch (type) {
> +	case KVM_DEV_TYPE_ARM_VGIC_V2:
> +		len = KVM_VGIC_V2_DIST_SIZE;
> +		break;
> +#ifdef CONFIG_ARM_GIC_V3
> +	case KVM_DEV_TYPE_ARM_VGIC_V3:
> +		len = KVM_VGIC_V3_DIST_SIZE;
> +		break;
> +#endif
> +	}

But this only registers the GIC distributor, leaving out the
redistributor regions introduced by GICv3. To me it looks like this
kvm_iodevice registration code should be moved into *-emul.c, where each
emulated device registers what it needs.
Especially in the wake of the upcoming v2M/ITS emulation I think we need
a proper solution for this, so I am wondering if we could just leave
that patch out (at least for now) and keep the two-line special
treatment for the VGIC above in.
That should enable ioeventfd without breaking the VGIC.

Cheers,
Andre.

> +
> +	kvm_iodevice_init(dev, &vgic_io_dev_ops);
> +
> +	mutex_lock(&kvm->slots_lock);
> +
> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
> +			base, len, dev);
> +	if (ret < 0)
> +		goto out_unlock;
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	kvm->arch.vgic.io_dev = dev;
> +
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&kvm->slots_lock);
> +	kfree(dev);
> +	return ret;
> +}
> +
> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
> +{
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +
> +	if (dist) {
> +		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
> +		kfree(dist->io_dev);
> +		dist->io_dev = NULL;
> +	}
>  }
>  
>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	int i;
>  
> +	vgic_unregister_kvm_io_dev(kvm);
> +
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_vgic_vcpu_destroy(vcpu);
>  
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-27 13:31     ` Andre Przywara
  0 siblings, 0 replies; 26+ messages in thread
From: Andre Przywara @ 2015-01-27 13:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Nikolay,

On 24/01/15 11:59, Nikolay Nikolaev wrote:
> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> a single MMIO handling path - that is through the kvm_io_bus_ API.
> 
> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> Both read and write calls are redirected to vgic_io_dev_access where
> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> 
> 
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  arch/arm/kvm/mmio.c    |    3 -
>  include/kvm/arm_vgic.h |    3 -
>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 114 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> index d852137..8dc2fde 100644
> --- a/arch/arm/kvm/mmio.c
> +++ b/arch/arm/kvm/mmio.c
> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  			       fault_ipa, 0);
>  	}
>  
> -	if (vgic_handle_mmio(vcpu, run, &mmio))
> -		return 1;
> -

Why is this (whole patch) actually needed? Is that just to make it nicer
by pulling everything under one umbrella?
For enabling ioeventfd you actually don't need this patch, right?
(I am asking because this breaks GICv3 emulation, see below)

>  	if (handle_kernel_mmio(vcpu, run, &mmio))
>  		return 1;
>  
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7c55dd5..60639b1 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -237,6 +237,7 @@ struct vgic_dist {
>  	unsigned long		*irq_pending_on_cpu;
>  
>  	struct vgic_vm_ops	vm_ops;
> +	struct kvm_io_device	*io_dev;
>  #endif
>  };
>  
> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -		      struct kvm_exit_mmio *mmio);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 0cc6ab6..195d2ba 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -31,6 +31,9 @@
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_arm.h>
>  #include <asm/kvm_mmu.h>
> +#include <asm/kvm.h>
> +
> +#include "iodev.h"
>  
>  /*
>   * How the whole thing works (courtesy of Christoffer Dall):
> @@ -77,6 +80,7 @@
>  
>  #include "vgic.h"
>  
> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>  
>  int kvm_vgic_map_resources(struct kvm *kvm)
>  {
> +	vgic_register_kvm_io_dev(kvm);
>  	return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>  }
>  
> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  }
>  
>  /**
> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>   * @vcpu:      pointer to the vcpu performing the access
> - * @run:       pointer to the kvm_run structure
> - * @mmio:      pointer to the data describing the access
> + * @this:      pointer to the kvm_io_device structure
> + * @addr:      the MMIO address being accessed
> + * @len:       the length of the accessed data
> + * @val:       pointer to the value being written,
> + *             or where the read operation will store its result
> + * @is_write:  flag to show whether a write access is performed
>   *
> - * returns true if the MMIO access has been performed in kernel space,
> - * and false if it needs to be emulated in user space.
> + * returns 0 if the MMIO access has been performed in kernel space,
> + * and 1 if it needs to be emulated in user space.
>   * Calls the actual handling routine for the selected VGIC model.
>   */
> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> -		      struct kvm_exit_mmio *mmio)
> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			    gpa_t addr, int len, void *val, bool is_write)
>  {
> -	if (!irqchip_in_kernel(vcpu->kvm))
> -		return false;
> +	struct kvm_exit_mmio mmio;
> +	bool ret;
> +
> +	mmio = (struct kvm_exit_mmio) {
> +		.phys_addr = addr,
> +		.len = len,
> +		.is_write = is_write,
> +	};
> +
> +	if (is_write)
> +		memcpy(mmio.data, val, len);
>  
>  	/*
>  	 * This will currently call either vgic_v2_handle_mmio() or
>  	 * vgic_v3_handle_mmio(), which in turn will call
>  	 * vgic_handle_mmio_range() defined above.
>  	 */
> -	return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
> +	ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
> +
> +	if (!is_write)
> +		memcpy(val, mmio.data, len);
> +
> +	return ret ? 0 : 1;
> +}
> +
> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			  gpa_t addr, int len, void *val)
> +{
> +	return vgic_io_dev_access(vcpu, this, addr, len, val, false);
> +}
> +
> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			   gpa_t addr, int len, const void *val)
> +{
> +	return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
> +}
> +
> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
> +	.read       = vgic_io_dev_read,
> +	.write      = vgic_io_dev_write,
> +};
> +
> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
> +{
> +	int len = 0;
> +	int ret;
> +
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +	unsigned long base = dist->vgic_dist_base;
> +	u32 type = kvm->arch.vgic.vgic_model;
> +	struct kvm_io_device *dev;
> +
> +	if (IS_VGIC_ADDR_UNDEF(base)) {
> +		kvm_err("Need to set vgic distributor address first\n");
> +		return -ENXIO;
> +	}
> +
> +	dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	switch (type) {
> +	case KVM_DEV_TYPE_ARM_VGIC_V2:
> +		len = KVM_VGIC_V2_DIST_SIZE;
> +		break;
> +#ifdef CONFIG_ARM_GIC_V3
> +	case KVM_DEV_TYPE_ARM_VGIC_V3:
> +		len = KVM_VGIC_V3_DIST_SIZE;
> +		break;
> +#endif
> +	}

But this only registers the GIC distributor, leaving out the
redistributor regions introduced by GICv3. To me it looks like this
kvm_iodevice registration code should be moved into *-emul.c, where each
emulated device registers what it needs.
Especially in the wake of the upcoming v2M/ITS emulation I think we need
a proper solution for this, so I am wondering if we could just leave
that patch out (at least for now) and keep the two-line special
treatment for the VGIC above in.
That should enable ioeventfd without breaking the VGIC.

Cheers,
Andre.

> +
> +	kvm_iodevice_init(dev, &vgic_io_dev_ops);
> +
> +	mutex_lock(&kvm->slots_lock);
> +
> +	ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
> +			base, len, dev);
> +	if (ret < 0)
> +		goto out_unlock;
> +	mutex_unlock(&kvm->slots_lock);
> +
> +	kvm->arch.vgic.io_dev = dev;
> +
> +	return 0;
> +
> +out_unlock:
> +	mutex_unlock(&kvm->slots_lock);
> +	kfree(dev);
> +	return ret;
> +}
> +
> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
> +{
> +	struct vgic_dist *dist = &kvm->arch.vgic;
> +
> +	if (dist) {
> +		kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
> +		kfree(dist->io_dev);
> +		dist->io_dev = NULL;
> +	}
>  }
>  
>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>  	struct kvm_vcpu *vcpu;
>  	int i;
>  
> +	vgic_unregister_kvm_io_dev(kvm);
> +
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_vgic_vcpu_destroy(vcpu);
>  
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-27 13:31     ` Andre Przywara
@ 2015-01-27 16:51       ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-27 16:51 UTC (permalink / raw)
  To: Andre Przywara
  Cc: open list:KERNEL VIRTUAL MA...,
	Eric Auger, Marc Zyngier, kvmarm, Christoffer Dall,
	VirtualOpenSystems Technical Team, moderated list:ARM PORT

Hi Andre,

On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi Nikolay,
>
> On 24/01/15 11:59, Nikolay Nikolaev wrote:
> > In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> > a single MMIO handling path - that is through the kvm_io_bus_ API.
> >
> > Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> > Both read and write calls are redirected to vgic_io_dev_access where
> > kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> >
> >
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
> >  arch/arm/kvm/mmio.c    |    3 -
> >  include/kvm/arm_vgic.h |    3 -
> >  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 114 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> > index d852137..8dc2fde 100644
> > --- a/arch/arm/kvm/mmio.c
> > +++ b/arch/arm/kvm/mmio.c
> > @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >                              fault_ipa, 0);
> >       }
> >
> > -     if (vgic_handle_mmio(vcpu, run, &mmio))
> > -             return 1;
> > -
>
> Why is this (whole patch) actually needed? Is that just to make it nicer
> by pulling everything under one umbrella?


It started from this mail form Christofer:
https://lkml.org/lkml/2014/3/28/403

>
> For enabling ioeventfd you actually don't need this patch, right?
Yes, we don't need it.
> (I am asking because this breaks GICv3 emulation, see below)
>
> >       if (handle_kernel_mmio(vcpu, run, &mmio))
> >               return 1;
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 7c55dd5..60639b1 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -237,6 +237,7 @@ struct vgic_dist {
> >       unsigned long           *irq_pending_on_cpu;
> >
> >       struct vgic_vm_ops      vm_ops;
> > +     struct kvm_io_device    *io_dev;
> >  #endif
> >  };
> >
> > @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >                       bool level);
> >  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> >  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> > -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > -                   struct kvm_exit_mmio *mmio);
> >
> >  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
> >  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 0cc6ab6..195d2ba 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -31,6 +31,9 @@
> >  #include <asm/kvm_emulate.h>
> >  #include <asm/kvm_arm.h>
> >  #include <asm/kvm_mmu.h>
> > +#include <asm/kvm.h>
> > +
> > +#include "iodev.h"
> >
> >  /*
> >   * How the whole thing works (courtesy of Christoffer Dall):
> > @@ -77,6 +80,7 @@
> >
> >  #include "vgic.h"
> >
> > +static int vgic_register_kvm_io_dev(struct kvm *kvm);
> >  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
> >  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
> >  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
> > @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
> >
> >  int kvm_vgic_map_resources(struct kvm *kvm)
> >  {
> > +     vgic_register_kvm_io_dev(kvm);
> >       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
> >  }
> >
> > @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >  }
> >
> >  /**
> > - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
> > + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
> >   * @vcpu:      pointer to the vcpu performing the access
> > - * @run:       pointer to the kvm_run structure
> > - * @mmio:      pointer to the data describing the access
> > + * @this:      pointer to the kvm_io_device structure
> > + * @addr:      the MMIO address being accessed
> > + * @len:       the length of the accessed data
> > + * @val:       pointer to the value being written,
> > + *             or where the read operation will store its result
> > + * @is_write:  flag to show whether a write access is performed
> >   *
> > - * returns true if the MMIO access has been performed in kernel space,
> > - * and false if it needs to be emulated in user space.
> > + * returns 0 if the MMIO access has been performed in kernel space,
> > + * and 1 if it needs to be emulated in user space.
> >   * Calls the actual handling routine for the selected VGIC model.
> >   */
> > -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > -                   struct kvm_exit_mmio *mmio)
> > +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                         gpa_t addr, int len, void *val, bool is_write)
> >  {
> > -     if (!irqchip_in_kernel(vcpu->kvm))
> > -             return false;
> > +     struct kvm_exit_mmio mmio;
> > +     bool ret;
> > +
> > +     mmio = (struct kvm_exit_mmio) {
> > +             .phys_addr = addr,
> > +             .len = len,
> > +             .is_write = is_write,
> > +     };
> > +
> > +     if (is_write)
> > +             memcpy(mmio.data, val, len);
> >
> >       /*
> >        * This will currently call either vgic_v2_handle_mmio() or
> >        * vgic_v3_handle_mmio(), which in turn will call
> >        * vgic_handle_mmio_range() defined above.
> >        */
> > -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
> > +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
> > +
> > +     if (!is_write)
> > +             memcpy(val, mmio.data, len);
> > +
> > +     return ret ? 0 : 1;
> > +}
> > +
> > +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                       gpa_t addr, int len, void *val)
> > +{
> > +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
> > +}
> > +
> > +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                        gpa_t addr, int len, const void *val)
> > +{
> > +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
> > +}
> > +
> > +static const struct kvm_io_device_ops vgic_io_dev_ops = {
> > +     .read       = vgic_io_dev_read,
> > +     .write      = vgic_io_dev_write,
> > +};
> > +
> > +static int vgic_register_kvm_io_dev(struct kvm *kvm)
> > +{
> > +     int len = 0;
> > +     int ret;
> > +
> > +     struct vgic_dist *dist = &kvm->arch.vgic;
> > +     unsigned long base = dist->vgic_dist_base;
> > +     u32 type = kvm->arch.vgic.vgic_model;
> > +     struct kvm_io_device *dev;
> > +
> > +     if (IS_VGIC_ADDR_UNDEF(base)) {
> > +             kvm_err("Need to set vgic distributor address first\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +
> > +     switch (type) {
> > +     case KVM_DEV_TYPE_ARM_VGIC_V2:
> > +             len = KVM_VGIC_V2_DIST_SIZE;
> > +             break;
> > +#ifdef CONFIG_ARM_GIC_V3
> > +     case KVM_DEV_TYPE_ARM_VGIC_V3:
> > +             len = KVM_VGIC_V3_DIST_SIZE;
> > +             break;
> > +#endif
> > +     }
>
> But this only registers the GIC distributor, leaving out the
> redistributor regions introduced by GICv3. To me it looks like this
I see GICv3 needs more work.

> kvm_iodevice registration code should be moved into *-emul.c, where each
> emulated device registers what it needs.
> Especially in the wake of the upcoming v2M/ITS emulation I think we need
> a proper solution for this, so I am wondering if we could just leave
> that patch out (at least for now) and keep the two-line special
> treatment for the VGIC above in.
> That should enable ioeventfd without breaking the VGIC.
Then we're back to the original RFC patch series.
I have no issues droppin this one (and propably patch 1 in the series)
and leaving only the eventfd related handling.
I just need some consensus/confirmation on the mailing list.

regards,
Nikolay Nikolaev

>
> Cheers,
> Andre.
>
> > +
> > +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
> > +
> > +     mutex_lock(&kvm->slots_lock);
> > +
> > +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
> > +                     base, len, dev);
> > +     if (ret < 0)
> > +             goto out_unlock;
> > +     mutex_unlock(&kvm->slots_lock);
> > +
> > +     kvm->arch.vgic.io_dev = dev;
> > +
> > +     return 0;
> > +
> > +out_unlock:
> > +     mutex_unlock(&kvm->slots_lock);
> > +     kfree(dev);
> > +     return ret;
> > +}
> > +
> > +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
> > +{
> > +     struct vgic_dist *dist = &kvm->arch.vgic;
> > +
> > +     if (dist) {
> > +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
> > +             kfree(dist->io_dev);
> > +             dist->io_dev = NULL;
> > +     }
> >  }
> >
> >  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
> > @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
> >       struct kvm_vcpu *vcpu;
> >       int i;
> >
> > +     vgic_unregister_kvm_io_dev(kvm);
> > +
> >       kvm_for_each_vcpu(i, vcpu, kvm)
> >               kvm_vgic_vcpu_destroy(vcpu);
> >
> >
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> >

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-27 16:51       ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-27 16:51 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Andre,

On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi Nikolay,
>
> On 24/01/15 11:59, Nikolay Nikolaev wrote:
> > In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> > a single MMIO handling path - that is through the kvm_io_bus_ API.
> >
> > Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> > Both read and write calls are redirected to vgic_io_dev_access where
> > kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> >
> >
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
> >  arch/arm/kvm/mmio.c    |    3 -
> >  include/kvm/arm_vgic.h |    3 -
> >  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 114 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> > index d852137..8dc2fde 100644
> > --- a/arch/arm/kvm/mmio.c
> > +++ b/arch/arm/kvm/mmio.c
> > @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >                              fault_ipa, 0);
> >       }
> >
> > -     if (vgic_handle_mmio(vcpu, run, &mmio))
> > -             return 1;
> > -
>
> Why is this (whole patch) actually needed? Is that just to make it nicer
> by pulling everything under one umbrella?


It started from this mail form Christofer:
https://lkml.org/lkml/2014/3/28/403

>
> For enabling ioeventfd you actually don't need this patch, right?
Yes, we don't need it.
> (I am asking because this breaks GICv3 emulation, see below)
>
> >       if (handle_kernel_mmio(vcpu, run, &mmio))
> >               return 1;
> >
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 7c55dd5..60639b1 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -237,6 +237,7 @@ struct vgic_dist {
> >       unsigned long           *irq_pending_on_cpu;
> >
> >       struct vgic_vm_ops      vm_ops;
> > +     struct kvm_io_device    *io_dev;
> >  #endif
> >  };
> >
> > @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >                       bool level);
> >  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> >  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> > -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > -                   struct kvm_exit_mmio *mmio);
> >
> >  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
> >  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 0cc6ab6..195d2ba 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -31,6 +31,9 @@
> >  #include <asm/kvm_emulate.h>
> >  #include <asm/kvm_arm.h>
> >  #include <asm/kvm_mmu.h>
> > +#include <asm/kvm.h>
> > +
> > +#include "iodev.h"
> >
> >  /*
> >   * How the whole thing works (courtesy of Christoffer Dall):
> > @@ -77,6 +80,7 @@
> >
> >  #include "vgic.h"
> >
> > +static int vgic_register_kvm_io_dev(struct kvm *kvm);
> >  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
> >  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
> >  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
> > @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
> >
> >  int kvm_vgic_map_resources(struct kvm *kvm)
> >  {
> > +     vgic_register_kvm_io_dev(kvm);
> >       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
> >  }
> >
> > @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >  }
> >
> >  /**
> > - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
> > + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
> >   * @vcpu:      pointer to the vcpu performing the access
> > - * @run:       pointer to the kvm_run structure
> > - * @mmio:      pointer to the data describing the access
> > + * @this:      pointer to the kvm_io_device structure
> > + * @addr:      the MMIO address being accessed
> > + * @len:       the length of the accessed data
> > + * @val:       pointer to the value being written,
> > + *             or where the read operation will store its result
> > + * @is_write:  flag to show whether a write access is performed
> >   *
> > - * returns true if the MMIO access has been performed in kernel space,
> > - * and false if it needs to be emulated in user space.
> > + * returns 0 if the MMIO access has been performed in kernel space,
> > + * and 1 if it needs to be emulated in user space.
> >   * Calls the actual handling routine for the selected VGIC model.
> >   */
> > -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > -                   struct kvm_exit_mmio *mmio)
> > +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                         gpa_t addr, int len, void *val, bool is_write)
> >  {
> > -     if (!irqchip_in_kernel(vcpu->kvm))
> > -             return false;
> > +     struct kvm_exit_mmio mmio;
> > +     bool ret;
> > +
> > +     mmio = (struct kvm_exit_mmio) {
> > +             .phys_addr = addr,
> > +             .len = len,
> > +             .is_write = is_write,
> > +     };
> > +
> > +     if (is_write)
> > +             memcpy(mmio.data, val, len);
> >
> >       /*
> >        * This will currently call either vgic_v2_handle_mmio() or
> >        * vgic_v3_handle_mmio(), which in turn will call
> >        * vgic_handle_mmio_range() defined above.
> >        */
> > -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
> > +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
> > +
> > +     if (!is_write)
> > +             memcpy(val, mmio.data, len);
> > +
> > +     return ret ? 0 : 1;
> > +}
> > +
> > +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                       gpa_t addr, int len, void *val)
> > +{
> > +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
> > +}
> > +
> > +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> > +                        gpa_t addr, int len, const void *val)
> > +{
> > +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
> > +}
> > +
> > +static const struct kvm_io_device_ops vgic_io_dev_ops = {
> > +     .read       = vgic_io_dev_read,
> > +     .write      = vgic_io_dev_write,
> > +};
> > +
> > +static int vgic_register_kvm_io_dev(struct kvm *kvm)
> > +{
> > +     int len = 0;
> > +     int ret;
> > +
> > +     struct vgic_dist *dist = &kvm->arch.vgic;
> > +     unsigned long base = dist->vgic_dist_base;
> > +     u32 type = kvm->arch.vgic.vgic_model;
> > +     struct kvm_io_device *dev;
> > +
> > +     if (IS_VGIC_ADDR_UNDEF(base)) {
> > +             kvm_err("Need to set vgic distributor address first\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
> > +     if (!dev)
> > +             return -ENOMEM;
> > +
> > +     switch (type) {
> > +     case KVM_DEV_TYPE_ARM_VGIC_V2:
> > +             len = KVM_VGIC_V2_DIST_SIZE;
> > +             break;
> > +#ifdef CONFIG_ARM_GIC_V3
> > +     case KVM_DEV_TYPE_ARM_VGIC_V3:
> > +             len = KVM_VGIC_V3_DIST_SIZE;
> > +             break;
> > +#endif
> > +     }
>
> But this only registers the GIC distributor, leaving out the
> redistributor regions introduced by GICv3. To me it looks like this
I see GICv3 needs more work.

> kvm_iodevice registration code should be moved into *-emul.c, where each
> emulated device registers what it needs.
> Especially in the wake of the upcoming v2M/ITS emulation I think we need
> a proper solution for this, so I am wondering if we could just leave
> that patch out (at least for now) and keep the two-line special
> treatment for the VGIC above in.
> That should enable ioeventfd without breaking the VGIC.
Then we're back to the original RFC patch series.
I have no issues droppin this one (and propably patch 1 in the series)
and leaving only the eventfd related handling.
I just need some consensus/confirmation on the mailing list.

regards,
Nikolay Nikolaev

>
> Cheers,
> Andre.
>
> > +
> > +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
> > +
> > +     mutex_lock(&kvm->slots_lock);
> > +
> > +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
> > +                     base, len, dev);
> > +     if (ret < 0)
> > +             goto out_unlock;
> > +     mutex_unlock(&kvm->slots_lock);
> > +
> > +     kvm->arch.vgic.io_dev = dev;
> > +
> > +     return 0;
> > +
> > +out_unlock:
> > +     mutex_unlock(&kvm->slots_lock);
> > +     kfree(dev);
> > +     return ret;
> > +}
> > +
> > +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
> > +{
> > +     struct vgic_dist *dist = &kvm->arch.vgic;
> > +
> > +     if (dist) {
> > +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
> > +             kfree(dist->io_dev);
> > +             dist->io_dev = NULL;
> > +     }
> >  }
> >
> >  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
> > @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
> >       struct kvm_vcpu *vcpu;
> >       int i;
> >
> > +     vgic_unregister_kvm_io_dev(kvm);
> > +
> >       kvm_for_each_vcpu(i, vcpu, kvm)
> >               kvm_vgic_vcpu_destroy(vcpu);
> >
> >
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm at lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> >

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-27 16:51       ` Nikolay Nikolaev
@ 2015-01-27 17:26         ` Eric Auger
  -1 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2015-01-27 17:26 UTC (permalink / raw)
  To: Nikolay Nikolaev, Andre Przywara
  Cc: open list:KERNEL VIRTUAL MA...,
	Marc Zyngier, ARM PORT, VirtualOpenSystems Technical Team,
	kvmarm, Christoffer Dall

On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
> Hi Andre,
> 
> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Hi Nikolay,
>>
>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>
>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>> Both read and write calls are redirected to vgic_io_dev_access where
>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>
>>>
>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>> ---
>>>  arch/arm/kvm/mmio.c    |    3 -
>>>  include/kvm/arm_vgic.h |    3 -
>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>> index d852137..8dc2fde 100644
>>> --- a/arch/arm/kvm/mmio.c
>>> +++ b/arch/arm/kvm/mmio.c
>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>                              fault_ipa, 0);
>>>       }
>>>
>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>> -             return 1;
>>> -
>>
>> Why is this (whole patch) actually needed? Is that just to make it nicer
>> by pulling everything under one umbrella?
> 
> 
> It started from this mail form Christofer:
> https://lkml.org/lkml/2014/3/28/403
Hi Nikolay, Andre,

I also understood that the target was to handle all kernel mmio through
the same API, hence the first patch. This patch shows that at least for
GICv2 it was doable without upheavals in vgic code and it also serves
ioeventd which is good. Andre do you think the price to pay to integrate
missing redistributors and forthcoming components is too high?

Best Regards

Eric


> 
>>
>> For enabling ioeventfd you actually don't need this patch, right?
> Yes, we don't need it.
>> (I am asking because this breaks GICv3 emulation, see below)
>>
>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>               return 1;
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 7c55dd5..60639b1 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>       unsigned long           *irq_pending_on_cpu;
>>>
>>>       struct vgic_vm_ops      vm_ops;
>>> +     struct kvm_io_device    *io_dev;
>>>  #endif
>>>  };
>>>
>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>                       bool level);
>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>> -                   struct kvm_exit_mmio *mmio);
>>>
>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index 0cc6ab6..195d2ba 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -31,6 +31,9 @@
>>>  #include <asm/kvm_emulate.h>
>>>  #include <asm/kvm_arm.h>
>>>  #include <asm/kvm_mmu.h>
>>> +#include <asm/kvm.h>
>>> +
>>> +#include "iodev.h"
>>>
>>>  /*
>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>> @@ -77,6 +80,7 @@
>>>
>>>  #include "vgic.h"
>>>
>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>
>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>  {
>>> +     vgic_register_kvm_io_dev(kvm);
>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>  }
>>>
>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>  }
>>>
>>>  /**
>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>   * @vcpu:      pointer to the vcpu performing the access
>>> - * @run:       pointer to the kvm_run structure
>>> - * @mmio:      pointer to the data describing the access
>>> + * @this:      pointer to the kvm_io_device structure
>>> + * @addr:      the MMIO address being accessed
>>> + * @len:       the length of the accessed data
>>> + * @val:       pointer to the value being written,
>>> + *             or where the read operation will store its result
>>> + * @is_write:  flag to show whether a write access is performed
>>>   *
>>> - * returns true if the MMIO access has been performed in kernel space,
>>> - * and false if it needs to be emulated in user space.
>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>> + * and 1 if it needs to be emulated in user space.
>>>   * Calls the actual handling routine for the selected VGIC model.
>>>   */
>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>> -                   struct kvm_exit_mmio *mmio)
>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>  {
>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>> -             return false;
>>> +     struct kvm_exit_mmio mmio;
>>> +     bool ret;
>>> +
>>> +     mmio = (struct kvm_exit_mmio) {
>>> +             .phys_addr = addr,
>>> +             .len = len,
>>> +             .is_write = is_write,
>>> +     };
>>> +
>>> +     if (is_write)
>>> +             memcpy(mmio.data, val, len);
>>>
>>>       /*
>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>        * vgic_handle_mmio_range() defined above.
>>>        */
>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>> +
>>> +     if (!is_write)
>>> +             memcpy(val, mmio.data, len);
>>> +
>>> +     return ret ? 0 : 1;
>>> +}
>>> +
>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                       gpa_t addr, int len, void *val)
>>> +{
>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>> +}
>>> +
>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                        gpa_t addr, int len, const void *val)
>>> +{
>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>> +}
>>> +
>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>> +     .read       = vgic_io_dev_read,
>>> +     .write      = vgic_io_dev_write,
>>> +};
>>> +
>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>> +{
>>> +     int len = 0;
>>> +     int ret;
>>> +
>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>> +     unsigned long base = dist->vgic_dist_base;
>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>> +     struct kvm_io_device *dev;
>>> +
>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>> +             kvm_err("Need to set vgic distributor address first\n");
>>> +             return -ENXIO;
>>> +     }
>>> +
>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>> +     if (!dev)
>>> +             return -ENOMEM;
>>> +
>>> +     switch (type) {
>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>> +             break;
>>> +#ifdef CONFIG_ARM_GIC_V3
>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>> +             break;
>>> +#endif
>>> +     }
>>
>> But this only registers the GIC distributor, leaving out the
>> redistributor regions introduced by GICv3. To me it looks like this
> I see GICv3 needs more work.
> 
>> kvm_iodevice registration code should be moved into *-emul.c, where each
>> emulated device registers what it needs.
>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>> a proper solution for this, so I am wondering if we could just leave
>> that patch out (at least for now) and keep the two-line special
>> treatment for the VGIC above in.
>> That should enable ioeventfd without breaking the VGIC.
> Then we're back to the original RFC patch series.
> I have no issues droppin this one (and propably patch 1 in the series)
> and leaving only the eventfd related handling.
> I just need some consensus/confirmation on the mailing list.
> 
> regards,
> Nikolay Nikolaev
> 
>>
>> Cheers,
>> Andre.
>>
>>> +
>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>> +
>>> +     mutex_lock(&kvm->slots_lock);
>>> +
>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>> +                     base, len, dev);
>>> +     if (ret < 0)
>>> +             goto out_unlock;
>>> +     mutex_unlock(&kvm->slots_lock);
>>> +
>>> +     kvm->arch.vgic.io_dev = dev;
>>> +
>>> +     return 0;
>>> +
>>> +out_unlock:
>>> +     mutex_unlock(&kvm->slots_lock);
>>> +     kfree(dev);
>>> +     return ret;
>>> +}
>>> +
>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>> +{
>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>> +
>>> +     if (dist) {
>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>> +             kfree(dist->io_dev);
>>> +             dist->io_dev = NULL;
>>> +     }
>>>  }
>>>
>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>       struct kvm_vcpu *vcpu;
>>>       int i;
>>>
>>> +     vgic_unregister_kvm_io_dev(kvm);
>>> +
>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>
>>>
>>> _______________________________________________
>>> kvmarm mailing list
>>> kvmarm@lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-27 17:26         ` Eric Auger
  0 siblings, 0 replies; 26+ messages in thread
From: Eric Auger @ 2015-01-27 17:26 UTC (permalink / raw)
  To: linux-arm-kernel

On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
> Hi Andre,
> 
> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> Hi Nikolay,
>>
>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>
>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>> Both read and write calls are redirected to vgic_io_dev_access where
>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>
>>>
>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>> ---
>>>  arch/arm/kvm/mmio.c    |    3 -
>>>  include/kvm/arm_vgic.h |    3 -
>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>> index d852137..8dc2fde 100644
>>> --- a/arch/arm/kvm/mmio.c
>>> +++ b/arch/arm/kvm/mmio.c
>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>                              fault_ipa, 0);
>>>       }
>>>
>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>> -             return 1;
>>> -
>>
>> Why is this (whole patch) actually needed? Is that just to make it nicer
>> by pulling everything under one umbrella?
> 
> 
> It started from this mail form Christofer:
> https://lkml.org/lkml/2014/3/28/403
Hi Nikolay, Andre,

I also understood that the target was to handle all kernel mmio through
the same API, hence the first patch. This patch shows that at least for
GICv2 it was doable without upheavals in vgic code and it also serves
ioeventd which is good. Andre do you think the price to pay to integrate
missing redistributors and forthcoming components is too high?

Best Regards

Eric


> 
>>
>> For enabling ioeventfd you actually don't need this patch, right?
> Yes, we don't need it.
>> (I am asking because this breaks GICv3 emulation, see below)
>>
>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>               return 1;
>>>
>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>> index 7c55dd5..60639b1 100644
>>> --- a/include/kvm/arm_vgic.h
>>> +++ b/include/kvm/arm_vgic.h
>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>       unsigned long           *irq_pending_on_cpu;
>>>
>>>       struct vgic_vm_ops      vm_ops;
>>> +     struct kvm_io_device    *io_dev;
>>>  #endif
>>>  };
>>>
>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>                       bool level);
>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>> -                   struct kvm_exit_mmio *mmio);
>>>
>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index 0cc6ab6..195d2ba 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -31,6 +31,9 @@
>>>  #include <asm/kvm_emulate.h>
>>>  #include <asm/kvm_arm.h>
>>>  #include <asm/kvm_mmu.h>
>>> +#include <asm/kvm.h>
>>> +
>>> +#include "iodev.h"
>>>
>>>  /*
>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>> @@ -77,6 +80,7 @@
>>>
>>>  #include "vgic.h"
>>>
>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>
>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>  {
>>> +     vgic_register_kvm_io_dev(kvm);
>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>  }
>>>
>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>  }
>>>
>>>  /**
>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>   * @vcpu:      pointer to the vcpu performing the access
>>> - * @run:       pointer to the kvm_run structure
>>> - * @mmio:      pointer to the data describing the access
>>> + * @this:      pointer to the kvm_io_device structure
>>> + * @addr:      the MMIO address being accessed
>>> + * @len:       the length of the accessed data
>>> + * @val:       pointer to the value being written,
>>> + *             or where the read operation will store its result
>>> + * @is_write:  flag to show whether a write access is performed
>>>   *
>>> - * returns true if the MMIO access has been performed in kernel space,
>>> - * and false if it needs to be emulated in user space.
>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>> + * and 1 if it needs to be emulated in user space.
>>>   * Calls the actual handling routine for the selected VGIC model.
>>>   */
>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>> -                   struct kvm_exit_mmio *mmio)
>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>  {
>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>> -             return false;
>>> +     struct kvm_exit_mmio mmio;
>>> +     bool ret;
>>> +
>>> +     mmio = (struct kvm_exit_mmio) {
>>> +             .phys_addr = addr,
>>> +             .len = len,
>>> +             .is_write = is_write,
>>> +     };
>>> +
>>> +     if (is_write)
>>> +             memcpy(mmio.data, val, len);
>>>
>>>       /*
>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>        * vgic_handle_mmio_range() defined above.
>>>        */
>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>> +
>>> +     if (!is_write)
>>> +             memcpy(val, mmio.data, len);
>>> +
>>> +     return ret ? 0 : 1;
>>> +}
>>> +
>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                       gpa_t addr, int len, void *val)
>>> +{
>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>> +}
>>> +
>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>> +                        gpa_t addr, int len, const void *val)
>>> +{
>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>> +}
>>> +
>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>> +     .read       = vgic_io_dev_read,
>>> +     .write      = vgic_io_dev_write,
>>> +};
>>> +
>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>> +{
>>> +     int len = 0;
>>> +     int ret;
>>> +
>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>> +     unsigned long base = dist->vgic_dist_base;
>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>> +     struct kvm_io_device *dev;
>>> +
>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>> +             kvm_err("Need to set vgic distributor address first\n");
>>> +             return -ENXIO;
>>> +     }
>>> +
>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>> +     if (!dev)
>>> +             return -ENOMEM;
>>> +
>>> +     switch (type) {
>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>> +             break;
>>> +#ifdef CONFIG_ARM_GIC_V3
>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>> +             break;
>>> +#endif
>>> +     }
>>
>> But this only registers the GIC distributor, leaving out the
>> redistributor regions introduced by GICv3. To me it looks like this
> I see GICv3 needs more work.
> 
>> kvm_iodevice registration code should be moved into *-emul.c, where each
>> emulated device registers what it needs.
>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>> a proper solution for this, so I am wondering if we could just leave
>> that patch out (at least for now) and keep the two-line special
>> treatment for the VGIC above in.
>> That should enable ioeventfd without breaking the VGIC.
> Then we're back to the original RFC patch series.
> I have no issues droppin this one (and propably patch 1 in the series)
> and leaving only the eventfd related handling.
> I just need some consensus/confirmation on the mailing list.
> 
> regards,
> Nikolay Nikolaev
> 
>>
>> Cheers,
>> Andre.
>>
>>> +
>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>> +
>>> +     mutex_lock(&kvm->slots_lock);
>>> +
>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>> +                     base, len, dev);
>>> +     if (ret < 0)
>>> +             goto out_unlock;
>>> +     mutex_unlock(&kvm->slots_lock);
>>> +
>>> +     kvm->arch.vgic.io_dev = dev;
>>> +
>>> +     return 0;
>>> +
>>> +out_unlock:
>>> +     mutex_unlock(&kvm->slots_lock);
>>> +     kfree(dev);
>>> +     return ret;
>>> +}
>>> +
>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>> +{
>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>> +
>>> +     if (dist) {
>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>> +             kfree(dist->io_dev);
>>> +             dist->io_dev = NULL;
>>> +     }
>>>  }
>>>
>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>       struct kvm_vcpu *vcpu;
>>>       int i;
>>>
>>> +     vgic_unregister_kvm_io_dev(kvm);
>>> +
>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>
>>>
>>> _______________________________________________
>>> kvmarm mailing list
>>> kvmarm at lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-27 17:26         ` Eric Auger
@ 2015-01-27 17:44           ` Andre Przywara
  -1 siblings, 0 replies; 26+ messages in thread
From: Andre Przywara @ 2015-01-27 17:44 UTC (permalink / raw)
  To: Eric Auger, Nikolay Nikolaev, Christoffer Dall
  Cc: open list:KERNEL VIRTUAL MA...,
	Marc Zyngier, kvmarm, VirtualOpenSystems Technical Team,
	ARM PORT

Hi,

On 27/01/15 17:26, Eric Auger wrote:
> On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
>> Hi Andre,
>>
>> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>
>>> Hi Nikolay,
>>>
>>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>>
>>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>>> Both read and write calls are redirected to vgic_io_dev_access where
>>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>>
>>>>
>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>> ---
>>>>  arch/arm/kvm/mmio.c    |    3 -
>>>>  include/kvm/arm_vgic.h |    3 -
>>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>> index d852137..8dc2fde 100644
>>>> --- a/arch/arm/kvm/mmio.c
>>>> +++ b/arch/arm/kvm/mmio.c
>>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>                              fault_ipa, 0);
>>>>       }
>>>>
>>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>>> -             return 1;
>>>> -
>>>
>>> Why is this (whole patch) actually needed? Is that just to make it nicer
>>> by pulling everything under one umbrella?
>>
>>
>> It started from this mail form Christofer:
>> https://lkml.org/lkml/2014/3/28/403
> Hi Nikolay, Andre,
> 
> I also understood that the target was to handle all kernel mmio through
> the same API, hence the first patch. This patch shows that at least for
> GICv2 it was doable without upheavals in vgic code and it also serves
> ioeventd which is good. Andre do you think the price to pay to integrate
> missing redistributors and forthcoming components is too high?

Hopefully not, actually I reckon that moving the "upper level" MMIO
dispatching out of vgic.c and letting the specific VGIC models register
what they need themselves (in their -emul.c files) sounds quite promising.
But this particular patch does not serve this purpose:
a) we replace two lines with a bunch of more layered code
b) we copy the MMIOed data to convert between the interfaces
c) we miss GICv3 emulation

So this needs to be addressed in a more general way (which maybe I will
give a try). That being sad I don't see why we would need to do this
right now and hold back ioeventfd by this rather orthogonal issue.

Christoffer, what's your take on this?

Cheers,
Andre.

> Best Regards
> 
> Eric
> 
> 
>>
>>>
>>> For enabling ioeventfd you actually don't need this patch, right?
>> Yes, we don't need it.
>>> (I am asking because this breaks GICv3 emulation, see below)
>>>
>>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>               return 1;
>>>>
>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>> index 7c55dd5..60639b1 100644
>>>> --- a/include/kvm/arm_vgic.h
>>>> +++ b/include/kvm/arm_vgic.h
>>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>>       unsigned long           *irq_pending_on_cpu;
>>>>
>>>>       struct vgic_vm_ops      vm_ops;
>>>> +     struct kvm_io_device    *io_dev;
>>>>  #endif
>>>>  };
>>>>
>>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>                       bool level);
>>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> -                   struct kvm_exit_mmio *mmio);
>>>>
>>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>> index 0cc6ab6..195d2ba 100644
>>>> --- a/virt/kvm/arm/vgic.c
>>>> +++ b/virt/kvm/arm/vgic.c
>>>> @@ -31,6 +31,9 @@
>>>>  #include <asm/kvm_emulate.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/kvm_mmu.h>
>>>> +#include <asm/kvm.h>
>>>> +
>>>> +#include "iodev.h"
>>>>
>>>>  /*
>>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>>> @@ -77,6 +80,7 @@
>>>>
>>>>  #include "vgic.h"
>>>>
>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>>
>>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>>  {
>>>> +     vgic_register_kvm_io_dev(kvm);
>>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>>  }
>>>>
>>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>  }
>>>>
>>>>  /**
>>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>>   * @vcpu:      pointer to the vcpu performing the access
>>>> - * @run:       pointer to the kvm_run structure
>>>> - * @mmio:      pointer to the data describing the access
>>>> + * @this:      pointer to the kvm_io_device structure
>>>> + * @addr:      the MMIO address being accessed
>>>> + * @len:       the length of the accessed data
>>>> + * @val:       pointer to the value being written,
>>>> + *             or where the read operation will store its result
>>>> + * @is_write:  flag to show whether a write access is performed
>>>>   *
>>>> - * returns true if the MMIO access has been performed in kernel space,
>>>> - * and false if it needs to be emulated in user space.
>>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>>> + * and 1 if it needs to be emulated in user space.
>>>>   * Calls the actual handling routine for the selected VGIC model.
>>>>   */
>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> -                   struct kvm_exit_mmio *mmio)
>>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>>  {
>>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>>> -             return false;
>>>> +     struct kvm_exit_mmio mmio;
>>>> +     bool ret;
>>>> +
>>>> +     mmio = (struct kvm_exit_mmio) {
>>>> +             .phys_addr = addr,
>>>> +             .len = len,
>>>> +             .is_write = is_write,
>>>> +     };
>>>> +
>>>> +     if (is_write)
>>>> +             memcpy(mmio.data, val, len);
>>>>
>>>>       /*
>>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>>        * vgic_handle_mmio_range() defined above.
>>>>        */
>>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>>> +
>>>> +     if (!is_write)
>>>> +             memcpy(val, mmio.data, len);
>>>> +
>>>> +     return ret ? 0 : 1;
>>>> +}
>>>> +
>>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                       gpa_t addr, int len, void *val)
>>>> +{
>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>>> +}
>>>> +
>>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                        gpa_t addr, int len, const void *val)
>>>> +{
>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>>> +}
>>>> +
>>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>>> +     .read       = vgic_io_dev_read,
>>>> +     .write      = vgic_io_dev_write,
>>>> +};
>>>> +
>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>>> +{
>>>> +     int len = 0;
>>>> +     int ret;
>>>> +
>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>> +     unsigned long base = dist->vgic_dist_base;
>>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>>> +     struct kvm_io_device *dev;
>>>> +
>>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>>> +             kvm_err("Need to set vgic distributor address first\n");
>>>> +             return -ENXIO;
>>>> +     }
>>>> +
>>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>>> +     if (!dev)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     switch (type) {
>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>>> +             break;
>>>> +#ifdef CONFIG_ARM_GIC_V3
>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>>> +             break;
>>>> +#endif
>>>> +     }
>>>
>>> But this only registers the GIC distributor, leaving out the
>>> redistributor regions introduced by GICv3. To me it looks like this
>> I see GICv3 needs more work.
>>
>>> kvm_iodevice registration code should be moved into *-emul.c, where each
>>> emulated device registers what it needs.
>>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>>> a proper solution for this, so I am wondering if we could just leave
>>> that patch out (at least for now) and keep the two-line special
>>> treatment for the VGIC above in.
>>> That should enable ioeventfd without breaking the VGIC.
>> Then we're back to the original RFC patch series.
>> I have no issues droppin this one (and propably patch 1 in the series)
>> and leaving only the eventfd related handling.
>> I just need some consensus/confirmation on the mailing list.
>>
>> regards,
>> Nikolay Nikolaev
>>
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +
>>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>>> +
>>>> +     mutex_lock(&kvm->slots_lock);
>>>> +
>>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>>> +                     base, len, dev);
>>>> +     if (ret < 0)
>>>> +             goto out_unlock;
>>>> +     mutex_unlock(&kvm->slots_lock);
>>>> +
>>>> +     kvm->arch.vgic.io_dev = dev;
>>>> +
>>>> +     return 0;
>>>> +
>>>> +out_unlock:
>>>> +     mutex_unlock(&kvm->slots_lock);
>>>> +     kfree(dev);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>>> +{
>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>> +
>>>> +     if (dist) {
>>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>>> +             kfree(dist->io_dev);
>>>> +             dist->io_dev = NULL;
>>>> +     }
>>>>  }
>>>>
>>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>>       struct kvm_vcpu *vcpu;
>>>>       int i;
>>>>
>>>> +     vgic_unregister_kvm_io_dev(kvm);
>>>> +
>>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>>
>>>>
>>>> _______________________________________________
>>>> kvmarm mailing list
>>>> kvmarm@lists.cs.columbia.edu
>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>
> 
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-27 17:44           ` Andre Przywara
  0 siblings, 0 replies; 26+ messages in thread
From: Andre Przywara @ 2015-01-27 17:44 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On 27/01/15 17:26, Eric Auger wrote:
> On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
>> Hi Andre,
>>
>> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>
>>> Hi Nikolay,
>>>
>>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>>
>>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>>> Both read and write calls are redirected to vgic_io_dev_access where
>>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>>
>>>>
>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>> ---
>>>>  arch/arm/kvm/mmio.c    |    3 -
>>>>  include/kvm/arm_vgic.h |    3 -
>>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>> index d852137..8dc2fde 100644
>>>> --- a/arch/arm/kvm/mmio.c
>>>> +++ b/arch/arm/kvm/mmio.c
>>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>                              fault_ipa, 0);
>>>>       }
>>>>
>>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>>> -             return 1;
>>>> -
>>>
>>> Why is this (whole patch) actually needed? Is that just to make it nicer
>>> by pulling everything under one umbrella?
>>
>>
>> It started from this mail form Christofer:
>> https://lkml.org/lkml/2014/3/28/403
> Hi Nikolay, Andre,
> 
> I also understood that the target was to handle all kernel mmio through
> the same API, hence the first patch. This patch shows that at least for
> GICv2 it was doable without upheavals in vgic code and it also serves
> ioeventd which is good. Andre do you think the price to pay to integrate
> missing redistributors and forthcoming components is too high?

Hopefully not, actually I reckon that moving the "upper level" MMIO
dispatching out of vgic.c and letting the specific VGIC models register
what they need themselves (in their -emul.c files) sounds quite promising.
But this particular patch does not serve this purpose:
a) we replace two lines with a bunch of more layered code
b) we copy the MMIOed data to convert between the interfaces
c) we miss GICv3 emulation

So this needs to be addressed in a more general way (which maybe I will
give a try). That being sad I don't see why we would need to do this
right now and hold back ioeventfd by this rather orthogonal issue.

Christoffer, what's your take on this?

Cheers,
Andre.

> Best Regards
> 
> Eric
> 
> 
>>
>>>
>>> For enabling ioeventfd you actually don't need this patch, right?
>> Yes, we don't need it.
>>> (I am asking because this breaks GICv3 emulation, see below)
>>>
>>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>               return 1;
>>>>
>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>> index 7c55dd5..60639b1 100644
>>>> --- a/include/kvm/arm_vgic.h
>>>> +++ b/include/kvm/arm_vgic.h
>>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>>       unsigned long           *irq_pending_on_cpu;
>>>>
>>>>       struct vgic_vm_ops      vm_ops;
>>>> +     struct kvm_io_device    *io_dev;
>>>>  #endif
>>>>  };
>>>>
>>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>                       bool level);
>>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> -                   struct kvm_exit_mmio *mmio);
>>>>
>>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>> index 0cc6ab6..195d2ba 100644
>>>> --- a/virt/kvm/arm/vgic.c
>>>> +++ b/virt/kvm/arm/vgic.c
>>>> @@ -31,6 +31,9 @@
>>>>  #include <asm/kvm_emulate.h>
>>>>  #include <asm/kvm_arm.h>
>>>>  #include <asm/kvm_mmu.h>
>>>> +#include <asm/kvm.h>
>>>> +
>>>> +#include "iodev.h"
>>>>
>>>>  /*
>>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>>> @@ -77,6 +80,7 @@
>>>>
>>>>  #include "vgic.h"
>>>>
>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>>
>>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>>  {
>>>> +     vgic_register_kvm_io_dev(kvm);
>>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>>  }
>>>>
>>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>  }
>>>>
>>>>  /**
>>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>>   * @vcpu:      pointer to the vcpu performing the access
>>>> - * @run:       pointer to the kvm_run structure
>>>> - * @mmio:      pointer to the data describing the access
>>>> + * @this:      pointer to the kvm_io_device structure
>>>> + * @addr:      the MMIO address being accessed
>>>> + * @len:       the length of the accessed data
>>>> + * @val:       pointer to the value being written,
>>>> + *             or where the read operation will store its result
>>>> + * @is_write:  flag to show whether a write access is performed
>>>>   *
>>>> - * returns true if the MMIO access has been performed in kernel space,
>>>> - * and false if it needs to be emulated in user space.
>>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>>> + * and 1 if it needs to be emulated in user space.
>>>>   * Calls the actual handling routine for the selected VGIC model.
>>>>   */
>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>> -                   struct kvm_exit_mmio *mmio)
>>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>>  {
>>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>>> -             return false;
>>>> +     struct kvm_exit_mmio mmio;
>>>> +     bool ret;
>>>> +
>>>> +     mmio = (struct kvm_exit_mmio) {
>>>> +             .phys_addr = addr,
>>>> +             .len = len,
>>>> +             .is_write = is_write,
>>>> +     };
>>>> +
>>>> +     if (is_write)
>>>> +             memcpy(mmio.data, val, len);
>>>>
>>>>       /*
>>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>>        * vgic_handle_mmio_range() defined above.
>>>>        */
>>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>>> +
>>>> +     if (!is_write)
>>>> +             memcpy(val, mmio.data, len);
>>>> +
>>>> +     return ret ? 0 : 1;
>>>> +}
>>>> +
>>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                       gpa_t addr, int len, void *val)
>>>> +{
>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>>> +}
>>>> +
>>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>> +                        gpa_t addr, int len, const void *val)
>>>> +{
>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>>> +}
>>>> +
>>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>>> +     .read       = vgic_io_dev_read,
>>>> +     .write      = vgic_io_dev_write,
>>>> +};
>>>> +
>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>>> +{
>>>> +     int len = 0;
>>>> +     int ret;
>>>> +
>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>> +     unsigned long base = dist->vgic_dist_base;
>>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>>> +     struct kvm_io_device *dev;
>>>> +
>>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>>> +             kvm_err("Need to set vgic distributor address first\n");
>>>> +             return -ENXIO;
>>>> +     }
>>>> +
>>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>>> +     if (!dev)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     switch (type) {
>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>>> +             break;
>>>> +#ifdef CONFIG_ARM_GIC_V3
>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>>> +             break;
>>>> +#endif
>>>> +     }
>>>
>>> But this only registers the GIC distributor, leaving out the
>>> redistributor regions introduced by GICv3. To me it looks like this
>> I see GICv3 needs more work.
>>
>>> kvm_iodevice registration code should be moved into *-emul.c, where each
>>> emulated device registers what it needs.
>>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>>> a proper solution for this, so I am wondering if we could just leave
>>> that patch out (at least for now) and keep the two-line special
>>> treatment for the VGIC above in.
>>> That should enable ioeventfd without breaking the VGIC.
>> Then we're back to the original RFC patch series.
>> I have no issues droppin this one (and propably patch 1 in the series)
>> and leaving only the eventfd related handling.
>> I just need some consensus/confirmation on the mailing list.
>>
>> regards,
>> Nikolay Nikolaev
>>
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +
>>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>>> +
>>>> +     mutex_lock(&kvm->slots_lock);
>>>> +
>>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>>> +                     base, len, dev);
>>>> +     if (ret < 0)
>>>> +             goto out_unlock;
>>>> +     mutex_unlock(&kvm->slots_lock);
>>>> +
>>>> +     kvm->arch.vgic.io_dev = dev;
>>>> +
>>>> +     return 0;
>>>> +
>>>> +out_unlock:
>>>> +     mutex_unlock(&kvm->slots_lock);
>>>> +     kfree(dev);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>>> +{
>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>> +
>>>> +     if (dist) {
>>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>>> +             kfree(dist->io_dev);
>>>> +             dist->io_dev = NULL;
>>>> +     }
>>>>  }
>>>>
>>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>>       struct kvm_vcpu *vcpu;
>>>>       int i;
>>>>
>>>> +     vgic_unregister_kvm_io_dev(kvm);
>>>> +
>>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>>
>>>>
>>>> _______________________________________________
>>>> kvmarm mailing list
>>>> kvmarm at lists.cs.columbia.edu
>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>
> 
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-27 17:44           ` Andre Przywara
@ 2015-01-29 15:57             ` Christoffer Dall
  -1 siblings, 0 replies; 26+ messages in thread
From: Christoffer Dall @ 2015-01-29 15:57 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Eric Auger, Nikolay Nikolaev, open list:KERNEL VIRTUAL MA...,
	Marc Zyngier, kvmarm, VirtualOpenSystems Technical Team,
	ARM PORT

On Tue, Jan 27, 2015 at 05:44:26PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 27/01/15 17:26, Eric Auger wrote:
> > On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
> >> Hi Andre,
> >>
> >> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> >>>
> >>> Hi Nikolay,
> >>>
> >>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
> >>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> >>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
> >>>>
> >>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> >>>> Both read and write calls are redirected to vgic_io_dev_access where
> >>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> >>>>
> >>>>
> >>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> >>>> ---
> >>>>  arch/arm/kvm/mmio.c    |    3 -
> >>>>  include/kvm/arm_vgic.h |    3 -
> >>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
> >>>>  3 files changed, 114 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> >>>> index d852137..8dc2fde 100644
> >>>> --- a/arch/arm/kvm/mmio.c
> >>>> +++ b/arch/arm/kvm/mmio.c
> >>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >>>>                              fault_ipa, 0);
> >>>>       }
> >>>>
> >>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
> >>>> -             return 1;
> >>>> -
> >>>
> >>> Why is this (whole patch) actually needed? Is that just to make it nicer
> >>> by pulling everything under one umbrella?
> >>
> >>
> >> It started from this mail form Christofer:
> >> https://lkml.org/lkml/2014/3/28/403
> > Hi Nikolay, Andre,
> > 
> > I also understood that the target was to handle all kernel mmio through
> > the same API, hence the first patch. This patch shows that at least for
> > GICv2 it was doable without upheavals in vgic code and it also serves
> > ioeventd which is good. Andre do you think the price to pay to integrate
> > missing redistributors and forthcoming components is too high?
> 
> Hopefully not, actually I reckon that moving the "upper level" MMIO
> dispatching out of vgic.c and letting the specific VGIC models register
> what they need themselves (in their -emul.c files) sounds quite promising.
> But this particular patch does not serve this purpose:
> a) we replace two lines with a bunch of more layered code
> b) we copy the MMIOed data to convert between the interfaces
> c) we miss GICv3 emulation
> 
> So this needs to be addressed in a more general way (which maybe I will
> give a try). That being sad I don't see why we would need to do this
> right now and hold back ioeventfd by this rather orthogonal issue.
> 
> Christoffer, what's your take on this?
> 
Well, I'd like to not special-case the vgic handling function just
because we want to get this in sooner.

The fact that this is conflicting with gicv3 that just got in and that
we're at -rc6 now, makes me think it's probably too late to do proper
testing and review of this before queuing it, so why not fix it right
instead of saying "we'll fix this later" and never get to it...

-Christoffer

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-29 15:57             ` Christoffer Dall
  0 siblings, 0 replies; 26+ messages in thread
From: Christoffer Dall @ 2015-01-29 15:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 27, 2015 at 05:44:26PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 27/01/15 17:26, Eric Auger wrote:
> > On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
> >> Hi Andre,
> >>
> >> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> >>>
> >>> Hi Nikolay,
> >>>
> >>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
> >>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
> >>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
> >>>>
> >>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
> >>>> Both read and write calls are redirected to vgic_io_dev_access where
> >>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
> >>>>
> >>>>
> >>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> >>>> ---
> >>>>  arch/arm/kvm/mmio.c    |    3 -
> >>>>  include/kvm/arm_vgic.h |    3 -
> >>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
> >>>>  3 files changed, 114 insertions(+), 15 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
> >>>> index d852137..8dc2fde 100644
> >>>> --- a/arch/arm/kvm/mmio.c
> >>>> +++ b/arch/arm/kvm/mmio.c
> >>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >>>>                              fault_ipa, 0);
> >>>>       }
> >>>>
> >>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
> >>>> -             return 1;
> >>>> -
> >>>
> >>> Why is this (whole patch) actually needed? Is that just to make it nicer
> >>> by pulling everything under one umbrella?
> >>
> >>
> >> It started from this mail form Christofer:
> >> https://lkml.org/lkml/2014/3/28/403
> > Hi Nikolay, Andre,
> > 
> > I also understood that the target was to handle all kernel mmio through
> > the same API, hence the first patch. This patch shows that at least for
> > GICv2 it was doable without upheavals in vgic code and it also serves
> > ioeventd which is good. Andre do you think the price to pay to integrate
> > missing redistributors and forthcoming components is too high?
> 
> Hopefully not, actually I reckon that moving the "upper level" MMIO
> dispatching out of vgic.c and letting the specific VGIC models register
> what they need themselves (in their -emul.c files) sounds quite promising.
> But this particular patch does not serve this purpose:
> a) we replace two lines with a bunch of more layered code
> b) we copy the MMIOed data to convert between the interfaces
> c) we miss GICv3 emulation
> 
> So this needs to be addressed in a more general way (which maybe I will
> give a try). That being sad I don't see why we would need to do this
> right now and hold back ioeventfd by this rather orthogonal issue.
> 
> Christoffer, what's your take on this?
> 
Well, I'd like to not special-case the vgic handling function just
because we want to get this in sooner.

The fact that this is conflicting with gicv3 that just got in and that
we're at -rc6 now, makes me think it's probably too late to do proper
testing and review of this before queuing it, so why not fix it right
instead of saying "we'll fix this later" and never get to it...

-Christoffer

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
  2015-01-27 17:44           ` Andre Przywara
@ 2015-01-30  7:48             ` Nikolay Nikolaev
  -1 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-30  7:48 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Eric Auger, Christoffer Dall, open list:KERNEL VIRTUAL MA...,
	Marc Zyngier, kvmarm, VirtualOpenSystems Technical Team,
	ARM PORT

On Tue, Jan 27, 2015 at 7:44 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 27/01/15 17:26, Eric Auger wrote:
>> On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
>>> Hi Andre,
>>>
>>> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>
>>>> Hi Nikolay,
>>>>
>>>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>>>
>>>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>>>> Both read and write calls are redirected to vgic_io_dev_access where
>>>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>>>
>>>>>
>>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>>> ---
>>>>>  arch/arm/kvm/mmio.c    |    3 -
>>>>>  include/kvm/arm_vgic.h |    3 -
>>>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>>> index d852137..8dc2fde 100644
>>>>> --- a/arch/arm/kvm/mmio.c
>>>>> +++ b/arch/arm/kvm/mmio.c
>>>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>                              fault_ipa, 0);
>>>>>       }
>>>>>
>>>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>>>> -             return 1;
>>>>> -
>>>>
>>>> Why is this (whole patch) actually needed? Is that just to make it nicer
>>>> by pulling everything under one umbrella?
>>>
>>>
>>> It started from this mail form Christofer:
>>> https://lkml.org/lkml/2014/3/28/403
>> Hi Nikolay, Andre,
>>
>> I also understood that the target was to handle all kernel mmio through
>> the same API, hence the first patch. This patch shows that at least for
>> GICv2 it was doable without upheavals in vgic code and it also serves
>> ioeventd which is good. Andre do you think the price to pay to integrate
>> missing redistributors and forthcoming components is too high?
>
> Hopefully not, actually I reckon that moving the "upper level" MMIO
> dispatching out of vgic.c and letting the specific VGIC models register
> what they need themselves (in their -emul.c files) sounds quite promising.
> But this particular patch does not serve this purpose:
> a) we replace two lines with a bunch of more layered code
> b) we copy the MMIOed data to convert between the interfaces
> c) we miss GICv3 emulation
>
> So this needs to be addressed in a more general way (which maybe I will
> give a try). That being sad I don't see why we would need to do this

Andre,

we've already overspent our resource budget on this so I would be more
than glad if you can take over the VGIC part here.
Of course I'll help with whatever I can (review, testing, patches
logistics etc)

Here is the v3 of the patches in case you need to clone and base your
work on top of it:
https://git.virtualopensystems.com/common/linux/commits/ioeventfd_v3

regards,
Nikolay Nikolaev

> right now and hold back ioeventfd by this rather orthogonal issue.
>
> Christoffer, what's your take on this?
>
> Cheers,
> Andre.
>
>> Best Regards
>>
>> Eric
>>
>>
>>>
>>>>
>>>> For enabling ioeventfd you actually don't need this patch, right?
>>> Yes, we don't need it.
>>>> (I am asking because this breaks GICv3 emulation, see below)
>>>>
>>>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>>               return 1;
>>>>>
>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>>> index 7c55dd5..60639b1 100644
>>>>> --- a/include/kvm/arm_vgic.h
>>>>> +++ b/include/kvm/arm_vgic.h
>>>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>>>       unsigned long           *irq_pending_on_cpu;
>>>>>
>>>>>       struct vgic_vm_ops      vm_ops;
>>>>> +     struct kvm_io_device    *io_dev;
>>>>>  #endif
>>>>>  };
>>>>>
>>>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>>                       bool level);
>>>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>> -                   struct kvm_exit_mmio *mmio);
>>>>>
>>>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>> index 0cc6ab6..195d2ba 100644
>>>>> --- a/virt/kvm/arm/vgic.c
>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>> @@ -31,6 +31,9 @@
>>>>>  #include <asm/kvm_emulate.h>
>>>>>  #include <asm/kvm_arm.h>
>>>>>  #include <asm/kvm_mmu.h>
>>>>> +#include <asm/kvm.h>
>>>>> +
>>>>> +#include "iodev.h"
>>>>>
>>>>>  /*
>>>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>>>> @@ -77,6 +80,7 @@
>>>>>
>>>>>  #include "vgic.h"
>>>>>
>>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>>>
>>>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>>>  {
>>>>> +     vgic_register_kvm_io_dev(kvm);
>>>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>>>  }
>>>>>
>>>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>  }
>>>>>
>>>>>  /**
>>>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>>>   * @vcpu:      pointer to the vcpu performing the access
>>>>> - * @run:       pointer to the kvm_run structure
>>>>> - * @mmio:      pointer to the data describing the access
>>>>> + * @this:      pointer to the kvm_io_device structure
>>>>> + * @addr:      the MMIO address being accessed
>>>>> + * @len:       the length of the accessed data
>>>>> + * @val:       pointer to the value being written,
>>>>> + *             or where the read operation will store its result
>>>>> + * @is_write:  flag to show whether a write access is performed
>>>>>   *
>>>>> - * returns true if the MMIO access has been performed in kernel space,
>>>>> - * and false if it needs to be emulated in user space.
>>>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>>>> + * and 1 if it needs to be emulated in user space.
>>>>>   * Calls the actual handling routine for the selected VGIC model.
>>>>>   */
>>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>> -                   struct kvm_exit_mmio *mmio)
>>>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>>>  {
>>>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>>>> -             return false;
>>>>> +     struct kvm_exit_mmio mmio;
>>>>> +     bool ret;
>>>>> +
>>>>> +     mmio = (struct kvm_exit_mmio) {
>>>>> +             .phys_addr = addr,
>>>>> +             .len = len,
>>>>> +             .is_write = is_write,
>>>>> +     };
>>>>> +
>>>>> +     if (is_write)
>>>>> +             memcpy(mmio.data, val, len);
>>>>>
>>>>>       /*
>>>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>>>        * vgic_handle_mmio_range() defined above.
>>>>>        */
>>>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>>>> +
>>>>> +     if (!is_write)
>>>>> +             memcpy(val, mmio.data, len);
>>>>> +
>>>>> +     return ret ? 0 : 1;
>>>>> +}
>>>>> +
>>>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                       gpa_t addr, int len, void *val)
>>>>> +{
>>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>>>> +}
>>>>> +
>>>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                        gpa_t addr, int len, const void *val)
>>>>> +{
>>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>>>> +}
>>>>> +
>>>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>>>> +     .read       = vgic_io_dev_read,
>>>>> +     .write      = vgic_io_dev_write,
>>>>> +};
>>>>> +
>>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>>>> +{
>>>>> +     int len = 0;
>>>>> +     int ret;
>>>>> +
>>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>>> +     unsigned long base = dist->vgic_dist_base;
>>>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>>>> +     struct kvm_io_device *dev;
>>>>> +
>>>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>>>> +             kvm_err("Need to set vgic distributor address first\n");
>>>>> +             return -ENXIO;
>>>>> +     }
>>>>> +
>>>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>>>> +     if (!dev)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     switch (type) {
>>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>>>> +             break;
>>>>> +#ifdef CONFIG_ARM_GIC_V3
>>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>>>> +             break;
>>>>> +#endif
>>>>> +     }
>>>>
>>>> But this only registers the GIC distributor, leaving out the
>>>> redistributor regions introduced by GICv3. To me it looks like this
>>> I see GICv3 needs more work.
>>>
>>>> kvm_iodevice registration code should be moved into *-emul.c, where each
>>>> emulated device registers what it needs.
>>>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>>>> a proper solution for this, so I am wondering if we could just leave
>>>> that patch out (at least for now) and keep the two-line special
>>>> treatment for the VGIC above in.
>>>> That should enable ioeventfd without breaking the VGIC.
>>> Then we're back to the original RFC patch series.
>>> I have no issues droppin this one (and propably patch 1 in the series)
>>> and leaving only the eventfd related handling.
>>> I just need some consensus/confirmation on the mailing list.
>>>
>>> regards,
>>> Nikolay Nikolaev
>>>
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>> +
>>>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>>>> +
>>>>> +     mutex_lock(&kvm->slots_lock);
>>>>> +
>>>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>>>> +                     base, len, dev);
>>>>> +     if (ret < 0)
>>>>> +             goto out_unlock;
>>>>> +     mutex_unlock(&kvm->slots_lock);
>>>>> +
>>>>> +     kvm->arch.vgic.io_dev = dev;
>>>>> +
>>>>> +     return 0;
>>>>> +
>>>>> +out_unlock:
>>>>> +     mutex_unlock(&kvm->slots_lock);
>>>>> +     kfree(dev);
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>>>> +{
>>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>>> +
>>>>> +     if (dist) {
>>>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>>>> +             kfree(dist->io_dev);
>>>>> +             dist->io_dev = NULL;
>>>>> +     }
>>>>>  }
>>>>>
>>>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>>>       struct kvm_vcpu *vcpu;
>>>>>       int i;
>>>>>
>>>>> +     vgic_unregister_kvm_io_dev(kvm);
>>>>> +
>>>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> kvmarm mailing list
>>>>> kvmarm@lists.cs.columbia.edu
>>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>>
>>
>>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend
@ 2015-01-30  7:48             ` Nikolay Nikolaev
  0 siblings, 0 replies; 26+ messages in thread
From: Nikolay Nikolaev @ 2015-01-30  7:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 27, 2015 at 7:44 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 27/01/15 17:26, Eric Auger wrote:
>> On 01/27/2015 05:51 PM, Nikolay Nikolaev wrote:
>>> Hi Andre,
>>>
>>> On Tue, Jan 27, 2015 at 3:31 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>
>>>> Hi Nikolay,
>>>>
>>>> On 24/01/15 11:59, Nikolay Nikolaev wrote:
>>>>> In io_mem_abort remove the call to vgic_handle_mmio. The target is to have
>>>>> a single MMIO handling path - that is through the kvm_io_bus_ API.
>>>>>
>>>>> Register a kvm_io_device in kvm_vgic_init on the whole vGIC MMIO region.
>>>>> Both read and write calls are redirected to vgic_io_dev_access where
>>>>> kvm_exit_mmio is composed to pass it to vm_ops.handle_mmio.
>>>>>
>>>>>
>>>>> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
>>>>> ---
>>>>>  arch/arm/kvm/mmio.c    |    3 -
>>>>>  include/kvm/arm_vgic.h |    3 -
>>>>>  virt/kvm/arm/vgic.c    |  123 ++++++++++++++++++++++++++++++++++++++++++++----
>>>>>  3 files changed, 114 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/kvm/mmio.c b/arch/arm/kvm/mmio.c
>>>>> index d852137..8dc2fde 100644
>>>>> --- a/arch/arm/kvm/mmio.c
>>>>> +++ b/arch/arm/kvm/mmio.c
>>>>> @@ -230,9 +230,6 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>                              fault_ipa, 0);
>>>>>       }
>>>>>
>>>>> -     if (vgic_handle_mmio(vcpu, run, &mmio))
>>>>> -             return 1;
>>>>> -
>>>>
>>>> Why is this (whole patch) actually needed? Is that just to make it nicer
>>>> by pulling everything under one umbrella?
>>>
>>>
>>> It started from this mail form Christofer:
>>> https://lkml.org/lkml/2014/3/28/403
>> Hi Nikolay, Andre,
>>
>> I also understood that the target was to handle all kernel mmio through
>> the same API, hence the first patch. This patch shows that at least for
>> GICv2 it was doable without upheavals in vgic code and it also serves
>> ioeventd which is good. Andre do you think the price to pay to integrate
>> missing redistributors and forthcoming components is too high?
>
> Hopefully not, actually I reckon that moving the "upper level" MMIO
> dispatching out of vgic.c and letting the specific VGIC models register
> what they need themselves (in their -emul.c files) sounds quite promising.
> But this particular patch does not serve this purpose:
> a) we replace two lines with a bunch of more layered code
> b) we copy the MMIOed data to convert between the interfaces
> c) we miss GICv3 emulation
>
> So this needs to be addressed in a more general way (which maybe I will
> give a try). That being sad I don't see why we would need to do this

Andre,

we've already overspent our resource budget on this so I would be more
than glad if you can take over the VGIC part here.
Of course I'll help with whatever I can (review, testing, patches
logistics etc)

Here is the v3 of the patches in case you need to clone and base your
work on top of it:
https://git.virtualopensystems.com/common/linux/commits/ioeventfd_v3

regards,
Nikolay Nikolaev

> right now and hold back ioeventfd by this rather orthogonal issue.
>
> Christoffer, what's your take on this?
>
> Cheers,
> Andre.
>
>> Best Regards
>>
>> Eric
>>
>>
>>>
>>>>
>>>> For enabling ioeventfd you actually don't need this patch, right?
>>> Yes, we don't need it.
>>>> (I am asking because this breaks GICv3 emulation, see below)
>>>>
>>>>>       if (handle_kernel_mmio(vcpu, run, &mmio))
>>>>>               return 1;
>>>>>
>>>>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>>>>> index 7c55dd5..60639b1 100644
>>>>> --- a/include/kvm/arm_vgic.h
>>>>> +++ b/include/kvm/arm_vgic.h
>>>>> @@ -237,6 +237,7 @@ struct vgic_dist {
>>>>>       unsigned long           *irq_pending_on_cpu;
>>>>>
>>>>>       struct vgic_vm_ops      vm_ops;
>>>>> +     struct kvm_io_device    *io_dev;
>>>>>  #endif
>>>>>  };
>>>>>
>>>>> @@ -311,8 +312,6 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>>                       bool level);
>>>>>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>>>>>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>> -                   struct kvm_exit_mmio *mmio);
>>>>>
>>>>>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>>>>>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>> index 0cc6ab6..195d2ba 100644
>>>>> --- a/virt/kvm/arm/vgic.c
>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>> @@ -31,6 +31,9 @@
>>>>>  #include <asm/kvm_emulate.h>
>>>>>  #include <asm/kvm_arm.h>
>>>>>  #include <asm/kvm_mmu.h>
>>>>> +#include <asm/kvm.h>
>>>>> +
>>>>> +#include "iodev.h"
>>>>>
>>>>>  /*
>>>>>   * How the whole thing works (courtesy of Christoffer Dall):
>>>>> @@ -77,6 +80,7 @@
>>>>>
>>>>>  #include "vgic.h"
>>>>>
>>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm);
>>>>>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>>>>>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>>>>>  static struct vgic_lr vgic_get_lr(const struct kvm_vcpu *vcpu, int lr);
>>>>> @@ -97,6 +101,7 @@ static bool queue_sgi(struct kvm_vcpu *vcpu, int irq)
>>>>>
>>>>>  int kvm_vgic_map_resources(struct kvm *kvm)
>>>>>  {
>>>>> +     vgic_register_kvm_io_dev(kvm);
>>>>>       return kvm->arch.vgic.vm_ops.map_resources(kvm, vgic);
>>>>>  }
>>>>>
>>>>> @@ -776,27 +781,123 @@ bool vgic_handle_mmio_range(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>>  }
>>>>>
>>>>>  /**
>>>>> - * vgic_handle_mmio - handle an in-kernel MMIO access for the GIC emulation
>>>>> + * vgic_io_dev_access - handle an in-kernel MMIO access for the GIC emulation
>>>>>   * @vcpu:      pointer to the vcpu performing the access
>>>>> - * @run:       pointer to the kvm_run structure
>>>>> - * @mmio:      pointer to the data describing the access
>>>>> + * @this:      pointer to the kvm_io_device structure
>>>>> + * @addr:      the MMIO address being accessed
>>>>> + * @len:       the length of the accessed data
>>>>> + * @val:       pointer to the value being written,
>>>>> + *             or where the read operation will store its result
>>>>> + * @is_write:  flag to show whether a write access is performed
>>>>>   *
>>>>> - * returns true if the MMIO access has been performed in kernel space,
>>>>> - * and false if it needs to be emulated in user space.
>>>>> + * returns 0 if the MMIO access has been performed in kernel space,
>>>>> + * and 1 if it needs to be emulated in user space.
>>>>>   * Calls the actual handling routine for the selected VGIC model.
>>>>>   */
>>>>> -bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>>>>> -                   struct kvm_exit_mmio *mmio)
>>>>> +static int vgic_io_dev_access(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                         gpa_t addr, int len, void *val, bool is_write)
>>>>>  {
>>>>> -     if (!irqchip_in_kernel(vcpu->kvm))
>>>>> -             return false;
>>>>> +     struct kvm_exit_mmio mmio;
>>>>> +     bool ret;
>>>>> +
>>>>> +     mmio = (struct kvm_exit_mmio) {
>>>>> +             .phys_addr = addr,
>>>>> +             .len = len,
>>>>> +             .is_write = is_write,
>>>>> +     };
>>>>> +
>>>>> +     if (is_write)
>>>>> +             memcpy(mmio.data, val, len);
>>>>>
>>>>>       /*
>>>>>        * This will currently call either vgic_v2_handle_mmio() or
>>>>>        * vgic_v3_handle_mmio(), which in turn will call
>>>>>        * vgic_handle_mmio_range() defined above.
>>>>>        */
>>>>> -     return vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, run, mmio);
>>>>> +     ret = vcpu->kvm->arch.vgic.vm_ops.handle_mmio(vcpu, vcpu->run, &mmio);
>>>>> +
>>>>> +     if (!is_write)
>>>>> +             memcpy(val, mmio.data, len);
>>>>> +
>>>>> +     return ret ? 0 : 1;
>>>>> +}
>>>>> +
>>>>> +static int vgic_io_dev_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                       gpa_t addr, int len, void *val)
>>>>> +{
>>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, val, false);
>>>>> +}
>>>>> +
>>>>> +static int vgic_io_dev_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
>>>>> +                        gpa_t addr, int len, const void *val)
>>>>> +{
>>>>> +     return vgic_io_dev_access(vcpu, this, addr, len, (void *)val, true);
>>>>> +}
>>>>> +
>>>>> +static const struct kvm_io_device_ops vgic_io_dev_ops = {
>>>>> +     .read       = vgic_io_dev_read,
>>>>> +     .write      = vgic_io_dev_write,
>>>>> +};
>>>>> +
>>>>> +static int vgic_register_kvm_io_dev(struct kvm *kvm)
>>>>> +{
>>>>> +     int len = 0;
>>>>> +     int ret;
>>>>> +
>>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>>> +     unsigned long base = dist->vgic_dist_base;
>>>>> +     u32 type = kvm->arch.vgic.vgic_model;
>>>>> +     struct kvm_io_device *dev;
>>>>> +
>>>>> +     if (IS_VGIC_ADDR_UNDEF(base)) {
>>>>> +             kvm_err("Need to set vgic distributor address first\n");
>>>>> +             return -ENXIO;
>>>>> +     }
>>>>> +
>>>>> +     dev = kzalloc(sizeof(struct kvm_io_device), GFP_KERNEL);
>>>>> +     if (!dev)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     switch (type) {
>>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V2:
>>>>> +             len = KVM_VGIC_V2_DIST_SIZE;
>>>>> +             break;
>>>>> +#ifdef CONFIG_ARM_GIC_V3
>>>>> +     case KVM_DEV_TYPE_ARM_VGIC_V3:
>>>>> +             len = KVM_VGIC_V3_DIST_SIZE;
>>>>> +             break;
>>>>> +#endif
>>>>> +     }
>>>>
>>>> But this only registers the GIC distributor, leaving out the
>>>> redistributor regions introduced by GICv3. To me it looks like this
>>> I see GICv3 needs more work.
>>>
>>>> kvm_iodevice registration code should be moved into *-emul.c, where each
>>>> emulated device registers what it needs.
>>>> Especially in the wake of the upcoming v2M/ITS emulation I think we need
>>>> a proper solution for this, so I am wondering if we could just leave
>>>> that patch out (at least for now) and keep the two-line special
>>>> treatment for the VGIC above in.
>>>> That should enable ioeventfd without breaking the VGIC.
>>> Then we're back to the original RFC patch series.
>>> I have no issues droppin this one (and propably patch 1 in the series)
>>> and leaving only the eventfd related handling.
>>> I just need some consensus/confirmation on the mailing list.
>>>
>>> regards,
>>> Nikolay Nikolaev
>>>
>>>>
>>>> Cheers,
>>>> Andre.
>>>>
>>>>> +
>>>>> +     kvm_iodevice_init(dev, &vgic_io_dev_ops);
>>>>> +
>>>>> +     mutex_lock(&kvm->slots_lock);
>>>>> +
>>>>> +     ret = kvm_io_bus_register_dev(kvm, KVM_MMIO_BUS,
>>>>> +                     base, len, dev);
>>>>> +     if (ret < 0)
>>>>> +             goto out_unlock;
>>>>> +     mutex_unlock(&kvm->slots_lock);
>>>>> +
>>>>> +     kvm->arch.vgic.io_dev = dev;
>>>>> +
>>>>> +     return 0;
>>>>> +
>>>>> +out_unlock:
>>>>> +     mutex_unlock(&kvm->slots_lock);
>>>>> +     kfree(dev);
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static void vgic_unregister_kvm_io_dev(struct kvm *kvm)
>>>>> +{
>>>>> +     struct vgic_dist *dist = &kvm->arch.vgic;
>>>>> +
>>>>> +     if (dist) {
>>>>> +             kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS, dist->io_dev);
>>>>> +             kfree(dist->io_dev);
>>>>> +             dist->io_dev = NULL;
>>>>> +     }
>>>>>  }
>>>>>
>>>>>  static int vgic_nr_shared_irqs(struct vgic_dist *dist)
>>>>> @@ -1428,6 +1529,8 @@ void kvm_vgic_destroy(struct kvm *kvm)
>>>>>       struct kvm_vcpu *vcpu;
>>>>>       int i;
>>>>>
>>>>> +     vgic_unregister_kvm_io_dev(kvm);
>>>>> +
>>>>>       kvm_for_each_vcpu(i, vcpu, kvm)
>>>>>               kvm_vgic_vcpu_destroy(vcpu);
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> kvmarm mailing list
>>>>> kvmarm at lists.cs.columbia.edu
>>>>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>>>>
>>
>>

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2015-01-30  7:49 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-24 11:59 [PATCH v3 0/5] ARM: KVM: Enable the ioeventfd capability of KVM on ARM Nikolay Nikolaev
2015-01-24 11:59 ` Nikolay Nikolaev
2015-01-24 11:59 ` [PATCH v3 1/5] KVM: Redesign kvm_io_bus_ API to pass VCPU structure to the callbacks Nikolay Nikolaev
2015-01-24 11:59   ` Nikolay Nikolaev
2015-01-24 19:08   ` Paolo Bonzini
2015-01-24 19:08     ` Paolo Bonzini
2015-01-24 11:59 ` [PATCH v3 2/5] KVM: ARM: on IO mem abort - route the call to KVM MMIO bus Nikolay Nikolaev
2015-01-24 11:59   ` Nikolay Nikolaev
2015-01-24 11:59 ` [PATCH v3 3/5] KVM: ARM VGIC add kvm_io_bus_ frontend Nikolay Nikolaev
2015-01-24 11:59   ` Nikolay Nikolaev
2015-01-27 13:31   ` Andre Przywara
2015-01-27 13:31     ` Andre Przywara
2015-01-27 16:51     ` Nikolay Nikolaev
2015-01-27 16:51       ` Nikolay Nikolaev
2015-01-27 17:26       ` Eric Auger
2015-01-27 17:26         ` Eric Auger
2015-01-27 17:44         ` Andre Przywara
2015-01-27 17:44           ` Andre Przywara
2015-01-29 15:57           ` Christoffer Dall
2015-01-29 15:57             ` Christoffer Dall
2015-01-30  7:48           ` Nikolay Nikolaev
2015-01-30  7:48             ` Nikolay Nikolaev
2015-01-24 11:59 ` [PATCH v3 4/5] ARM/ARM64: enable linking against eventfd Nikolay Nikolaev
2015-01-24 11:59   ` Nikolay Nikolaev
2015-01-24 12:00 ` [PATCH v3 5/5] ARM: enable KVM_CAP_IOEVENTFD Nikolay Nikolaev
2015-01-24 12:00   ` Nikolay Nikolaev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.