[PATCH v3 00/10] kvmtool: arm64: GICv3 guest support

[PATCH v3 00/10] kvmtool: arm64: GICv3 guest support

[Andre Przywara]

Hi,

a new version of the GICv3 support series for kvmtool.

I got rid of passing the number of redistributors around kvmtool.
The new patch 06/10 simplifies ARM's MMIO dispatching, so that we no
longer need to know the GIC size at this point. The FDT code uses
base and size values now directly and these values are private to
arm/gic.c.

The new 07/10 patch aims to solve the number-of-VCPUs problem Marc
mentioned. Instead of letting kvmtool have knowledge about particular
limits, let the kernel decide on this matter. Since KVM_CAP_MAX_VCPUS
is not really reliable on ARM, let's be a bit more relaxed about
KVM_CREATE_VCPU failing and stop with creating more VCPUs if we get
an EINVAL in return.

I also addressed the other comments Marcs gave, but I had to leave
some of the default switch-cases in due to the compiler complaining
otherwise.

Cheers,
Andre.
-----

Since Linux 3.19 the kernel can emulate a GICv3 for KVM guests.
This allows more than 8 VCPUs in a guest and enables in-kernel irqchip
for non-backwards-compatible GICv3 implementations.

This series updates kvmtool to support this feature.
The first half of the series is mostly from Marc and supports some
newer features of the virtual GIC which we later depend on. The second
part enables support for a guest GICv3 by adding a new command line
parameter (--irqchip=).

We now use the KVM_CREATE_DEVICE interface to create a virtual GIC
and only fall back to the now legacy KVM_CREATE_IRQCHIP call if the
former is not supported by the kernel.
Also we use two new features the KVM_CREATE_DEVICE interface
introduces:
* We now set the number of actually used interrupts to avoid
  allocating too many of them without ever using them.
* We tell the kernel explicitly that we are finished with the GIC
  initialisation. This is a requirement for future VGIC versions.

The final three patches introduce virtual GICv3 support, so on
supported hardware (and given kernel support) the user can ask KVM to
emulate a GICv3, lifting the 8 VCPU limit of KVM. This is done by
specifying "--irqchip=gicv3" on the command line.
For the time being the kernel only supports a virtual GICv3 on ARM64,
but as the GIC is shared in kvmtool, I had to add the macro
definitions to not break the build on ARM.

This series goes on top of the new official stand-alone repo hosted
on Will's kernel.org git [1].
Find a branch with those patches included at my repo [2].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
[2] git://linux-arm.org/kvmtool.git (branch gicv3/v3)
    http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/gicv3/v3

Andre Przywara (6):
  arm: finish VGIC initialisation explicitly
  arm: simplify MMIO dispatching
  limit number of VCPUs on demand
  arm: prepare for instantiating different IRQ chip devices
  arm: add support for supplying GICv3 redistributor addresses
  arm: use new irqchip parameter to create different vGIC types

Marc Zyngier (4):
  AArch64: Reserve two 64k pages for GIC CPU interface
  AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
  irq: add irq__get_nr_allocated_lines
  AArch{32,64}: dynamically configure the number of GIC interrupts

 arm/aarch32/arm-cpu.c                    |   2 +-
 arm/aarch64/arm-cpu.c                    |   2 +-
 arm/aarch64/include/kvm/kvm-arch.h       |   2 +-
 arm/gic.c                                | 190 +++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h             |   9 +-
 arm/include/arm-common/kvm-arch.h        |  19 ++--
 arm/include/arm-common/kvm-config-arch.h |   9 +-
 arm/include/arm-common/kvm-cpu-arch.h    |  14 ++-
 arm/kvm-cpu.c                            |  27 ++---
 arm/kvm.c                                |   6 +-
 include/kvm/irq.h                        |   1 +
 irq.c                                    |   5 +
 kvm-cpu.c                                |   7 ++
 13 files changed, 242 insertions(+), 51 deletions(-)

-- 
2.3.5

[PATCH v3 00/10] kvmtool: arm64: GICv3 guest support

[Andre Przywara]

Hi,

a new version of the GICv3 support series for kvmtool.

I got rid of passing the number of redistributors around kvmtool.
The new patch 06/10 simplifies ARM's MMIO dispatching, so that we no
longer need to know the GIC size at this point. The FDT code uses
base and size values now directly and these values are private to
arm/gic.c.

The new 07/10 patch aims to solve the number-of-VCPUs problem Marc
mentioned. Instead of letting kvmtool have knowledge about particular
limits, let the kernel decide on this matter. Since KVM_CAP_MAX_VCPUS
is not really reliable on ARM, let's be a bit more relaxed about
KVM_CREATE_VCPU failing and stop with creating more VCPUs if we get
an EINVAL in return.

I also addressed the other comments Marcs gave, but I had to leave
some of the default switch-cases in due to the compiler complaining
otherwise.

Cheers,
Andre.
-----

Since Linux 3.19 the kernel can emulate a GICv3 for KVM guests.
This allows more than 8 VCPUs in a guest and enables in-kernel irqchip
for non-backwards-compatible GICv3 implementations.

This series updates kvmtool to support this feature.
The first half of the series is mostly from Marc and supports some
newer features of the virtual GIC which we later depend on. The second
part enables support for a guest GICv3 by adding a new command line
parameter (--irqchip=).

We now use the KVM_CREATE_DEVICE interface to create a virtual GIC
and only fall back to the now legacy KVM_CREATE_IRQCHIP call if the
former is not supported by the kernel.
Also we use two new features the KVM_CREATE_DEVICE interface
introduces:
* We now set the number of actually used interrupts to avoid
  allocating too many of them without ever using them.
* We tell the kernel explicitly that we are finished with the GIC
  initialisation. This is a requirement for future VGIC versions.

The final three patches introduce virtual GICv3 support, so on
supported hardware (and given kernel support) the user can ask KVM to
emulate a GICv3, lifting the 8 VCPU limit of KVM. This is done by
specifying "--irqchip=gicv3" on the command line.
For the time being the kernel only supports a virtual GICv3 on ARM64,
but as the GIC is shared in kvmtool, I had to add the macro
definitions to not break the build on ARM.

This series goes on top of the new official stand-alone repo hosted
on Will's kernel.org git [1].
Find a branch with those patches included at my repo [2].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git
[2] git://linux-arm.org/kvmtool.git (branch gicv3/v3)
    http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/gicv3/v3

Andre Przywara (6):
  arm: finish VGIC initialisation explicitly
  arm: simplify MMIO dispatching
  limit number of VCPUs on demand
  arm: prepare for instantiating different IRQ chip devices
  arm: add support for supplying GICv3 redistributor addresses
  arm: use new irqchip parameter to create different vGIC types

Marc Zyngier (4):
  AArch64: Reserve two 64k pages for GIC CPU interface
  AArch{32,64}: use KVM_CREATE_DEVICE & co to instanciate the GIC
  irq: add irq__get_nr_allocated_lines
  AArch{32,64}: dynamically configure the number of GIC interrupts

 arm/aarch32/arm-cpu.c                    |   2 +-
 arm/aarch64/arm-cpu.c                    |   2 +-
 arm/aarch64/include/kvm/kvm-arch.h       |   2 +-
 arm/gic.c                                | 190 +++++++++++++++++++++++++++++--
 arm/include/arm-common/gic.h             |   9 +-
 arm/include/arm-common/kvm-arch.h        |  19 ++--
 arm/include/arm-common/kvm-config-arch.h |   9 +-
 arm/include/arm-common/kvm-cpu-arch.h    |  14 ++-
 arm/kvm-cpu.c                            |  27 ++---
 arm/kvm.c                                |   6 +-
 include/kvm/irq.h                        |   1 +
 irq.c                                    |   5 +
 kvm-cpu.c                                |   7 ++
 13 files changed, 242 insertions(+), 51 deletions(-)

-- 
2.3.5

[PATCH v3 01/10] AArch64: Reserve two 64k pages for GIC CPU interface

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

On AArch64 system with a GICv2, the GICC range can be aligned
to the last 4k block of a 64k page, ending up straddling two
64k pages. In order not to conflict with the distributor mapping,
allocate two 64k pages to the CPU interface.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/include/kvm/kvm-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 2f08a26..4925736 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -2,7 +2,7 @@
 #define KVM__KVM_ARCH_H
 
 #define ARM_GIC_DIST_SIZE	0x10000
-#define ARM_GIC_CPUI_SIZE	0x10000
+#define ARM_GIC_CPUI_SIZE	0x20000
 
 #define ARM_KERN_OFFSET(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
 				0x8000				:	\
-- 
2.3.5

[PATCH v3 01/10] AArch64: Reserve two 64k pages for GIC CPU interface

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

On AArch64 system with a GICv2, the GICC range can be aligned
to the last 4k block of a 64k page, ending up straddling two
64k pages. In order not to conflict with the distributor mapping,
allocate two 64k pages to the CPU interface.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/include/kvm/kvm-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 2f08a26..4925736 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -2,7 +2,7 @@
 #define KVM__KVM_ARCH_H
 
 #define ARM_GIC_DIST_SIZE	0x10000
-#define ARM_GIC_CPUI_SIZE	0x10000
+#define ARM_GIC_CPUI_SIZE	0x20000
 
 #define ARM_KERN_OFFSET(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
 				0x8000				:	\
-- 
2.3.5

[PATCH v3 02/10] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

As of 3.14, KVM/arm supports the creation/configuration of the GIC through
a more generic device API, which is now the preferred way to do so.

Plumb the new API in, and allow the old code to be used as a fallback.

[Andre: Rename some functions on the way to differentiate between
creation and initialisation more clearly and fix error path.]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                    | 69 +++++++++++++++++++++++++++++++++++++++-----
 arm/include/arm-common/gic.h |  2 +-
 arm/kvm.c                    |  6 ++--
 3 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 5d8cbe6..1ff3663 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -7,7 +7,50 @@
 #include <linux/byteorder.h>
 #include <linux/kvm.h>
 
-int gic__init_irqchip(struct kvm *kvm)
+static int gic_fd = -1;
+
+static int gic__create_device(struct kvm *kvm)
+{
+	int err;
+	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
+	u64 dist_addr = ARM_GIC_DIST_BASE;
+	struct kvm_create_device gic_device = {
+		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+	};
+	struct kvm_device_attr cpu_if_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_CPU,
+		.addr	= (u64)(unsigned long)&cpu_if_addr,
+	};
+	struct kvm_device_attr dist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
+		.addr	= (u64)(unsigned long)&dist_addr,
+	};
+
+	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
+	if (err)
+		return err;
+
+	gic_fd = gic_device.fd;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	if (err)
+		goto out_err;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	if (err)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	close(gic_fd);
+	gic_fd = -1;
+	return err;
+}
+
+static int gic__create_irqchip(struct kvm *kvm)
 {
 	int err;
 	struct kvm_arm_device_addr gic_addr[] = {
@@ -23,12 +66,6 @@ int gic__init_irqchip(struct kvm *kvm)
 		}
 	};
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
 	if (err)
 		return err;
@@ -41,6 +78,24 @@ int gic__init_irqchip(struct kvm *kvm)
 	return err;
 }
 
+int gic__create(struct kvm *kvm)
+{
+	int err;
+
+	if (kvm->nrcpus > GIC_MAX_CPUS) {
+		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
+				kvm->nrcpus, GIC_MAX_CPUS);
+		kvm->nrcpus = GIC_MAX_CPUS;
+	}
+
+	/* Try the new way first, and fallback on legacy method otherwise */
+	err = gic__create_device(kvm);
+	if (err)
+		err = gic__create_irqchip(kvm);
+
+	return err;
+}
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 5a36f2c..44859f7 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -24,7 +24,7 @@
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__init_irqchip(struct kvm *kvm);
+int gic__create(struct kvm *kvm);
 void gic__generate_fdt_nodes(void *fdt, u32 phandle);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 58ad9fa..bcd2533 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -81,7 +81,7 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
-	/* Initialise the virtual GIC. */
-	if (gic__init_irqchip(kvm))
-		die("Failed to initialise virtual GIC");
+	/* Create the virtual GIC. */
+	if (gic__create(kvm))
+		die("Failed to create virtual GIC");
 }
-- 
2.3.5

[PATCH v3 02/10] AArch{32, 64}: use KVM_CREATE_DEVICE & co to instanciate the GIC

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

As of 3.14, KVM/arm supports the creation/configuration of the GIC through
a more generic device API, which is now the preferred way to do so.

Plumb the new API in, and allow the old code to be used as a fallback.

[Andre: Rename some functions on the way to differentiate between
creation and initialisation more clearly and fix error path.]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                    | 69 +++++++++++++++++++++++++++++++++++++++-----
 arm/include/arm-common/gic.h |  2 +-
 arm/kvm.c                    |  6 ++--
 3 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 5d8cbe6..1ff3663 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -7,7 +7,50 @@
 #include <linux/byteorder.h>
 #include <linux/kvm.h>
 
-int gic__init_irqchip(struct kvm *kvm)
+static int gic_fd = -1;
+
+static int gic__create_device(struct kvm *kvm)
+{
+	int err;
+	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
+	u64 dist_addr = ARM_GIC_DIST_BASE;
+	struct kvm_create_device gic_device = {
+		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+	};
+	struct kvm_device_attr cpu_if_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_CPU,
+		.addr	= (u64)(unsigned long)&cpu_if_addr,
+	};
+	struct kvm_device_attr dist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
+		.addr	= (u64)(unsigned long)&dist_addr,
+	};
+
+	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
+	if (err)
+		return err;
+
+	gic_fd = gic_device.fd;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	if (err)
+		goto out_err;
+
+	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &dist_attr);
+	if (err)
+		goto out_err;
+
+	return 0;
+
+out_err:
+	close(gic_fd);
+	gic_fd = -1;
+	return err;
+}
+
+static int gic__create_irqchip(struct kvm *kvm)
 {
 	int err;
 	struct kvm_arm_device_addr gic_addr[] = {
@@ -23,12 +66,6 @@ int gic__init_irqchip(struct kvm *kvm)
 		}
 	};
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	err = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
 	if (err)
 		return err;
@@ -41,6 +78,24 @@ int gic__init_irqchip(struct kvm *kvm)
 	return err;
 }
 
+int gic__create(struct kvm *kvm)
+{
+	int err;
+
+	if (kvm->nrcpus > GIC_MAX_CPUS) {
+		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
+				kvm->nrcpus, GIC_MAX_CPUS);
+		kvm->nrcpus = GIC_MAX_CPUS;
+	}
+
+	/* Try the new way first, and fallback on legacy method otherwise */
+	err = gic__create_device(kvm);
+	if (err)
+		err = gic__create_irqchip(kvm);
+
+	return err;
+}
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 5a36f2c..44859f7 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -24,7 +24,7 @@
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__init_irqchip(struct kvm *kvm);
+int gic__create(struct kvm *kvm);
 void gic__generate_fdt_nodes(void *fdt, u32 phandle);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 58ad9fa..bcd2533 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -81,7 +81,7 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 	madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
-	/* Initialise the virtual GIC. */
-	if (gic__init_irqchip(kvm))
-		die("Failed to initialise virtual GIC");
+	/* Create the virtual GIC. */
+	if (gic__create(kvm))
+		die("Failed to create virtual GIC");
 }
-- 
2.3.5

[PATCH v3 03/10] irq: add irq__get_nr_allocated_lines

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

The ARM GIC emulation needs to be told the number of interrupts
it has to support. As commit 1c262fa1dc7bc ("kvm tools: irq: make
irq__alloc_line generic") made the interrupt counter private,
add a new accessor returning the number of interrupt lines we've
allocated so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/irq.h | 1 +
 irq.c             | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index 4cec6f0..8a78e43 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -11,6 +11,7 @@
 struct kvm;
 
 int irq__alloc_line(void);
+int irq__get_nr_allocated_lines(void);
 
 int irq__init(struct kvm *kvm);
 int irq__exit(struct kvm *kvm);
diff --git a/irq.c b/irq.c
index 33ea8d2..71eaa05 100644
--- a/irq.c
+++ b/irq.c
@@ -7,3 +7,8 @@ int irq__alloc_line(void)
 {
 	return next_line++;
 }
+
+int irq__get_nr_allocated_lines(void)
+{
+	return next_line - KVM_IRQ_OFFSET;
+}
-- 
2.3.5

[PATCH v3 03/10] irq: add irq__get_nr_allocated_lines

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

The ARM GIC emulation needs to be told the number of interrupts
it has to support. As commit 1c262fa1dc7bc ("kvm tools: irq: make
irq__alloc_line generic") made the interrupt counter private,
add a new accessor returning the number of interrupt lines we've
allocated so far.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/irq.h | 1 +
 irq.c             | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index 4cec6f0..8a78e43 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -11,6 +11,7 @@
 struct kvm;
 
 int irq__alloc_line(void);
+int irq__get_nr_allocated_lines(void);
 
 int irq__init(struct kvm *kvm);
 int irq__exit(struct kvm *kvm);
diff --git a/irq.c b/irq.c
index 33ea8d2..71eaa05 100644
--- a/irq.c
+++ b/irq.c
@@ -7,3 +7,8 @@ int irq__alloc_line(void)
 {
 	return next_line++;
 }
+
+int irq__get_nr_allocated_lines(void)
+{
+	return next_line - KVM_IRQ_OFFSET;
+}
-- 
2.3.5

[PATCH v3 04/10] AArch{32, 64}: dynamically configure the number of GIC interrupts

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

In order to reduce the memory usage of large guests (as well
as improve performance), tell KVM about the number of interrupts
we require.

To avoid synchronization with the various device creation,
use a late_init callback to compute the GIC configuration.
[Andre: rename to gic__init_gic() to ease future expansion]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index 1ff3663..8560c9b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -1,10 +1,12 @@
 #include "kvm/fdt.h"
+#include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
 #include "arm-common/gic.h"
 
 #include <linux/byteorder.h>
+#include <linux/kernel.h>
 #include <linux/kvm.h>
 
 static int gic_fd = -1;
@@ -96,6 +98,29 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+static int gic__init_gic(struct kvm *kvm)
+{
+	int lines = irq__get_nr_allocated_lines();
+	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
+	struct kvm_device_attr nr_irqs_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
+		.addr	= (u64)(unsigned long)&nr_irqs,
+	};
+
+	/*
+	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
+	 * give us some default number of interrupts.
+	 */
+	if (gic_fd < 0)
+		return 0;
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
+		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+
+	return 0;
+}
+late_init(gic__init_gic)
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
-- 
2.3.5

[PATCH v3 04/10] AArch{32, 64}: dynamically configure the number of GIC interrupts

[Andre Przywara]

From: Marc Zyngier <marc.zyngier@arm.com>

In order to reduce the memory usage of large guests (as well
as improve performance), tell KVM about the number of interrupts
we require.

To avoid synchronization with the various device creation,
use a late_init callback to compute the GIC configuration.
[Andre: rename to gic__init_gic() to ease future expansion]

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index 1ff3663..8560c9b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -1,10 +1,12 @@
 #include "kvm/fdt.h"
+#include "kvm/irq.h"
 #include "kvm/kvm.h"
 #include "kvm/virtio.h"
 
 #include "arm-common/gic.h"
 
 #include <linux/byteorder.h>
+#include <linux/kernel.h>
 #include <linux/kvm.h>
 
 static int gic_fd = -1;
@@ -96,6 +98,29 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+static int gic__init_gic(struct kvm *kvm)
+{
+	int lines = irq__get_nr_allocated_lines();
+	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
+	struct kvm_device_attr nr_irqs_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
+		.addr	= (u64)(unsigned long)&nr_irqs,
+	};
+
+	/*
+	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
+	 * give us some default number of interrupts.
+	 */
+	if (gic_fd < 0)
+		return 0;
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
+		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+
+	return 0;
+}
+late_init(gic__init_gic)
+
 void gic__generate_fdt_nodes(void *fdt, u32 phandle)
 {
 	u64 reg_prop[] = {
-- 
2.3.5

[PATCH v3 05/10] arm: finish VGIC initialisation explicitly

[Andre Przywara]

Since Linux 3.19-rc1 there is a new API to explicitly initialise
the in-kernel GIC emulation by a userland KVM device call.
Use that to tell the kernel we are finished with the GIC
initialisation, since the automatic GIC init will only be provided
as a legacy functionality in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arm/gic.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 8560c9b..99f0d2b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -98,24 +98,43 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+/*
+ * Sets the number of used interrupts and finalizes the GIC init explicitly.
+ */
 static int gic__init_gic(struct kvm *kvm)
 {
+	int ret;
+
 	int lines = irq__get_nr_allocated_lines();
 	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
 	struct kvm_device_attr nr_irqs_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
 		.addr	= (u64)(unsigned long)&nr_irqs,
 	};
+	struct kvm_device_attr vgic_init_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
+		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
+	};
 
 	/*
 	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
-	 * give us some default number of interrupts.
+	 * give us some default number of interrupts. The GIC initialization
+	 * will be done automatically in this case.
 	 */
 	if (gic_fd < 0)
 		return 0;
 
-	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
-		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+		if (ret)
+			return ret;
+	}
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.3.5

[PATCH v3 05/10] arm: finish VGIC initialisation explicitly

[Andre Przywara]

Since Linux 3.19-rc1 there is a new API to explicitly initialise
the in-kernel GIC emulation by a userland KVM device call.
Use that to tell the kernel we are finished with the GIC
initialisation, since the automatic GIC init will only be provided
as a legacy functionality in the future.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arm/gic.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 8560c9b..99f0d2b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -98,24 +98,43 @@ int gic__create(struct kvm *kvm)
 	return err;
 }
 
+/*
+ * Sets the number of used interrupts and finalizes the GIC init explicitly.
+ */
 static int gic__init_gic(struct kvm *kvm)
 {
+	int ret;
+
 	int lines = irq__get_nr_allocated_lines();
 	u32 nr_irqs = ALIGN(lines, 32) + GIC_SPI_IRQ_BASE;
 	struct kvm_device_attr nr_irqs_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
 		.addr	= (u64)(unsigned long)&nr_irqs,
 	};
+	struct kvm_device_attr vgic_init_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_CTRL,
+		.attr	= KVM_DEV_ARM_VGIC_CTRL_INIT,
+	};
 
 	/*
 	 * If we didn't use the KVM_CREATE_DEVICE method, KVM will
-	 * give us some default number of interrupts.
+	 * give us some default number of interrupts. The GIC initialization
+	 * will be done automatically in this case.
 	 */
 	if (gic_fd < 0)
 		return 0;
 
-	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr))
-		return ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &nr_irqs_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &nr_irqs_attr);
+		if (ret)
+			return ret;
+	}
+
+	if (!ioctl(gic_fd, KVM_HAS_DEVICE_ATTR, &vgic_init_attr)) {
+		ret = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &vgic_init_attr);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
-- 
2.3.5

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Currently we separate any incoming MMIO request into one of the ARM
memory map regions and take care to spare the GIC.
It turns out that this is unnecessary, as we only have one special
region (the IO port area in the first 64 KByte). The MMIO rbtree
takes care about unhandled MMIO ranges, so we can simply drop all the
special range checking (except that for the IO range) in
kvm_cpu__emulate_mmio().
As the GIC is handled in the kernel, a GIC MMIO access should never
reach userland (and we don't know what to do with it anyway).
This lets us delete some more code and simplifies future extensions
(like expanding the GIC regions).
To be in line with the other architectures, move the now simpler
code into a header file.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/include/arm-common/kvm-arch.h     | 12 ------------
 arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
 arm/kvm-cpu.c                         | 16 ----------------
 3 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 082131d..90d6733 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
 	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
 }
 
-static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
-{
-	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
-	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
-}
-
-static inline bool arm_addr_in_pci_region(u64 phys_addr)
-{
-	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
-	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
-}
-
 struct kvm_arch {
 	/*
 	 * We may have to align the guest memory for virtio, so keep the
diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
index 36c7872..329979a 100644
--- a/arm/include/arm-common/kvm-cpu-arch.h
+++ b/arm/include/arm-common/kvm-cpu-arch.h
@@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
 	return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-			   u32 len, u8 is_write);
+static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
+					 u8 *data, u32 len, u8 is_write)
+{
+	if (arm_addr_in_ioport_region(phys_addr)) {
+		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
+		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
+
+		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
+	}
+
+	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
+}
 
 unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
 
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index ab08815..7780251 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
 	return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-			   u32 len, u8 is_write)
-{
-	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
-		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-	} else if (arm_addr_in_ioport_region(phys_addr)) {
-		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
-		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
-	} else if (arm_addr_in_pci_region(phys_addr)) {
-		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-	}
-
-	return false;
-}
-
 void kvm_cpu__show_page_tables(struct kvm_cpu *vcpu)
 {
 }
-- 
2.3.5

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Currently we separate any incoming MMIO request into one of the ARM
memory map regions and take care to spare the GIC.
It turns out that this is unnecessary, as we only have one special
region (the IO port area in the first 64 KByte). The MMIO rbtree
takes care about unhandled MMIO ranges, so we can simply drop all the
special range checking (except that for the IO range) in
kvm_cpu__emulate_mmio().
As the GIC is handled in the kernel, a GIC MMIO access should never
reach userland (and we don't know what to do with it anyway).
This lets us delete some more code and simplifies future extensions
(like expanding the GIC regions).
To be in line with the other architectures, move the now simpler
code into a header file.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/include/arm-common/kvm-arch.h     | 12 ------------
 arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
 arm/kvm-cpu.c                         | 16 ----------------
 3 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 082131d..90d6733 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
 	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
 }
 
-static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
-{
-	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
-	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
-}
-
-static inline bool arm_addr_in_pci_region(u64 phys_addr)
-{
-	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
-	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
-}
-
 struct kvm_arch {
 	/*
 	 * We may have to align the guest memory for virtio, so keep the
diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
index 36c7872..329979a 100644
--- a/arm/include/arm-common/kvm-cpu-arch.h
+++ b/arm/include/arm-common/kvm-cpu-arch.h
@@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
 	return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-			   u32 len, u8 is_write);
+static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
+					 u8 *data, u32 len, u8 is_write)
+{
+	if (arm_addr_in_ioport_region(phys_addr)) {
+		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
+		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
+
+		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
+	}
+
+	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
+}
 
 unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
 
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index ab08815..7780251 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
 	return false;
 }
 
-bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
-			   u32 len, u8 is_write)
-{
-	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
-		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-	} else if (arm_addr_in_ioport_region(phys_addr)) {
-		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
-		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
-		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
-	} else if (arm_addr_in_pci_region(phys_addr)) {
-		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
-	}
-
-	return false;
-}
-
 void kvm_cpu__show_page_tables(struct kvm_cpu *vcpu)
 {
 }
-- 
2.3.5

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Currently we separate any incoming MMIO request into one of the ARM
> memory map regions and take care to spare the GIC.
> It turns out that this is unnecessary, as we only have one special
> region (the IO port area in the first 64 KByte). The MMIO rbtree
> takes care about unhandled MMIO ranges, so we can simply drop all the
> special range checking (except that for the IO range) in
> kvm_cpu__emulate_mmio().
> As the GIC is handled in the kernel, a GIC MMIO access should never
> reach userland (and we don't know what to do with it anyway).
> This lets us delete some more code and simplifies future extensions
> (like expanding the GIC regions).
> To be in line with the other architectures, move the now simpler
> code into a header file.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>  arm/kvm-cpu.c                         | 16 ----------------
>  3 files changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 082131d..90d6733 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>  }
>  
> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
> -{
> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
> -}
> -
> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
> -{
> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
> -}
> -
>  struct kvm_arch {
>  	/*
>  	 * We may have to align the guest memory for virtio, so keep the
> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
> index 36c7872..329979a 100644
> --- a/arm/include/arm-common/kvm-cpu-arch.h
> +++ b/arm/include/arm-common/kvm-cpu-arch.h
> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>  	return false;
>  }
>  
> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
> -			   u32 len, u8 is_write);
> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
> +					 u8 *data, u32 len, u8 is_write)
> +{
> +	if (arm_addr_in_ioport_region(phys_addr)) {
> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
> +
> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
> +	}
> +
> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> +}
>  
>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>  
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index ab08815..7780251 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>  	return false;
>  }
>  
> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
> -			   u32 len, u8 is_write)
> -{
> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
> -	} else if (arm_addr_in_pci_region(phys_addr)) {
> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> -	}

Can you explain why this arm_addr_in_pci_region(phys_addr) check has
disappeared in your updated version on this function? It may be a non
issue, but I'd very much like to understand.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Currently we separate any incoming MMIO request into one of the ARM
> memory map regions and take care to spare the GIC.
> It turns out that this is unnecessary, as we only have one special
> region (the IO port area in the first 64 KByte). The MMIO rbtree
> takes care about unhandled MMIO ranges, so we can simply drop all the
> special range checking (except that for the IO range) in
> kvm_cpu__emulate_mmio().
> As the GIC is handled in the kernel, a GIC MMIO access should never
> reach userland (and we don't know what to do with it anyway).
> This lets us delete some more code and simplifies future extensions
> (like expanding the GIC regions).
> To be in line with the other architectures, move the now simpler
> code into a header file.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>  arm/kvm-cpu.c                         | 16 ----------------
>  3 files changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 082131d..90d6733 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>  }
>  
> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
> -{
> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
> -}
> -
> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
> -{
> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
> -}
> -
>  struct kvm_arch {
>  	/*
>  	 * We may have to align the guest memory for virtio, so keep the
> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
> index 36c7872..329979a 100644
> --- a/arm/include/arm-common/kvm-cpu-arch.h
> +++ b/arm/include/arm-common/kvm-cpu-arch.h
> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>  	return false;
>  }
>  
> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
> -			   u32 len, u8 is_write);
> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
> +					 u8 *data, u32 len, u8 is_write)
> +{
> +	if (arm_addr_in_ioport_region(phys_addr)) {
> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
> +
> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
> +	}
> +
> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> +}
>  
>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>  
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index ab08815..7780251 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>  	return false;
>  }
>  
> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
> -			   u32 len, u8 is_write)
> -{
> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
> -	} else if (arm_addr_in_pci_region(phys_addr)) {
> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
> -	}

Can you explain why this arm_addr_in_pci_region(phys_addr) check has
disappeared in your updated version on this function? It may be a non
issue, but I'd very much like to understand.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Hi Marc,

On 06/17/2015 01:48 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently we separate any incoming MMIO request into one of the ARM
>> memory map regions and take care to spare the GIC.
>> It turns out that this is unnecessary, as we only have one special
>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>> takes care about unhandled MMIO ranges, so we can simply drop all the
>> special range checking (except that for the IO range) in
>> kvm_cpu__emulate_mmio().
>> As the GIC is handled in the kernel, a GIC MMIO access should never
>> reach userland (and we don't know what to do with it anyway).
>> This lets us delete some more code and simplifies future extensions
>> (like expanding the GIC regions).
>> To be in line with the other architectures, move the now simpler
>> code into a header file.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>>  arm/kvm-cpu.c                         | 16 ----------------
>>  3 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..90d6733 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>  }
>>  
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> -{
>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>> -}
>> -
>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>> -{
>> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>> -}
>> -
>>  struct kvm_arch {
>>  	/*
>>  	 * We may have to align the guest memory for virtio, so keep the
>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>> index 36c7872..329979a 100644
>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>>  	return false;
>>  }
>>  
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> -			   u32 len, u8 is_write);
>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>> +					 u8 *data, u32 len, u8 is_write)
>> +{
>> +	if (arm_addr_in_ioport_region(phys_addr)) {
>> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> +
>> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> +	}
>> +
>> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> +}
>>  
>>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>  
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..7780251 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>  	return false;
>>  }
>>  
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> -			   u32 len, u8 is_write)
>> -{
>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
>> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> -	} else if (arm_addr_in_pci_region(phys_addr)) {
>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> -	}
> 
> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
> disappeared in your updated version on this function? It may be a non
> issue, but I'd very much like to understand.

If you look above the calls to kvm__emulate_mmio() are exactly the same
for the PCI and the virtio_mmio region, also as the areas are
non-overlapping the if branches can be reordered.
arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
while arm_addr_in_pci_region() gives true between 1GB and 2GB.

So this translates into: do kvm__emulate_io() for anything below 64K and
kvm__emulate_mmio() for everything else except for the GIC area,
admittedly in a quite convoluted way.

So my patch just removes the check for the GIC region and rewrites it to
match that description in the last sentence, with the rationale given in
the commit message.
Does that make sense?
If you desperately want some extra barfing for misguided GIC requests,
I'd rather introduce that to the "no match" code path in
kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
panic() handlers.

Cheers,
Andre.

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Hi Marc,

On 06/17/2015 01:48 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently we separate any incoming MMIO request into one of the ARM
>> memory map regions and take care to spare the GIC.
>> It turns out that this is unnecessary, as we only have one special
>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>> takes care about unhandled MMIO ranges, so we can simply drop all the
>> special range checking (except that for the IO range) in
>> kvm_cpu__emulate_mmio().
>> As the GIC is handled in the kernel, a GIC MMIO access should never
>> reach userland (and we don't know what to do with it anyway).
>> This lets us delete some more code and simplifies future extensions
>> (like expanding the GIC regions).
>> To be in line with the other architectures, move the now simpler
>> code into a header file.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>>  arm/kvm-cpu.c                         | 16 ----------------
>>  3 files changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index 082131d..90d6733 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>  }
>>  
>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>> -{
>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>> -}
>> -
>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>> -{
>> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>> -}
>> -
>>  struct kvm_arch {
>>  	/*
>>  	 * We may have to align the guest memory for virtio, so keep the
>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>> index 36c7872..329979a 100644
>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>>  	return false;
>>  }
>>  
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> -			   u32 len, u8 is_write);
>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>> +					 u8 *data, u32 len, u8 is_write)
>> +{
>> +	if (arm_addr_in_ioport_region(phys_addr)) {
>> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> +
>> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> +	}
>> +
>> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> +}
>>  
>>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>  
>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index ab08815..7780251 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>  	return false;
>>  }
>>  
>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>> -			   u32 len, u8 is_write)
>> -{
>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
>> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>> -	} else if (arm_addr_in_pci_region(phys_addr)) {
>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>> -	}
> 
> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
> disappeared in your updated version on this function? It may be a non
> issue, but I'd very much like to understand.

If you look above the calls to kvm__emulate_mmio() are exactly the same
for the PCI and the virtio_mmio region, also as the areas are
non-overlapping the if branches can be reordered.
arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
while arm_addr_in_pci_region() gives true between 1GB and 2GB.

So this translates into: do kvm__emulate_io() for anything below 64K and
kvm__emulate_mmio() for everything else except for the GIC area,
admittedly in a quite convoluted way.

So my patch just removes the check for the GIC region and rewrites it to
match that description in the last sentence, with the rationale given in
the commit message.
Does that make sense?
If you desperately want some extra barfing for misguided GIC requests,
I'd rather introduce that to the "no match" code path in
kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
panic() handlers.

Cheers,
Andre.

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[Marc Zyngier]

On 17/06/15 14:49, Andre Przywara wrote:
> Hi Marc,
> 
> On 06/17/2015 01:48 PM, Marc Zyngier wrote:
>> On 17/06/15 12:21, Andre Przywara wrote:
>>> Currently we separate any incoming MMIO request into one of the ARM
>>> memory map regions and take care to spare the GIC.
>>> It turns out that this is unnecessary, as we only have one special
>>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>>> takes care about unhandled MMIO ranges, so we can simply drop all the
>>> special range checking (except that for the IO range) in
>>> kvm_cpu__emulate_mmio().
>>> As the GIC is handled in the kernel, a GIC MMIO access should never
>>> reach userland (and we don't know what to do with it anyway).
>>> This lets us delete some more code and simplifies future extensions
>>> (like expanding the GIC regions).
>>> To be in line with the other architectures, move the now simpler
>>> code into a header file.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>>>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>>>  arm/kvm-cpu.c                         | 16 ----------------
>>>  3 files changed, 12 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index 082131d..90d6733 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>>  }
>>>  
>>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>>> -{
>>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>>> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>> -}
>>> -
>>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>>> -{
>>> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>>> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>>> -}
>>> -
>>>  struct kvm_arch {
>>>  	/*
>>>  	 * We may have to align the guest memory for virtio, so keep the
>>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>>> index 36c7872..329979a 100644
>>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>>>  	return false;
>>>  }
>>>  
>>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>> -			   u32 len, u8 is_write);
>>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>>> +					 u8 *data, u32 len, u8 is_write)
>>> +{
>>> +	if (arm_addr_in_ioport_region(phys_addr)) {
>>> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>>> +
>>> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>>> +	}
>>> +
>>> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> +}
>>>  
>>>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>>  
>>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>>> index ab08815..7780251 100644
>>> --- a/arm/kvm-cpu.c
>>> +++ b/arm/kvm-cpu.c
>>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>>  	return false;
>>>  }
>>>  
>>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>> -			   u32 len, u8 is_write)
>>> -{
>>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>>> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>>> -	} else if (arm_addr_in_pci_region(phys_addr)) {
>>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> -	}
>>
>> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
>> disappeared in your updated version on this function? It may be a non
>> issue, but I'd very much like to understand.
> 
> If you look above the calls to kvm__emulate_mmio() are exactly the same
> for the PCI and the virtio_mmio region, also as the areas are
> non-overlapping the if branches can be reordered.
> arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
> while arm_addr_in_pci_region() gives true between 1GB and 2GB.
> 
> So this translates into: do kvm__emulate_io() for anything below 64K and
> kvm__emulate_mmio() for everything else except for the GIC area,
> admittedly in a quite convoluted way.
> 
> So my patch just removes the check for the GIC region and rewrites it to
> match that description in the last sentence, with the rationale given in
> the commit message.
> Does that make sense?
> If you desperately want some extra barfing for misguided GIC requests,
> I'd rather introduce that to the "no match" code path in
> kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
> panic() handlers.

No, that's fine.

I just wondered what was the rational behind having the
arm_addr_in_pci_region() call there. It might have guarded something,
but if you're absolutely positive that this doesn't cause a regression,
that's OK with me.

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Marc Zyngier]

On 17/06/15 14:49, Andre Przywara wrote:
> Hi Marc,
> 
> On 06/17/2015 01:48 PM, Marc Zyngier wrote:
>> On 17/06/15 12:21, Andre Przywara wrote:
>>> Currently we separate any incoming MMIO request into one of the ARM
>>> memory map regions and take care to spare the GIC.
>>> It turns out that this is unnecessary, as we only have one special
>>> region (the IO port area in the first 64 KByte). The MMIO rbtree
>>> takes care about unhandled MMIO ranges, so we can simply drop all the
>>> special range checking (except that for the IO range) in
>>> kvm_cpu__emulate_mmio().
>>> As the GIC is handled in the kernel, a GIC MMIO access should never
>>> reach userland (and we don't know what to do with it anyway).
>>> This lets us delete some more code and simplifies future extensions
>>> (like expanding the GIC regions).
>>> To be in line with the other architectures, move the now simpler
>>> code into a header file.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  arm/include/arm-common/kvm-arch.h     | 12 ------------
>>>  arm/include/arm-common/kvm-cpu-arch.h | 14 ++++++++++++--
>>>  arm/kvm-cpu.c                         | 16 ----------------
>>>  3 files changed, 12 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>>> index 082131d..90d6733 100644
>>> --- a/arm/include/arm-common/kvm-arch.h
>>> +++ b/arm/include/arm-common/kvm-arch.h
>>> @@ -45,18 +45,6 @@ static inline bool arm_addr_in_ioport_region(u64 phys_addr)
>>>  	return phys_addr >= KVM_IOPORT_AREA && phys_addr < limit;
>>>  }
>>>  
>>> -static inline bool arm_addr_in_virtio_mmio_region(u64 phys_addr)
>>> -{
>>> -	u64 limit = KVM_VIRTIO_MMIO_AREA + ARM_VIRTIO_MMIO_SIZE;
>>> -	return phys_addr >= KVM_VIRTIO_MMIO_AREA && phys_addr < limit;
>>> -}
>>> -
>>> -static inline bool arm_addr_in_pci_region(u64 phys_addr)
>>> -{
>>> -	u64 limit = KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE + ARM_PCI_MMIO_SIZE;
>>> -	return phys_addr >= KVM_PCI_CFG_AREA && phys_addr < limit;
>>> -}
>>> -
>>>  struct kvm_arch {
>>>  	/*
>>>  	 * We may have to align the guest memory for virtio, so keep the
>>> diff --git a/arm/include/arm-common/kvm-cpu-arch.h b/arm/include/arm-common/kvm-cpu-arch.h
>>> index 36c7872..329979a 100644
>>> --- a/arm/include/arm-common/kvm-cpu-arch.h
>>> +++ b/arm/include/arm-common/kvm-cpu-arch.h
>>> @@ -44,8 +44,18 @@ static inline bool kvm_cpu__emulate_io(struct kvm_cpu *vcpu, u16 port, void *dat
>>>  	return false;
>>>  }
>>>  
>>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>> -			   u32 len, u8 is_write);
>>> +static inline bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr,
>>> +					 u8 *data, u32 len, u8 is_write)
>>> +{
>>> +	if (arm_addr_in_ioport_region(phys_addr)) {
>>> +		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>> +		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>>> +
>>> +		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>>> +	}
>>> +
>>> +	return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> +}
>>>  
>>>  unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu);
>>>  
>>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>>> index ab08815..7780251 100644
>>> --- a/arm/kvm-cpu.c
>>> +++ b/arm/kvm-cpu.c
>>> @@ -139,22 +139,6 @@ bool kvm_cpu__handle_exit(struct kvm_cpu *vcpu)
>>>  	return false;
>>>  }
>>>  
>>> -bool kvm_cpu__emulate_mmio(struct kvm_cpu *vcpu, u64 phys_addr, u8 *data,
>>> -			   u32 len, u8 is_write)
>>> -{
>>> -	if (arm_addr_in_virtio_mmio_region(phys_addr)) {
>>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> -	} else if (arm_addr_in_ioport_region(phys_addr)) {
>>> -		int direction = is_write ? KVM_EXIT_IO_OUT : KVM_EXIT_IO_IN;
>>> -		u16 port = (phys_addr - KVM_IOPORT_AREA) & USHRT_MAX;
>>> -		return kvm__emulate_io(vcpu, port, data, direction, len, 1);
>>> -	} else if (arm_addr_in_pci_region(phys_addr)) {
>>> -		return kvm__emulate_mmio(vcpu, phys_addr, data, len, is_write);
>>> -	}
>>
>> Can you explain why this arm_addr_in_pci_region(phys_addr) check has
>> disappeared in your updated version on this function? It may be a non
>> issue, but I'd very much like to understand.
> 
> If you look above the calls to kvm__emulate_mmio() are exactly the same
> for the PCI and the virtio_mmio region, also as the areas are
> non-overlapping the if branches can be reordered.
> arm_addr_in_virtio_mmio_region() is true between 64k and (1GB - GIC),
> while arm_addr_in_pci_region() gives true between 1GB and 2GB.
> 
> So this translates into: do kvm__emulate_io() for anything below 64K and
> kvm__emulate_mmio() for everything else except for the GIC area,
> admittedly in a quite convoluted way.
> 
> So my patch just removes the check for the GIC region and rewrites it to
> match that description in the last sentence, with the rationale given in
> the commit message.
> Does that make sense?
> If you desperately want some extra barfing for misguided GIC requests,
> I'd rather introduce that to the "no match" code path in
> kvm__emulate_mmio or register some dummy MMIO regions for the GIC with
> panic() handlers.

No, that's fine.

I just wondered what was the rational behind having the
arm_addr_in_pci_region() call there. It might have guarded something,
but if you're absolutely positive that this doesn't cause a regression,
that's OK with me.

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Hi Will,

do you want me to respin the whole series to address the remaining minor
comments in the last four patches or do you want to take patch 01-06
already (which I think Marc has already agreed upon)?
Then I would just send an updated version of the remaining patches.

Cheers,
Andre.


....

> No, that's fine.
> 
> I just wondered what was the rational behind having the
> arm_addr_in_pci_region() call there. It might have guarded something,
> but if you're absolutely positive that this doesn't cause a regression,
> that's OK with me.
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> 	M.
> 

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Andre Przywara]

Hi Will,

do you want me to respin the whole series to address the remaining minor
comments in the last four patches or do you want to take patch 01-06
already (which I think Marc has already agreed upon)?
Then I would just send an updated version of the remaining patches.

Cheers,
Andre.


....

> No, that's fine.
> 
> I just wondered what was the rational behind having the
> arm_addr_in_pci_region() call there. It might have guarded something,
> but if you're absolutely positive that this doesn't cause a regression,
> that's OK with me.
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> 	M.
> 

Re: [PATCH v3 06/10] arm: simplify MMIO dispatching

[Will Deacon]

On Wed, Jun 24, 2015 at 02:30:05PM +0100, Andre Przywara wrote:
> do you want me to respin the whole series to address the remaining minor
> comments in the last four patches or do you want to take patch 01-06
> already (which I think Marc has already agreed upon)?
> Then I would just send an updated version of the remaining patches.

Yes, please. It's easier to keep track if you just repost the series with
the changes and acks.

Will

[PATCH v3 06/10] arm: simplify MMIO dispatching

[Will Deacon]

On Wed, Jun 24, 2015 at 02:30:05PM +0100, Andre Przywara wrote:
> do you want me to respin the whole series to address the remaining minor
> comments in the last four patches or do you want to take patch 01-06
> already (which I think Marc has already agreed upon)?
> Then I would just send an updated version of the remaining patches.

Yes, please. It's easier to keep track if you just repost the series with
the changes and acks.

Will

[PATCH v3 07/10] limit number of VCPUs on demand

[Andre Przywara]

Currently the ARM GIC checks the number of VCPUs against a fixed
limit, which is GICv2 specific. Don't pretend we know better than the
kernel and let's get rid of that explicit check.
Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
which is the way the kernel communicates having reached a VCPU limit.
If we see this and have at least brought up one VCPU already
successfully, then don't panic, but limit the number of VCPUs instead.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c     |  6 ------
 arm/kvm-cpu.c | 11 +++++++++--
 kvm-cpu.c     |  7 +++++++
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 99f0d2b..05f85a2 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -84,12 +84,6 @@ int gic__create(struct kvm *kvm)
 {
 	int err;
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	/* Try the new way first, and fallback on legacy method otherwise */
 	err = gic__create_device(kvm);
 	if (err)
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index 7780251..c1cf51d 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 	};
 
 	vcpu = calloc(1, sizeof(struct kvm_cpu));
-	if (!vcpu)
+	if (!vcpu) {
+		errno = ENOMEM;
 		return NULL;
+	}
 
 	vcpu->vcpu_fd = ioctl(kvm->vm_fd, KVM_CREATE_VCPU, cpu_id);
-	if (vcpu->vcpu_fd < 0)
+	if (vcpu->vcpu_fd < 0) {
+		if (errno == EINVAL) {
+			free(vcpu);
+			return NULL;
+		}
 		die_perror("KVM_CREATE_VCPU ioctl");
+	}
 
 	mmap_size = ioctl(kvm->sys_fd, KVM_GET_VCPU_MMAP_SIZE, 0);
 	if (mmap_size < 0)
diff --git a/kvm-cpu.c b/kvm-cpu.c
index 5d90664..7a9d689 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -222,11 +222,18 @@ int kvm_cpu__init(struct kvm *kvm)
 	for (i = 0; i < kvm->nrcpus; i++) {
 		kvm->cpus[i] = kvm_cpu__arch_init(kvm, i);
 		if (!kvm->cpus[i]) {
+			if (i > 0 && errno == EINVAL)
+				break;
 			pr_warning("unable to initialize KVM VCPU");
 			goto fail_alloc;
 		}
 	}
 
+	if (i < kvm->nrcpus) {
+		kvm->nrcpus = i;
+		printf("  # The kernel limits the number of CPUs to %d\n", i);
+	}
+
 	return 0;
 
 fail_alloc:
-- 
2.3.5

[PATCH v3 07/10] limit number of VCPUs on demand

[Andre Przywara]

Currently the ARM GIC checks the number of VCPUs against a fixed
limit, which is GICv2 specific. Don't pretend we know better than the
kernel and let's get rid of that explicit check.
Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
which is the way the kernel communicates having reached a VCPU limit.
If we see this and have at least brought up one VCPU already
successfully, then don't panic, but limit the number of VCPUs instead.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c     |  6 ------
 arm/kvm-cpu.c | 11 +++++++++--
 kvm-cpu.c     |  7 +++++++
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 99f0d2b..05f85a2 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -84,12 +84,6 @@ int gic__create(struct kvm *kvm)
 {
 	int err;
 
-	if (kvm->nrcpus > GIC_MAX_CPUS) {
-		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
-				kvm->nrcpus, GIC_MAX_CPUS);
-		kvm->nrcpus = GIC_MAX_CPUS;
-	}
-
 	/* Try the new way first, and fallback on legacy method otherwise */
 	err = gic__create_device(kvm);
 	if (err)
diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
index 7780251..c1cf51d 100644
--- a/arm/kvm-cpu.c
+++ b/arm/kvm-cpu.c
@@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
 	};
 
 	vcpu = calloc(1, sizeof(struct kvm_cpu));
-	if (!vcpu)
+	if (!vcpu) {
+		errno = ENOMEM;
 		return NULL;
+	}
 
 	vcpu->vcpu_fd = ioctl(kvm->vm_fd, KVM_CREATE_VCPU, cpu_id);
-	if (vcpu->vcpu_fd < 0)
+	if (vcpu->vcpu_fd < 0) {
+		if (errno == EINVAL) {
+			free(vcpu);
+			return NULL;
+		}
 		die_perror("KVM_CREATE_VCPU ioctl");
+	}
 
 	mmap_size = ioctl(kvm->sys_fd, KVM_GET_VCPU_MMAP_SIZE, 0);
 	if (mmap_size < 0)
diff --git a/kvm-cpu.c b/kvm-cpu.c
index 5d90664..7a9d689 100644
--- a/kvm-cpu.c
+++ b/kvm-cpu.c
@@ -222,11 +222,18 @@ int kvm_cpu__init(struct kvm *kvm)
 	for (i = 0; i < kvm->nrcpus; i++) {
 		kvm->cpus[i] = kvm_cpu__arch_init(kvm, i);
 		if (!kvm->cpus[i]) {
+			if (i > 0 && errno == EINVAL)
+				break;
 			pr_warning("unable to initialize KVM VCPU");
 			goto fail_alloc;
 		}
 	}
 
+	if (i < kvm->nrcpus) {
+		kvm->nrcpus = i;
+		printf("  # The kernel limits the number of CPUs to %d\n", i);
+	}
+
 	return 0;
 
 fail_alloc:
-- 
2.3.5

Re: [PATCH v3 07/10] limit number of VCPUs on demand

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Currently the ARM GIC checks the number of VCPUs against a fixed
> limit, which is GICv2 specific. Don't pretend we know better than the
> kernel and let's get rid of that explicit check.
> Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
> which is the way the kernel communicates having reached a VCPU limit.
> If we see this and have at least brought up one VCPU already
> successfully, then don't panic, but limit the number of VCPUs instead.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c     |  6 ------
>  arm/kvm-cpu.c | 11 +++++++++--
>  kvm-cpu.c     |  7 +++++++
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 99f0d2b..05f85a2 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -84,12 +84,6 @@ int gic__create(struct kvm *kvm)
>  {
>  	int err;
>  
> -	if (kvm->nrcpus > GIC_MAX_CPUS) {
> -		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
> -				kvm->nrcpus, GIC_MAX_CPUS);
> -		kvm->nrcpus = GIC_MAX_CPUS;
> -	}
> -
>  	/* Try the new way first, and fallback on legacy method otherwise */
>  	err = gic__create_device(kvm);
>  	if (err)
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index 7780251..c1cf51d 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>  	};
>  
>  	vcpu = calloc(1, sizeof(struct kvm_cpu));
> -	if (!vcpu)
> +	if (!vcpu) {
> +		errno = ENOMEM;
>  		return NULL;
> +	}

Isn't errno already set when calloc fails?

>  
>  	vcpu->vcpu_fd = ioctl(kvm->vm_fd, KVM_CREATE_VCPU, cpu_id);
> -	if (vcpu->vcpu_fd < 0)
> +	if (vcpu->vcpu_fd < 0) {
> +		if (errno == EINVAL) {
> +			free(vcpu);
> +			return NULL;
> +		}
>  		die_perror("KVM_CREATE_VCPU ioctl");
> +	}
>  
>  	mmap_size = ioctl(kvm->sys_fd, KVM_GET_VCPU_MMAP_SIZE, 0);
>  	if (mmap_size < 0)
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index 5d90664..7a9d689 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -222,11 +222,18 @@ int kvm_cpu__init(struct kvm *kvm)
>  	for (i = 0; i < kvm->nrcpus; i++) {
>  		kvm->cpus[i] = kvm_cpu__arch_init(kvm, i);
>  		if (!kvm->cpus[i]) {
> +			if (i > 0 && errno == EINVAL)
> +				break;
>  			pr_warning("unable to initialize KVM VCPU");
>  			goto fail_alloc;
>  		}
>  	}
>  
> +	if (i < kvm->nrcpus) {
> +		kvm->nrcpus = i;
> +		printf("  # The kernel limits the number of CPUs to %d\n", i);
> +	}
> +
>  	return 0;
>  
>  fail_alloc:
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 07/10] limit number of VCPUs on demand

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Currently the ARM GIC checks the number of VCPUs against a fixed
> limit, which is GICv2 specific. Don't pretend we know better than the
> kernel and let's get rid of that explicit check.
> Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
> which is the way the kernel communicates having reached a VCPU limit.
> If we see this and have at least brought up one VCPU already
> successfully, then don't panic, but limit the number of VCPUs instead.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c     |  6 ------
>  arm/kvm-cpu.c | 11 +++++++++--
>  kvm-cpu.c     |  7 +++++++
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 99f0d2b..05f85a2 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -84,12 +84,6 @@ int gic__create(struct kvm *kvm)
>  {
>  	int err;
>  
> -	if (kvm->nrcpus > GIC_MAX_CPUS) {
> -		pr_warning("%d CPUS greater than maximum of %d -- truncating\n",
> -				kvm->nrcpus, GIC_MAX_CPUS);
> -		kvm->nrcpus = GIC_MAX_CPUS;
> -	}
> -
>  	/* Try the new way first, and fallback on legacy method otherwise */
>  	err = gic__create_device(kvm);
>  	if (err)
> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
> index 7780251..c1cf51d 100644
> --- a/arm/kvm-cpu.c
> +++ b/arm/kvm-cpu.c
> @@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>  	};
>  
>  	vcpu = calloc(1, sizeof(struct kvm_cpu));
> -	if (!vcpu)
> +	if (!vcpu) {
> +		errno = ENOMEM;
>  		return NULL;
> +	}

Isn't errno already set when calloc fails?

>  
>  	vcpu->vcpu_fd = ioctl(kvm->vm_fd, KVM_CREATE_VCPU, cpu_id);
> -	if (vcpu->vcpu_fd < 0)
> +	if (vcpu->vcpu_fd < 0) {
> +		if (errno == EINVAL) {
> +			free(vcpu);
> +			return NULL;
> +		}
>  		die_perror("KVM_CREATE_VCPU ioctl");
> +	}
>  
>  	mmap_size = ioctl(kvm->sys_fd, KVM_GET_VCPU_MMAP_SIZE, 0);
>  	if (mmap_size < 0)
> diff --git a/kvm-cpu.c b/kvm-cpu.c
> index 5d90664..7a9d689 100644
> --- a/kvm-cpu.c
> +++ b/kvm-cpu.c
> @@ -222,11 +222,18 @@ int kvm_cpu__init(struct kvm *kvm)
>  	for (i = 0; i < kvm->nrcpus; i++) {
>  		kvm->cpus[i] = kvm_cpu__arch_init(kvm, i);
>  		if (!kvm->cpus[i]) {
> +			if (i > 0 && errno == EINVAL)
> +				break;
>  			pr_warning("unable to initialize KVM VCPU");
>  			goto fail_alloc;
>  		}
>  	}
>  
> +	if (i < kvm->nrcpus) {
> +		kvm->nrcpus = i;
> +		printf("  # The kernel limits the number of CPUs to %d\n", i);
> +	}
> +
>  	return 0;
>  
>  fail_alloc:
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 07/10] limit number of VCPUs on demand

[Andre Przywara]

On 06/17/2015 01:53 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently the ARM GIC checks the number of VCPUs against a fixed
>> limit, which is GICv2 specific. Don't pretend we know better than the
>> kernel and let's get rid of that explicit check.
>> Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
>> which is the way the kernel communicates having reached a VCPU limit.
>> If we see this and have at least brought up one VCPU already
>> successfully, then don't panic, but limit the number of VCPUs instead.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

...

>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index 7780251..c1cf51d 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>>  	};
>>  
>>  	vcpu = calloc(1, sizeof(struct kvm_cpu));
>> -	if (!vcpu)
>> +	if (!vcpu) {
>> +		errno = ENOMEM;
>>  		return NULL;
>> +	}
> 
> Isn't errno already set when calloc fails?

Ah yes, that seems to be true at least for glibc or UNIX 98, according
to the manpage. I was misguided by the fact that calloc is not a
syscall. So I can drop this hunk.

Thanks,
Andre.

[PATCH v3 07/10] limit number of VCPUs on demand

[Andre Przywara]

On 06/17/2015 01:53 PM, Marc Zyngier wrote:
> On 17/06/15 12:21, Andre Przywara wrote:
>> Currently the ARM GIC checks the number of VCPUs against a fixed
>> limit, which is GICv2 specific. Don't pretend we know better than the
>> kernel and let's get rid of that explicit check.
>> Instead be more relaxed about KVM_CREATE_VCPU failing with EINVAL,
>> which is the way the kernel communicates having reached a VCPU limit.
>> If we see this and have at least brought up one VCPU already
>> successfully, then don't panic, but limit the number of VCPUs instead.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

...

>> diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c
>> index 7780251..c1cf51d 100644
>> --- a/arm/kvm-cpu.c
>> +++ b/arm/kvm-cpu.c
>> @@ -47,12 +47,19 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id)
>>  	};
>>  
>>  	vcpu = calloc(1, sizeof(struct kvm_cpu));
>> -	if (!vcpu)
>> +	if (!vcpu) {
>> +		errno = ENOMEM;
>>  		return NULL;
>> +	}
> 
> Isn't errno already set when calloc fails?

Ah yes, that seems to be true at least for glibc or UNIX 98, according
to the manpage. I was misguided by the fact that calloc is not a
syscall. So I can drop this hunk.

Thanks,
Andre.

[PATCH v3 08/10] arm: prepare for instantiating different IRQ chip devices

[Andre Przywara]

Extend the vGIC handling code to potentially deal with different IRQ
chip devices instead of hard-coding the GICv2 in.
We extend most vGIC functions to take a type parameter, but still put
GICv2 in at the top for the time being.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch32/arm-cpu.c        |  2 +-
 arm/aarch64/arm-cpu.c        |  2 +-
 arm/gic.c                    | 44 +++++++++++++++++++++++++++++++++++---------
 arm/include/arm-common/gic.h |  8 ++++++--
 arm/kvm.c                    |  2 +-
 5 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
index 946e443..d8d6293 100644
--- a/arm/aarch32/arm-cpu.c
+++ b/arm/aarch32/arm-cpu.c
@@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
 
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index 8efe877..f702b9e 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index 05f85a2..b6c5868 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -11,13 +11,13 @@
 
 static int gic_fd = -1;
 
-static int gic__create_device(struct kvm *kvm)
+static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
 	struct kvm_create_device gic_device = {
-		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+		.flags	= 0,
 	};
 	struct kvm_device_attr cpu_if_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -26,17 +26,27 @@ static int gic__create_device(struct kvm *kvm)
 	};
 	struct kvm_device_attr dist_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
-		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
+		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
+		break;
+	}
+
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
 	if (err)
 		return err;
 
 	gic_fd = gic_device.fd;
 
-	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	switch (type) {
+	case IRQCHIP_GICV2:
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+		break;
+	}
 	if (err)
 		goto out_err;
 
@@ -80,13 +90,20 @@ static int gic__create_irqchip(struct kvm *kvm)
 	return err;
 }
 
-int gic__create(struct kvm *kvm)
+int gic__create(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		break;
+	default:
+		return -ENODEV;
+	}
+
 	/* Try the new way first, and fallback on legacy method otherwise */
-	err = gic__create_device(kvm);
-	if (err)
+	err = gic__create_device(kvm, type);
+	if (err && type == IRQCHIP_GICV2)
 		err = gic__create_irqchip(kvm);
 
 	return err;
@@ -134,15 +151,24 @@ static int gic__init_gic(struct kvm *kvm)
 }
 late_init(gic__init_gic)
 
-void gic__generate_fdt_nodes(void *fdt, u32 phandle)
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 {
+	const char *compatible;
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
 		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		compatible = "arm,cortex-a15-gic";
+		break;
+	default:
+		return;
+	}
+
 	_FDT(fdt_begin_node(fdt, "intc"));
-	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
+	_FDT(fdt_property_string(fdt, "compatible", compatible));
 	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
 	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 44859f7..2ed76fa 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,10 +21,14 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
+enum irqchip_type {
+	IRQCHIP_GICV2
+};
+
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__create(struct kvm *kvm);
-void gic__generate_fdt_nodes(void *fdt, u32 phandle);
+int gic__create(struct kvm *kvm, enum irqchip_type type);
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index bcd2533..f9685c2 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm))
+	if (gic__create(kvm, IRQCHIP_GICV2))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

[PATCH v3 08/10] arm: prepare for instantiating different IRQ chip devices

[Andre Przywara]

Extend the vGIC handling code to potentially deal with different IRQ
chip devices instead of hard-coding the GICv2 in.
We extend most vGIC functions to take a type parameter, but still put
GICv2 in at the top for the time being.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch32/arm-cpu.c        |  2 +-
 arm/aarch64/arm-cpu.c        |  2 +-
 arm/gic.c                    | 44 +++++++++++++++++++++++++++++++++++---------
 arm/include/arm-common/gic.h |  8 ++++++--
 arm/kvm.c                    |  2 +-
 5 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
index 946e443..d8d6293 100644
--- a/arm/aarch32/arm-cpu.c
+++ b/arm/aarch32/arm-cpu.c
@@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
 
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index 8efe877..f702b9e 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle);
+	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index 05f85a2..b6c5868 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -11,13 +11,13 @@
 
 static int gic_fd = -1;
 
-static int gic__create_device(struct kvm *kvm)
+static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
 	u64 dist_addr = ARM_GIC_DIST_BASE;
 	struct kvm_create_device gic_device = {
-		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
+		.flags	= 0,
 	};
 	struct kvm_device_attr cpu_if_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -26,17 +26,27 @@ static int gic__create_device(struct kvm *kvm)
 	};
 	struct kvm_device_attr dist_attr = {
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
-		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
+		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
+		break;
+	}
+
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
 	if (err)
 		return err;
 
 	gic_fd = gic_device.fd;
 
-	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+	switch (type) {
+	case IRQCHIP_GICV2:
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
+		break;
+	}
 	if (err)
 		goto out_err;
 
@@ -80,13 +90,20 @@ static int gic__create_irqchip(struct kvm *kvm)
 	return err;
 }
 
-int gic__create(struct kvm *kvm)
+int gic__create(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		break;
+	default:
+		return -ENODEV;
+	}
+
 	/* Try the new way first, and fallback on legacy method otherwise */
-	err = gic__create_device(kvm);
-	if (err)
+	err = gic__create_device(kvm, type);
+	if (err && type == IRQCHIP_GICV2)
 		err = gic__create_irqchip(kvm);
 
 	return err;
@@ -134,15 +151,24 @@ static int gic__init_gic(struct kvm *kvm)
 }
 late_init(gic__init_gic)
 
-void gic__generate_fdt_nodes(void *fdt, u32 phandle)
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 {
+	const char *compatible;
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
 		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
 	};
 
+	switch (type) {
+	case IRQCHIP_GICV2:
+		compatible = "arm,cortex-a15-gic";
+		break;
+	default:
+		return;
+	}
+
 	_FDT(fdt_begin_node(fdt, "intc"));
-	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
+	_FDT(fdt_property_string(fdt, "compatible", compatible));
 	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
 	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
 	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 44859f7..2ed76fa 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -21,10 +21,14 @@
 #define GIC_MAX_CPUS			8
 #define GIC_MAX_IRQ			255
 
+enum irqchip_type {
+	IRQCHIP_GICV2
+};
+
 struct kvm;
 
 int gic__alloc_irqnum(void);
-int gic__create(struct kvm *kvm);
-void gic__generate_fdt_nodes(void *fdt, u32 phandle);
+int gic__create(struct kvm *kvm, enum irqchip_type type);
+void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
 
 #endif /* ARM_COMMON__GIC_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index bcd2533..f9685c2 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm))
+	if (gic__create(kvm, IRQCHIP_GICV2))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

Re: [PATCH v3 08/10] arm: prepare for instantiating different IRQ chip devices

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Extend the vGIC handling code to potentially deal with different IRQ
> chip devices instead of hard-coding the GICv2 in.
> We extend most vGIC functions to take a type parameter, but still put
> GICv2 in at the top for the time being.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch32/arm-cpu.c        |  2 +-
>  arm/aarch64/arm-cpu.c        |  2 +-
>  arm/gic.c                    | 44 +++++++++++++++++++++++++++++++++++---------
>  arm/include/arm-common/gic.h |  8 ++++++--
>  arm/kvm.c                    |  2 +-
>  5 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
> index 946e443..d8d6293 100644
> --- a/arm/aarch32/arm-cpu.c
> +++ b/arm/aarch32/arm-cpu.c
> @@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
>  
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index 8efe877..f702b9e 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index 05f85a2..b6c5868 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -11,13 +11,13 @@
>  
>  static int gic_fd = -1;
>  
> -static int gic__create_device(struct kvm *kvm)
> +static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>  	struct kvm_create_device gic_device = {
> -		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
> +		.flags	= 0,
>  	};
>  	struct kvm_device_attr cpu_if_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> @@ -26,17 +26,27 @@ static int gic__create_device(struct kvm *kvm)
>  	};
>  	struct kvm_device_attr dist_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
> +		break;
> +	}
> +
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>  	if (err)
>  		return err;
>  
>  	gic_fd = gic_device.fd;
>  
> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +		break;
> +	}
>  	if (err)
>  		goto out_err;
>  
> @@ -80,13 +90,20 @@ static int gic__create_irqchip(struct kvm *kvm)
>  	return err;
>  }
>  
> -int gic__create(struct kvm *kvm)
> +int gic__create(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
>  	/* Try the new way first, and fallback on legacy method otherwise */
> -	err = gic__create_device(kvm);
> -	if (err)
> +	err = gic__create_device(kvm, type);
> +	if (err && type == IRQCHIP_GICV2)
>  		err = gic__create_irqchip(kvm);
>  
>  	return err;
> @@ -134,15 +151,24 @@ static int gic__init_gic(struct kvm *kvm)
>  }
>  late_init(gic__init_gic)
>  
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  {
> +	const char *compatible;
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>  		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		compatible = "arm,cortex-a15-gic";
> +		break;
> +	default:
> +		return;
> +	}
> +
>  	_FDT(fdt_begin_node(fdt, "intc"));
> -	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
> +	_FDT(fdt_property_string(fdt, "compatible", compatible));
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 44859f7..2ed76fa 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,10 +21,14 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> +enum irqchip_type {
> +	IRQCHIP_GICV2

Nit: comma after each member of enum/struct.

> +};
> +
>  struct kvm;
>  
>  int gic__alloc_irqnum(void);
> -int gic__create(struct kvm *kvm);
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle);
> +int gic__create(struct kvm *kvm, enum irqchip_type type);
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
>  
>  #endif /* ARM_COMMON__GIC_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index bcd2533..f9685c2 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm))
> +	if (gic__create(kvm, IRQCHIP_GICV2))
>  		die("Failed to create virtual GIC");
>  }
> 

Otherwise,

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 08/10] arm: prepare for instantiating different IRQ chip devices

[Marc Zyngier]

On 17/06/15 12:21, Andre Przywara wrote:
> Extend the vGIC handling code to potentially deal with different IRQ
> chip devices instead of hard-coding the GICv2 in.
> We extend most vGIC functions to take a type parameter, but still put
> GICv2 in at the top for the time being.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch32/arm-cpu.c        |  2 +-
>  arm/aarch64/arm-cpu.c        |  2 +-
>  arm/gic.c                    | 44 +++++++++++++++++++++++++++++++++++---------
>  arm/include/arm-common/gic.h |  8 ++++++--
>  arm/kvm.c                    |  2 +-
>  5 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/arm/aarch32/arm-cpu.c b/arm/aarch32/arm-cpu.c
> index 946e443..d8d6293 100644
> --- a/arm/aarch32/arm-cpu.c
> +++ b/arm/aarch32/arm-cpu.c
> @@ -12,7 +12,7 @@ static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
>  
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index 8efe877..f702b9e 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index 05f85a2..b6c5868 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -11,13 +11,13 @@
>  
>  static int gic_fd = -1;
>  
> -static int gic__create_device(struct kvm *kvm)
> +static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  	u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
>  	u64 dist_addr = ARM_GIC_DIST_BASE;
>  	struct kvm_create_device gic_device = {
> -		.type	= KVM_DEV_TYPE_ARM_VGIC_V2,
> +		.flags	= 0,
>  	};
>  	struct kvm_device_attr cpu_if_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> @@ -26,17 +26,27 @@ static int gic__create_device(struct kvm *kvm)
>  	};
>  	struct kvm_device_attr dist_attr = {
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> -		.attr	= KVM_VGIC_V2_ADDR_TYPE_DIST,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
> +		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
> +		break;
> +	}
> +
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
>  	if (err)
>  		return err;
>  
>  	gic_fd = gic_device.fd;
>  
> -	err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
> +		break;
> +	}
>  	if (err)
>  		goto out_err;
>  
> @@ -80,13 +90,20 @@ static int gic__create_irqchip(struct kvm *kvm)
>  	return err;
>  }
>  
> -int gic__create(struct kvm *kvm)
> +int gic__create(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
>  	/* Try the new way first, and fallback on legacy method otherwise */
> -	err = gic__create_device(kvm);
> -	if (err)
> +	err = gic__create_device(kvm, type);
> +	if (err && type == IRQCHIP_GICV2)
>  		err = gic__create_irqchip(kvm);
>  
>  	return err;
> @@ -134,15 +151,24 @@ static int gic__init_gic(struct kvm *kvm)
>  }
>  late_init(gic__init_gic)
>  
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle)
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  {
> +	const char *compatible;
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
>  		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
>  	};
>  
> +	switch (type) {
> +	case IRQCHIP_GICV2:
> +		compatible = "arm,cortex-a15-gic";
> +		break;
> +	default:
> +		return;
> +	}
> +
>  	_FDT(fdt_begin_node(fdt, "intc"));
> -	_FDT(fdt_property_string(fdt, "compatible", "arm,cortex-a15-gic"));
> +	_FDT(fdt_property_string(fdt, "compatible", compatible));
>  	_FDT(fdt_property_cell(fdt, "#interrupt-cells", GIC_FDT_IRQ_NUM_CELLS));
>  	_FDT(fdt_property(fdt, "interrupt-controller", NULL, 0));
>  	_FDT(fdt_property(fdt, "reg", reg_prop, sizeof(reg_prop)));
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 44859f7..2ed76fa 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -21,10 +21,14 @@
>  #define GIC_MAX_CPUS			8
>  #define GIC_MAX_IRQ			255
>  
> +enum irqchip_type {
> +	IRQCHIP_GICV2

Nit: comma after each member of enum/struct.

> +};
> +
>  struct kvm;
>  
>  int gic__alloc_irqnum(void);
> -int gic__create(struct kvm *kvm);
> -void gic__generate_fdt_nodes(void *fdt, u32 phandle);
> +int gic__create(struct kvm *kvm, enum irqchip_type type);
> +void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type);
>  
>  #endif /* ARM_COMMON__GIC_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index bcd2533..f9685c2 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm))
> +	if (gic__create(kvm, IRQCHIP_GICV2))
>  		die("Failed to create virtual GIC");
>  }
> 

Otherwise,

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 09/10] arm: add support for supplying GICv3 redistributor addresses

[Andre Przywara]

Instead of the GIC virtual CPU interface an emulated GICv3 needs to
have accesses to its emulated redistributors trapped in the guest.
Add code to tell the kernel about the mapping if a GICv3 emulation was
requested by the user.

This contains some defines which are not (yet) in the (32 bit) header
files to allow compilation for ARM.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                         | 36 +++++++++++++++++++++++++++++++++++-
 arm/include/arm-common/gic.h      |  3 ++-
 arm/include/arm-common/kvm-arch.h |  7 +++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index b6c5868..efe4b42 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -9,7 +9,18 @@
 #include <linux/kernel.h>
 #include <linux/kvm.h>
 
+/* Those names are not defined for ARM (yet) */
+#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
+#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
+#endif
+
+#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
+#endif
+
 static int gic_fd = -1;
+static u64 gic_redists_base;
+static u64 gic_redists_size;
 
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
@@ -28,12 +39,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
+	struct kvm_device_attr redist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
+		.addr	= (u64)(unsigned long)&gic_redists_base,
+	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
 		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
 		break;
+	case IRQCHIP_GICV3:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
+		dist_attr.attr  = KVM_VGIC_V3_ADDR_TYPE_DIST;
+		break;
 	}
 
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
@@ -46,6 +66,9 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 	case IRQCHIP_GICV2:
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
 		break;
+	case IRQCHIP_GICV3:
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
+		break;
 	}
 	if (err)
 		goto out_err;
@@ -97,6 +120,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
 	switch (type) {
 	case IRQCHIP_GICV2:
 		break;
+	case IRQCHIP_GICV3:
+		gic_redists_size = kvm->cfg.nrcpus * ARM_GIC_REDIST_SIZE;
+		gic_redists_base = ARM_GIC_DIST_BASE - gic_redists_size;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -156,12 +183,19 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 	const char *compatible;
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		0, 0,				/* to be filled */
 	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		compatible = "arm,cortex-a15-gic";
+		reg_prop[2] = cpu_to_fdt64(ARM_GIC_CPUI_BASE);
+		reg_prop[3] = cpu_to_fdt64(ARM_GIC_CPUI_SIZE);
+		break;
+	case IRQCHIP_GICV3:
+		compatible = "arm,gic-v3";
+		reg_prop[2] = cpu_to_fdt64(gic_redists_base);
+		reg_prop[3] = cpu_to_fdt64(gic_redists_size);
 		break;
 	default:
 		return;
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 2ed76fa..403d93b 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -22,7 +22,8 @@
 #define GIC_MAX_IRQ			255
 
 enum irqchip_type {
-	IRQCHIP_GICV2
+	IRQCHIP_GICV2,
+	IRQCHIP_GICV3
 };
 
 struct kvm;
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 90d6733..0f5fb7f 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -30,6 +30,13 @@
 #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
 #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
 
+/*
+ * On a GICv3 there must be one redistributor per vCPU.
+ * The value here is the size for one, we multiply this at runtime with
+ * the number of requested vCPUs to get the actual size.
+ */
+#define ARM_GIC_REDIST_SIZE	0x20000
+
 #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
 
 #define KVM_VM_TYPE		0
-- 
2.3.5

[PATCH v3 09/10] arm: add support for supplying GICv3 redistributor addresses

[Andre Przywara]

Instead of the GIC virtual CPU interface an emulated GICv3 needs to
have accesses to its emulated redistributors trapped in the guest.
Add code to tell the kernel about the mapping if a GICv3 emulation was
requested by the user.

This contains some defines which are not (yet) in the (32 bit) header
files to allow compilation for ARM.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                         | 36 +++++++++++++++++++++++++++++++++++-
 arm/include/arm-common/gic.h      |  3 ++-
 arm/include/arm-common/kvm-arch.h |  7 +++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index b6c5868..efe4b42 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -9,7 +9,18 @@
 #include <linux/kernel.h>
 #include <linux/kvm.h>
 
+/* Those names are not defined for ARM (yet) */
+#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
+#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
+#endif
+
+#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
+#endif
+
 static int gic_fd = -1;
+static u64 gic_redists_base;
+static u64 gic_redists_size;
 
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
@@ -28,12 +39,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
 		.addr	= (u64)(unsigned long)&dist_addr,
 	};
+	struct kvm_device_attr redist_attr = {
+		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
+		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
+		.addr	= (u64)(unsigned long)&gic_redists_base,
+	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
 		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
 		break;
+	case IRQCHIP_GICV3:
+		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
+		dist_attr.attr  = KVM_VGIC_V3_ADDR_TYPE_DIST;
+		break;
 	}
 
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
@@ -46,6 +66,9 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 	case IRQCHIP_GICV2:
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
 		break;
+	case IRQCHIP_GICV3:
+		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
+		break;
 	}
 	if (err)
 		goto out_err;
@@ -97,6 +120,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
 	switch (type) {
 	case IRQCHIP_GICV2:
 		break;
+	case IRQCHIP_GICV3:
+		gic_redists_size = kvm->cfg.nrcpus * ARM_GIC_REDIST_SIZE;
+		gic_redists_base = ARM_GIC_DIST_BASE - gic_redists_size;
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -156,12 +183,19 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
 	const char *compatible;
 	u64 reg_prop[] = {
 		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
-		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
+		0, 0,				/* to be filled */
 	};
 
 	switch (type) {
 	case IRQCHIP_GICV2:
 		compatible = "arm,cortex-a15-gic";
+		reg_prop[2] = cpu_to_fdt64(ARM_GIC_CPUI_BASE);
+		reg_prop[3] = cpu_to_fdt64(ARM_GIC_CPUI_SIZE);
+		break;
+	case IRQCHIP_GICV3:
+		compatible = "arm,gic-v3";
+		reg_prop[2] = cpu_to_fdt64(gic_redists_base);
+		reg_prop[3] = cpu_to_fdt64(gic_redists_size);
 		break;
 	default:
 		return;
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 2ed76fa..403d93b 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -22,7 +22,8 @@
 #define GIC_MAX_IRQ			255
 
 enum irqchip_type {
-	IRQCHIP_GICV2
+	IRQCHIP_GICV2,
+	IRQCHIP_GICV3
 };
 
 struct kvm;
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 90d6733..0f5fb7f 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -30,6 +30,13 @@
 #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
 #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
 
+/*
+ * On a GICv3 there must be one redistributor per vCPU.
+ * The value here is the size for one, we multiply this at runtime with
+ * the number of requested vCPUs to get the actual size.
+ */
+#define ARM_GIC_REDIST_SIZE	0x20000
+
 #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
 
 #define KVM_VM_TYPE		0
-- 
2.3.5

Re: [PATCH v3 09/10] arm: add support for supplying GICv3 redistributor addresses

[Marc Zyngier]

On 17/06/15 12:22, Andre Przywara wrote:
> Instead of the GIC virtual CPU interface an emulated GICv3 needs to
> have accesses to its emulated redistributors trapped in the guest.
> Add code to tell the kernel about the mapping if a GICv3 emulation was
> requested by the user.
> 
> This contains some defines which are not (yet) in the (32 bit) header
> files to allow compilation for ARM.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c                         | 36 +++++++++++++++++++++++++++++++++++-
>  arm/include/arm-common/gic.h      |  3 ++-
>  arm/include/arm-common/kvm-arch.h |  7 +++++++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index b6c5868..efe4b42 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -9,7 +9,18 @@
>  #include <linux/kernel.h>
>  #include <linux/kvm.h>
>  
> +/* Those names are not defined for ARM (yet) */
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
> +#endif
> +
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
> +#endif
> +
>  static int gic_fd = -1;
> +static u64 gic_redists_base;
> +static u64 gic_redists_size;
>  
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
> @@ -28,12 +39,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
> +	struct kvm_device_attr redist_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +		.addr	= (u64)(unsigned long)&gic_redists_base,
> +	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>  		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		dist_attr.attr  = KVM_VGIC_V3_ADDR_TYPE_DIST;
> +		break;
>  	}
>  
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
> @@ -46,6 +66,9 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  	case IRQCHIP_GICV2:
>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>  		break;
> +	case IRQCHIP_GICV3:
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
> +		break;
>  	}
>  	if (err)
>  		goto out_err;
> @@ -97,6 +120,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_redists_size = kvm->cfg.nrcpus * ARM_GIC_REDIST_SIZE;
> +		gic_redists_base = ARM_GIC_DIST_BASE - gic_redists_size;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -156,12 +183,19 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	const char *compatible;
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		0, 0,				/* to be filled */
>  	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		compatible = "arm,cortex-a15-gic";
> +		reg_prop[2] = cpu_to_fdt64(ARM_GIC_CPUI_BASE);
> +		reg_prop[3] = cpu_to_fdt64(ARM_GIC_CPUI_SIZE);
> +		break;
> +	case IRQCHIP_GICV3:
> +		compatible = "arm,gic-v3";
> +		reg_prop[2] = cpu_to_fdt64(gic_redists_base);
> +		reg_prop[3] = cpu_to_fdt64(gic_redists_size);
>  		break;
>  	default:
>  		return;
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 2ed76fa..403d93b 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -22,7 +22,8 @@
>  #define GIC_MAX_IRQ			255
>  
>  enum irqchip_type {
> -	IRQCHIP_GICV2
> +	IRQCHIP_GICV2,
> +	IRQCHIP_GICV3

Same remark as for the previous patch.

>  };
>  
>  struct kvm;
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 90d6733..0f5fb7f 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -30,6 +30,13 @@
>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>  
> +/*
> + * On a GICv3 there must be one redistributor per vCPU.
> + * The value here is the size for one, we multiply this at runtime with
> + * the number of requested vCPUs to get the actual size.
> + */
> +#define ARM_GIC_REDIST_SIZE	0x20000
> +
>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>  
>  #define KVM_VM_TYPE		0
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 09/10] arm: add support for supplying GICv3 redistributor addresses

[Marc Zyngier]

On 17/06/15 12:22, Andre Przywara wrote:
> Instead of the GIC virtual CPU interface an emulated GICv3 needs to
> have accesses to its emulated redistributors trapped in the guest.
> Add code to tell the kernel about the mapping if a GICv3 emulation was
> requested by the user.
> 
> This contains some defines which are not (yet) in the (32 bit) header
> files to allow compilation for ARM.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/gic.c                         | 36 +++++++++++++++++++++++++++++++++++-
>  arm/include/arm-common/gic.h      |  3 ++-
>  arm/include/arm-common/kvm-arch.h |  7 +++++++
>  3 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index b6c5868..efe4b42 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -9,7 +9,18 @@
>  #include <linux/kernel.h>
>  #include <linux/kvm.h>
>  
> +/* Those names are not defined for ARM (yet) */
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_DIST
> +#define KVM_VGIC_V3_ADDR_TYPE_DIST 2
> +#endif
> +
> +#ifndef KVM_VGIC_V3_ADDR_TYPE_REDIST
> +#define KVM_VGIC_V3_ADDR_TYPE_REDIST 3
> +#endif
> +
>  static int gic_fd = -1;
> +static u64 gic_redists_base;
> +static u64 gic_redists_size;
>  
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
> @@ -28,12 +39,21 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
>  		.addr	= (u64)(unsigned long)&dist_addr,
>  	};
> +	struct kvm_device_attr redist_attr = {
> +		.group	= KVM_DEV_ARM_VGIC_GRP_ADDR,
> +		.attr	= KVM_VGIC_V3_ADDR_TYPE_REDIST,
> +		.addr	= (u64)(unsigned long)&gic_redists_base,
> +	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V2;
>  		dist_attr.attr  = KVM_VGIC_V2_ADDR_TYPE_DIST;
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
> +		dist_attr.attr  = KVM_VGIC_V3_ADDR_TYPE_DIST;
> +		break;
>  	}
>  
>  	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
> @@ -46,6 +66,9 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  	case IRQCHIP_GICV2:
>  		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &cpu_if_attr);
>  		break;
> +	case IRQCHIP_GICV3:
> +		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
> +		break;
>  	}
>  	if (err)
>  		goto out_err;
> @@ -97,6 +120,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		break;
> +	case IRQCHIP_GICV3:
> +		gic_redists_size = kvm->cfg.nrcpus * ARM_GIC_REDIST_SIZE;
> +		gic_redists_base = ARM_GIC_DIST_BASE - gic_redists_size;
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -156,12 +183,19 @@ void gic__generate_fdt_nodes(void *fdt, u32 phandle, enum irqchip_type type)
>  	const char *compatible;
>  	u64 reg_prop[] = {
>  		cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
> -		cpu_to_fdt64(ARM_GIC_CPUI_BASE), cpu_to_fdt64(ARM_GIC_CPUI_SIZE),
> +		0, 0,				/* to be filled */
>  	};
>  
>  	switch (type) {
>  	case IRQCHIP_GICV2:
>  		compatible = "arm,cortex-a15-gic";
> +		reg_prop[2] = cpu_to_fdt64(ARM_GIC_CPUI_BASE);
> +		reg_prop[3] = cpu_to_fdt64(ARM_GIC_CPUI_SIZE);
> +		break;
> +	case IRQCHIP_GICV3:
> +		compatible = "arm,gic-v3";
> +		reg_prop[2] = cpu_to_fdt64(gic_redists_base);
> +		reg_prop[3] = cpu_to_fdt64(gic_redists_size);
>  		break;
>  	default:
>  		return;
> diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
> index 2ed76fa..403d93b 100644
> --- a/arm/include/arm-common/gic.h
> +++ b/arm/include/arm-common/gic.h
> @@ -22,7 +22,8 @@
>  #define GIC_MAX_IRQ			255
>  
>  enum irqchip_type {
> -	IRQCHIP_GICV2
> +	IRQCHIP_GICV2,
> +	IRQCHIP_GICV3

Same remark as for the previous patch.

>  };
>  
>  struct kvm;
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 90d6733..0f5fb7f 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -30,6 +30,13 @@
>  #define KVM_PCI_MMIO_AREA	(KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
>  #define KVM_VIRTIO_MMIO_AREA	ARM_MMIO_AREA
>  
> +/*
> + * On a GICv3 there must be one redistributor per vCPU.
> + * The value here is the size for one, we multiply this at runtime with
> + * the number of requested vCPUs to get the actual size.
> + */
> +#define ARM_GIC_REDIST_SIZE	0x20000
> +
>  #define KVM_IRQ_OFFSET		GIC_SPI_IRQ_BASE
>  
>  #define KVM_VM_TYPE		0
> 

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 10/10] arm: use new irqchip parameter to create different vGIC types

[Andre Przywara]

Currently we unconditionally create a virtual GICv2 in the guest.
Add a --irqchip= parameter to let the user specify a different GIC
type for the guest.
For now we the only other supported type is GICv3.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/arm-cpu.c                    |  2 +-
 arm/gic.c                                | 17 +++++++++++++++++
 arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
 arm/kvm.c                                |  2 +-
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index f702b9e..3dc8ea3 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
+	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index efe4b42..5b49416 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -22,6 +22,23 @@ static int gic_fd = -1;
 static u64 gic_redists_base;
 static u64 gic_redists_size;
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset)
+{
+	enum irqchip_type *type = opt->value;
+
+	*type = IRQCHIP_GICV2;
+	if (!strcmp(arg, "gicv2")) {
+		*type = IRQCHIP_GICV2;
+	} else if (!strcmp(arg, "gicv3")) {
+		*type = IRQCHIP_GICV3;
+	} else if (strcmp(arg, "default")) {
+		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index a8ebd94..9529881 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -8,8 +8,11 @@ struct kvm_config_arch {
 	unsigned int	force_cntfrq;
 	bool		virtio_trans_pci;
 	bool		aarch32_guest;
+	enum irqchip_type irqchip;
 };
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset);
+
 #define OPT_ARCH_RUN(pfx, cfg)							\
 	pfx,									\
 	ARM_OPT_ARCH_RUN(cfg)							\
@@ -21,6 +24,10 @@ struct kvm_config_arch {
 		     "updated to program CNTFRQ correctly*"),			\
 	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
 		    "Force virtio devices to use PCI as their default "		\
-		    "transport"),
+		    "transport"),						\
+        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
+		     "[gicv2|gicv3]",					\
+		     "type of interrupt controller to emulate in the guest",	\
+		     irqchip_parser, NULL),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index f9685c2..d0e4a20 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm, IRQCHIP_GICV2))
+	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

[PATCH v3 10/10] arm: use new irqchip parameter to create different vGIC types

[Andre Przywara]

Currently we unconditionally create a virtual GICv2 in the guest.
Add a --irqchip= parameter to let the user specify a different GIC
type for the guest.
For now we the only other supported type is GICv3.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/aarch64/arm-cpu.c                    |  2 +-
 arm/gic.c                                | 17 +++++++++++++++++
 arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
 arm/kvm.c                                |  2 +-
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
index f702b9e..3dc8ea3 100644
--- a/arm/aarch64/arm-cpu.c
+++ b/arm/aarch64/arm-cpu.c
@@ -12,7 +12,7 @@
 static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
 {
 	int timer_interrupts[4] = {13, 14, 11, 10};
-	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
+	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
 	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
 }
 
diff --git a/arm/gic.c b/arm/gic.c
index efe4b42..5b49416 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -22,6 +22,23 @@ static int gic_fd = -1;
 static u64 gic_redists_base;
 static u64 gic_redists_size;
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset)
+{
+	enum irqchip_type *type = opt->value;
+
+	*type = IRQCHIP_GICV2;
+	if (!strcmp(arg, "gicv2")) {
+		*type = IRQCHIP_GICV2;
+	} else if (!strcmp(arg, "gicv3")) {
+		*type = IRQCHIP_GICV3;
+	} else if (strcmp(arg, "default")) {
+		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
+		return -1;
+	}
+
+	return 0;
+}
+
 static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 {
 	int err;
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index a8ebd94..9529881 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -8,8 +8,11 @@ struct kvm_config_arch {
 	unsigned int	force_cntfrq;
 	bool		virtio_trans_pci;
 	bool		aarch32_guest;
+	enum irqchip_type irqchip;
 };
 
+int irqchip_parser(const struct option *opt, const char *arg, int unset);
+
 #define OPT_ARCH_RUN(pfx, cfg)							\
 	pfx,									\
 	ARM_OPT_ARCH_RUN(cfg)							\
@@ -21,6 +24,10 @@ struct kvm_config_arch {
 		     "updated to program CNTFRQ correctly*"),			\
 	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
 		    "Force virtio devices to use PCI as their default "		\
-		    "transport"),
+		    "transport"),						\
+        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
+		     "[gicv2|gicv3]",					\
+		     "type of interrupt controller to emulate in the guest",	\
+		     irqchip_parser, NULL),
 
 #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index f9685c2..d0e4a20 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
 		MADV_MERGEABLE | MADV_HUGEPAGE);
 
 	/* Create the virtual GIC. */
-	if (gic__create(kvm, IRQCHIP_GICV2))
+	if (gic__create(kvm, kvm->cfg.arch.irqchip))
 		die("Failed to create virtual GIC");
 }
-- 
2.3.5

Re: [PATCH v3 10/10] arm: use new irqchip parameter to create different vGIC types

[Marc Zyngier]

On 17/06/15 12:22, Andre Przywara wrote:
> Currently we unconditionally create a virtual GICv2 in the guest.
> Add a --irqchip= parameter to let the user specify a different GIC
> type for the guest.
> For now we the only other supported type is GICv3.

Superfluous "we".

Also, spelling out the expected values would be a good thing
(--irqchip=GICv3 would fail).

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch64/arm-cpu.c                    |  2 +-
>  arm/gic.c                                | 17 +++++++++++++++++
>  arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
>  arm/kvm.c                                |  2 +-
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index f702b9e..3dc8ea3 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index efe4b42..5b49416 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -22,6 +22,23 @@ static int gic_fd = -1;
>  static u64 gic_redists_base;
>  static u64 gic_redists_size;
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset)
> +{
> +	enum irqchip_type *type = opt->value;
> +
> +	*type = IRQCHIP_GICV2;
> +	if (!strcmp(arg, "gicv2")) {
> +		*type = IRQCHIP_GICV2;
> +	} else if (!strcmp(arg, "gicv3")) {
> +		*type = IRQCHIP_GICV3;
> +	} else if (strcmp(arg, "default")) {
> +		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> index a8ebd94..9529881 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -8,8 +8,11 @@ struct kvm_config_arch {
>  	unsigned int	force_cntfrq;
>  	bool		virtio_trans_pci;
>  	bool		aarch32_guest;
> +	enum irqchip_type irqchip;
>  };
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset);
> +
>  #define OPT_ARCH_RUN(pfx, cfg)							\
>  	pfx,									\
>  	ARM_OPT_ARCH_RUN(cfg)							\
> @@ -21,6 +24,10 @@ struct kvm_config_arch {
>  		     "updated to program CNTFRQ correctly*"),			\
>  	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
>  		    "Force virtio devices to use PCI as their default "		\
> -		    "transport"),
> +		    "transport"),						\
> +        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
> +		     "[gicv2|gicv3]",					\

Looks like "default" is also an acceptable string, which you don't
document. I'd be inclined to remove the "default" handling altogether,
and document that without --irqchip, you get a GICv2. At some point, it
would be good to have a --irqchip=host, but we need some additional
kernel support for this.

> +		     "type of interrupt controller to emulate in the guest",	\
> +		     irqchip_parser, NULL),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index f9685c2..d0e4a20 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm, IRQCHIP_GICV2))
> +	if (gic__create(kvm, kvm->cfg.arch.irqchip))
>  		die("Failed to create virtual GIC");
>  }
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 10/10] arm: use new irqchip parameter to create different vGIC types

[Marc Zyngier]

On 17/06/15 12:22, Andre Przywara wrote:
> Currently we unconditionally create a virtual GICv2 in the guest.
> Add a --irqchip= parameter to let the user specify a different GIC
> type for the guest.
> For now we the only other supported type is GICv3.

Superfluous "we".

Also, spelling out the expected values would be a good thing
(--irqchip=GICv3 would fail).

> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/aarch64/arm-cpu.c                    |  2 +-
>  arm/gic.c                                | 17 +++++++++++++++++
>  arm/include/arm-common/kvm-config-arch.h |  9 ++++++++-
>  arm/kvm.c                                |  2 +-
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch64/arm-cpu.c b/arm/aarch64/arm-cpu.c
> index f702b9e..3dc8ea3 100644
> --- a/arm/aarch64/arm-cpu.c
> +++ b/arm/aarch64/arm-cpu.c
> @@ -12,7 +12,7 @@
>  static void generate_fdt_nodes(void *fdt, struct kvm *kvm, u32 gic_phandle)
>  {
>  	int timer_interrupts[4] = {13, 14, 11, 10};
> -	gic__generate_fdt_nodes(fdt, gic_phandle, IRQCHIP_GICV2);
> +	gic__generate_fdt_nodes(fdt, gic_phandle, kvm->cfg.arch.irqchip);
>  	timer__generate_fdt_nodes(fdt, kvm, timer_interrupts);
>  }
>  
> diff --git a/arm/gic.c b/arm/gic.c
> index efe4b42..5b49416 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -22,6 +22,23 @@ static int gic_fd = -1;
>  static u64 gic_redists_base;
>  static u64 gic_redists_size;
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset)
> +{
> +	enum irqchip_type *type = opt->value;
> +
> +	*type = IRQCHIP_GICV2;
> +	if (!strcmp(arg, "gicv2")) {
> +		*type = IRQCHIP_GICV2;
> +	} else if (!strcmp(arg, "gicv3")) {
> +		*type = IRQCHIP_GICV3;
> +	} else if (strcmp(arg, "default")) {
> +		fprintf(stderr, "irqchip: unknown type \"%s\"\n", arg);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
>  {
>  	int err;
> diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
> index a8ebd94..9529881 100644
> --- a/arm/include/arm-common/kvm-config-arch.h
> +++ b/arm/include/arm-common/kvm-config-arch.h
> @@ -8,8 +8,11 @@ struct kvm_config_arch {
>  	unsigned int	force_cntfrq;
>  	bool		virtio_trans_pci;
>  	bool		aarch32_guest;
> +	enum irqchip_type irqchip;
>  };
>  
> +int irqchip_parser(const struct option *opt, const char *arg, int unset);
> +
>  #define OPT_ARCH_RUN(pfx, cfg)							\
>  	pfx,									\
>  	ARM_OPT_ARCH_RUN(cfg)							\
> @@ -21,6 +24,10 @@ struct kvm_config_arch {
>  		     "updated to program CNTFRQ correctly*"),			\
>  	OPT_BOOLEAN('\0', "force-pci", &(cfg)->virtio_trans_pci,		\
>  		    "Force virtio devices to use PCI as their default "		\
> -		    "transport"),
> +		    "transport"),						\
> +        OPT_CALLBACK('\0', "irqchip", &(cfg)->irqchip,				\
> +		     "[gicv2|gicv3]",					\

Looks like "default" is also an acceptable string, which you don't
document. I'd be inclined to remove the "default" handling altogether,
and document that without --irqchip, you get a GICv2. At some point, it
would be good to have a --irqchip=host, but we need some additional
kernel support for this.

> +		     "type of interrupt controller to emulate in the guest",	\
> +		     irqchip_parser, NULL),
>  
>  #endif /* ARM_COMMON__KVM_CONFIG_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index f9685c2..d0e4a20 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -82,6 +82,6 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>  		MADV_MERGEABLE | MADV_HUGEPAGE);
>  
>  	/* Create the virtual GIC. */
> -	if (gic__create(kvm, IRQCHIP_GICV2))
> +	if (gic__create(kvm, kvm->cfg.arch.irqchip))
>  		die("Failed to create virtual GIC");
>  }
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...