[Qemu-devel] [RFC 0/8] KVM/ARM: Relax the max 123 vcpus limitation along with KVM GICv3

[Qemu-devel] [RFC 0/8] KVM/ARM: Relax the max 123 vcpus limitation along with KVM GICv3

[Eric Auger]

Currently the max number of VCPUs usable along with the KVM GICv3
device is limited to 123. The rationale is a single redistributor
region was supported and this latter was set to [0x80A0000, 0x9000000]
within the guest physical address space, surrounded with DIST and UART
MMIO regions.

[1] now allows to register several redistributor regions.
So this series overcomes the max 123 vcpu limitation by registering
a new redistributor region located just after the VIRT_MEM RAM region.
The total redistributor region capacity is set to 512 vcpus.

The max supported VCPUs in non accelerated mode is not modified.

Best Regards

Eric

Host Kernel dependencies:
[1] [RFC v2 00/12] KVM: arm/arm64: Allow multiple GICv3 redistributor regions
https://github.com/eauger/linux/tree/4.16-rc7-rdist-regions-rfc-v2

This QEMU series can be found at:
https://github.com/eauger/qemu/tree/v2.12.0-rc0-rdist_regions-rfc

Eric Auger (8):
  linux-headers: Partial update for KVM/ARM multiple redistributor
    region registration
  hw/intc/arm_gicv3: Use an array of redistributor regions
  kvm: Expose kvm_max_vcpus()
  hw/intc/arm_gicv3: Implement register_redist_region API
  hw/intc/arm_gicv3_kvm: Allow multiple redistributor regions
  hw/arm/virt: Allow GICv3 DT node with multiple redistributor regions
  hw/arm/virt-acpi-build: Handle multiple GICR structures
  hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

 accel/kvm/kvm-all.c                |  2 +-
 accel/stubs/kvm-stub.c             |  5 ++++
 hw/arm/virt-acpi-build.c           | 21 ++++++++++----
 hw/arm/virt.c                      | 55 +++++++++++++++++++++++++++++++-----
 hw/intc/arm_gicv3.c                | 21 ++++++++++++++
 hw/intc/arm_gicv3_common.c         |  9 ++++--
 hw/intc/arm_gicv3_kvm.c            | 57 ++++++++++++++++++++++++++++++++++++--
 include/hw/arm/virt.h              |  3 ++
 include/hw/intc/arm_gicv3_common.h | 14 +++++++++-
 include/sysemu/kvm.h               |  7 +++++
 linux-headers/asm-arm/kvm.h        |  7 +++--
 linux-headers/asm-arm64/kvm.h      |  7 +++--
 12 files changed, 182 insertions(+), 26 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC 1/8] linux-headers: Partial update for KVM/ARM multiple redistributor region registration

[Eric Auger]

This updates KVM/ARM headers against
https://github.com/eauger/linux/tree/4.16-rc7-rdist-regions-rfc-v2

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 linux-headers/asm-arm/kvm.h   | 7 ++++---
 linux-headers/asm-arm64/kvm.h | 7 ++++---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 4392955..7ef378b 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -88,9 +88,10 @@ struct kvm_regs {
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
 /* Supported VGICv3 address types  */
-#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
-#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
-#define KVM_VGIC_ITS_ADDR_TYPE		4
+#define KVM_VGIC_V3_ADDR_TYPE_DIST		2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST		3
+#define KVM_VGIC_ITS_ADDR_TYPE			4
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION	5
 
 #define KVM_VGIC_V3_DIST_SIZE		SZ_64K
 #define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 4e80651..25c520a 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -88,9 +88,10 @@ struct kvm_regs {
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
 /* Supported VGICv3 address types  */
-#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
-#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
-#define KVM_VGIC_ITS_ADDR_TYPE		4
+#define KVM_VGIC_V3_ADDR_TYPE_DIST		2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST		3
+#define KVM_VGIC_ITS_ADDR_TYPE			4
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION	5
 
 #define KVM_VGIC_V3_DIST_SIZE		SZ_64K
 #define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
-- 
2.5.5

[Qemu-devel] [RFC 2/8] hw/intc/arm_gicv3: Use an array of redistributor regions

[Eric Auger]

In the prospect to support multiple redistributor regions,
let's introduce a GICv3RDISTRegion struct datatype and a
statically sized array of those. For the time being, only
one redistributor region is used.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gicv3_common.c         |  5 +++--
 hw/intc/arm_gicv3_kvm.c            |  3 ++-
 include/hw/intc/arm_gicv3_common.h | 13 ++++++++++++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 7b54d52..cb4ee0e 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -199,11 +199,12 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
 
     memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
                           "gicv3_dist", 0x10000);
-    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s,
+    memory_region_init_io(&s->redist_region[0].mr, OBJECT(s),
+                          ops ? &ops[1] : NULL, s,
                           "gicv3_redist", 0x20000 * s->num_cpu);
 
     sysbus_init_mmio(sbd, &s->iomem_dist);
-    sysbus_init_mmio(sbd, &s->iomem_redist);
+    sysbus_init_mmio(sbd, &s->redist_region[0].mr);
 }
 
 static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index ec37177..a07bc55 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -755,7 +755,8 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 
     kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
-    kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
+    kvm_arm_register_device(&s->redist_region[0].mr, -1,
+                            KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
 
     if (kvm_has_gsi_routing()) {
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index bccdfe1..3cf132f 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -137,12 +137,20 @@ typedef struct GICv3CPUState GICv3CPUState;
 #define GICV3_S 0
 #define GICV3_NS 1
 
+#define GICV3_MAX_RDIST_REGIONS 8
+
 typedef struct {
     int irq;
     uint8_t prio;
     int grp;
 } PendingIrq;
 
+typedef struct GICv3RDISTRegion {
+    hwaddr base;
+    uint32_t count; /* number or redistributors */
+    MemoryRegion mr;
+} GICv3RDISTRegion ;
+
 struct GICv3CPUState {
     GICv3State *gic;
     CPUState *cpu;
@@ -210,7 +218,8 @@ struct GICv3State {
     /*< public >*/
 
     MemoryRegion iomem_dist; /* Distributor */
-    MemoryRegion iomem_redist; /* Redistributors */
+    GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
+    uint32_t nb_redist_regions;
 
     uint32_t num_cpu;
     uint32_t num_irq;
@@ -288,6 +297,8 @@ typedef struct ARMGICv3CommonClass {
 
     void (*pre_save)(GICv3State *s);
     void (*post_load)(GICv3State *s);
+    /* register an RDIST region at @base, containing @pfns 64kB pages */
+    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);
 } ARMGICv3CommonClass;
 
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
-- 
2.5.5

[Qemu-devel] [RFC 3/8] kvm: Expose kvm_max_vcpus()

[Eric Auger]

Make kvm_max_vcpus() public so that the machines can query this information
easily.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 accel/kvm/kvm-all.c    | 2 +-
 accel/stubs/kvm-stub.c | 5 +++++
 include/sysemu/kvm.h   | 7 +++++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index ffee68e..b04f4ef 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1462,7 +1462,7 @@ static int kvm_recommended_vcpus(KVMState *s)
     return (ret) ? ret : 4;
 }
 
-static int kvm_max_vcpus(KVMState *s)
+int kvm_max_vcpus(KVMState *s)
 {
     int ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
     return (ret) ? ret : kvm_recommended_vcpus(s);
diff --git a/accel/stubs/kvm-stub.c b/accel/stubs/kvm-stub.c
index 02d5170..e816050 100644
--- a/accel/stubs/kvm-stub.c
+++ b/accel/stubs/kvm-stub.c
@@ -170,4 +170,9 @@ bool kvm_arm_supports_user_irq(void)
 {
     return false;
 }
+
+int kvm_max_vcpus(KVMState *s)
+{
+    return -ENOSYS;
+}
 #endif
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 23669c4..1e7c621 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -546,6 +546,13 @@ int kvm_set_one_reg(CPUState *cs, uint64_t id, void *source);
  * Returns: 0 on success, or a negative errno on failure.
  */
 int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target);
+
+/**
+ * kvm_max_vcpus - return the number of supported vcpus
+ * @s: The KVMState pointer
+ */
+int kvm_max_vcpus(KVMState *s);
+
 struct ppc_radix_page_info *kvm_get_radix_page_info(void);
 int kvm_get_max_memslots(void);
 #endif
-- 
2.5.5

[Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API

[Eric Auger]

This patch defines and implements the register_redist_region() API
for both arm_gicv3 and arm_gicv3_kvm. Virt machine now directly calls
the function to set the single redistributor region. The associated
memory region init is removed from gicv3_init_irqs_and_mmio.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c                      |  6 +++++-
 hw/intc/arm_gicv3.c                | 21 +++++++++++++++++++++
 hw/intc/arm_gicv3_common.c         | 10 ++++++----
 hw/intc/arm_gicv3_kvm.c            | 38 +++++++++++++++++++++++++++++++++++---
 include/hw/intc/arm_gicv3_common.h |  5 +++--
 5 files changed, 70 insertions(+), 10 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 94dcb12..0eef6aa 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -526,7 +526,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     gicbusdev = SYS_BUS_DEVICE(gicdev);
     sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
     if (type == 3) {
-        sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
+        ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_GET_CLASS(gicdev);
+
+        agcc->register_redist_region((GICv3State *)gicdev,
+                                 vms->memmap[VIRT_GIC_REDIST].base,
+                                 vms->memmap[VIRT_GIC_REDIST].size >> 17);
     } else {
         sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
     }
diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 479c667..37f7564 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -378,6 +378,26 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
     gicv3_init_cpuif(s);
 }
 
+static int arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
+                                            uint32_t count)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+
+    if (s->nb_redist_regions > 0) {
+        return -EINVAL;
+    }
+
+    s->redist_region[s->nb_redist_regions].base = base;
+    s->redist_region[s->nb_redist_regions].count = count;
+    memory_region_init_io(&s->redist_region[s->nb_redist_regions].mr, OBJECT(s),
+                          gic_ops, s, "gicv3_redist", 0x20000 * count);
+    sysbus_init_mmio(sbd, &s->redist_region[s->nb_redist_regions].mr);
+    sysbus_mmio_map(sbd, 1, base);
+    s->nb_redist_regions++;
+
+    return 0;
+}
+
 static void arm_gicv3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -385,6 +405,7 @@ static void arm_gicv3_class_init(ObjectClass *klass, void *data)
     ARMGICv3Class *agc = ARM_GICV3_CLASS(klass);
 
     agcc->post_load = arm_gicv3_post_load;
+    agcc->register_redist_region = arm_gicv3_register_redist_region;
     device_class_set_parent_realize(dc, arm_gic_realize, &agc->parent_realize);
 }
 
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index cb4ee0e..c662e06 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -199,12 +199,8 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
 
     memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
                           "gicv3_dist", 0x10000);
-    memory_region_init_io(&s->redist_region[0].mr, OBJECT(s),
-                          ops ? &ops[1] : NULL, s,
-                          "gicv3_redist", 0x20000 * s->num_cpu);
 
     sysbus_init_mmio(sbd, &s->iomem_dist);
-    sysbus_init_mmio(sbd, &s->redist_region[0].mr);
 }
 
 static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
@@ -384,6 +380,12 @@ static void arm_gic_common_linux_init(ARMLinuxBootIf *obj,
     }
 }
 
+static inline
+bool arm_gicv3_common_support_multiple_redist_regions(GICv3State *s)
+{
+    return s->support_multiple_redist_regions;
+}
+
 static Property arm_gicv3_common_properties[] = {
     DEFINE_PROP_UINT32("num-cpu", GICv3State, num_cpu, 1),
     DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index a07bc55..b6a3faf 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -755,9 +755,6 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 
     kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
-    kvm_arm_register_device(&s->redist_region[0].mr, -1,
-                            KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
 
     if (kvm_has_gsi_routing()) {
         /* set up irq routing */
@@ -788,6 +785,40 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static int kvm_arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
+                                                uint32_t count)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+    int n = s->nb_redist_regions;
+    char *name;
+
+    if (!s->dev_fd) {
+        return -ENODEV;
+    }
+
+    if (n > 0) {
+        return -EINVAL;
+    }
+
+    name = g_strdup_printf("gicv3_redist_region[%d]", n);
+
+    s->redist_region[n].base = base;
+    s->redist_region[n].count = count;
+    memory_region_init_io(&s->redist_region[n].mr, OBJECT(s),
+                          NULL, s, name, count * 0x20000);
+    sysbus_init_mmio(sbd, &s->redist_region[n].mr);
+
+    kvm_arm_register_device(&s->redist_region[n].mr, -1,
+                            KVM_DEV_ARM_VGIC_GRP_ADDR,
+                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
+
+    sysbus_mmio_map(sbd, n + 1, base); /* first region is DIST */
+
+    s->nb_redist_regions++;
+    g_free(name);
+    return 0;
+}
+
 static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -796,6 +827,7 @@ static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
 
     agcc->pre_save = kvm_arm_gicv3_get;
     agcc->post_load = kvm_arm_gicv3_put;
+    agcc->register_redist_region = kvm_arm_gicv3_register_redist_region;
     device_class_set_parent_realize(dc, kvm_arm_gicv3_realize,
                                     &kgc->parent_realize);
     device_class_set_parent_reset(dc, kvm_arm_gicv3_reset, &kgc->parent_reset);
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 3cf132f..549ccc3 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -220,6 +220,7 @@ struct GICv3State {
     MemoryRegion iomem_dist; /* Distributor */
     GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
     uint32_t nb_redist_regions;
+    bool support_multiple_redist_regions;
 
     uint32_t num_cpu;
     uint32_t num_irq;
@@ -297,8 +298,8 @@ typedef struct ARMGICv3CommonClass {
 
     void (*pre_save)(GICv3State *s);
     void (*post_load)(GICv3State *s);
-    /* register an RDIST region at @base, containing @pfns 64kB pages */
-    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);
+    /* register an RDIST region at @base, containing @count redistributors */
+    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t count);
 } ARMGICv3CommonClass;
 
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
-- 
2.5.5

[Qemu-devel] [RFC 5/8] hw/intc/arm_gicv3_kvm: Allow multiple redistributor regions

[Eric Auger]

If the host kernel supports it, let's allow the regitration of more
than one redistributor region through the new GICv3 group/attribute:
KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION.

In that case we don't use kvm_arm_register_device anymore. this latter
registers the kvm device memory listener which resolves the absolute
gpa of the base address in kvm_arm_devlistener_add(). Then
kvm_arm_set_device_addr() is called on machine init done and invokes
the ioctl with the resolved absolute GPA.

In our case we know the absolute GPA at registration time and the
attribute value needs to be combined with the region index and
size of the region. So this does not nicely fit the current
infrastructure.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gicv3_kvm.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index b6a3faf..811d809 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -789,14 +789,21 @@ static int kvm_arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
                                                 uint32_t count)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+    bool multiple_redist_region_allowed;
     int n = s->nb_redist_regions;
+    Error **local_err = NULL;
     char *name;
+    int ret;
 
     if (!s->dev_fd) {
         return -ENODEV;
     }
 
-    if (n > 0) {
+    multiple_redist_region_allowed =
+        kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+                              KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
+
+    if (n > 0 && !multiple_redist_region_allowed) {
         return -EINVAL;
     }
 
@@ -808,15 +815,28 @@ static int kvm_arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
                           NULL, s, name, count * 0x20000);
     sysbus_init_mmio(sbd, &s->redist_region[n].mr);
 
-    kvm_arm_register_device(&s->redist_region[n].mr, -1,
-                            KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
-
+    if (!multiple_redist_region_allowed) {
+        /* use the legacy API */
+        kvm_arm_register_device(&s->redist_region[n].mr, -1,
+                                KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
+    } else {
+        uint64_t val = base | n;
+
+        val = deposit64(val, 52, 12, count);
+        ret = kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
+                                &val, true, local_err);
+        if (ret) {
+            goto out;
+        }
+    }
     sysbus_mmio_map(sbd, n + 1, base); /* first region is DIST */
 
     s->nb_redist_regions++;
+out:
     g_free(name);
-    return 0;
+    return ret;
 }
 
 static void kvm_arm_gicv3_class_init(ObjectClass *klass, void *data)
-- 
2.5.5

[Qemu-devel] [RFC 6/8] hw/arm/virt: Allow GICv3 DT node with multiple redistributor regions

[Eric Auger]

This patch adds the GICState handle in the virtual machine state and
allows to create a GIC device tree node advertising multiple redistributor
regions.

There is one range per distributor region following the GIC distributor.
Please refer to kernel documentation for further details:
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c         | 32 +++++++++++++++++++++++++++-----
 include/hw/arm/virt.h |  2 ++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 0eef6aa..8258f6f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -401,13 +401,34 @@ static void fdt_add_gic_node(VirtMachineState *vms)
     qemu_fdt_setprop_cell(vms->fdt, "/intc", "#size-cells", 0x2);
     qemu_fdt_setprop(vms->fdt, "/intc", "ranges", NULL, 0);
     if (vms->gic_version == 3) {
+        GICv3State *s = (GICv3State *)vms->gic;
+        uint64_t num_reg_values;
+        uint64_t *regs;
+        int r;
+
         qemu_fdt_setprop_string(vms->fdt, "/intc", "compatible",
                                 "arm,gic-v3");
-        qemu_fdt_setprop_sized_cells(vms->fdt, "/intc", "reg",
-                                     2, vms->memmap[VIRT_GIC_DIST].base,
-                                     2, vms->memmap[VIRT_GIC_DIST].size,
-                                     2, vms->memmap[VIRT_GIC_REDIST].base,
-                                     2, vms->memmap[VIRT_GIC_REDIST].size);
+
+        num_reg_values =  4 * (s->nb_redist_regions + 1);
+        regs = g_new0(uint64_t, num_reg_values);
+        qemu_fdt_setprop_cell(vms->fdt, "/intc", "#redistributor-regions",
+                              s->nb_redist_regions);
+        regs[0] = 2;
+        regs[1] = vms->memmap[VIRT_GIC_DIST].base;
+        regs[2] = 2;
+        regs[3] = vms->memmap[VIRT_GIC_DIST].size;
+
+        for (r = 1; r <= s->nb_redist_regions; r++) {
+            regs[4 * r] = 2;
+            regs[4 * r  + 1] = s->redist_region[r - 1].base;
+            regs[4 * r  + 2] = 2;
+            /* count redistributors of 2 x 64kB pages */
+            regs[4 * r  + 3] = (uint64_t)s->redist_region[r - 1].count << 17;
+        }
+        qemu_fdt_setprop_sized_cells_from_array(vms->fdt, "/intc", "reg",
+                                                num_reg_values / 2, regs);
+        g_free(regs);
+
         if (vms->virt) {
             qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GICV3_MAINT_IRQ,
@@ -513,6 +534,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
 
     gicdev = qdev_create(NULL, gictype);
+    vms->gic = (GICState *)gicdev;
     qdev_prop_set_uint32(gicdev, "revision", type);
     qdev_prop_set_uint32(gicdev, "num-cpu", smp_cpus);
     /* Note that the num-irq property counts both internal and external
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ba0c1a4..d168291 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -35,6 +35,7 @@
 #include "qemu/notify.h"
 #include "hw/boards.h"
 #include "hw/arm/arm.h"
+#include "hw/intc/arm_gic_common.h"
 
 #define NUM_GICV2M_SPIS       64
 #define NUM_VIRTIO_TRANSPORTS 32
@@ -92,6 +93,7 @@ typedef struct {
     MachineState parent;
     Notifier machine_done;
     FWCfgState *fw_cfg;
+    GICState *gic;
     bool secure;
     bool highmem;
     bool its;
-- 
2.5.5

[Qemu-devel] [RFC 7/8] hw/arm/virt-acpi-build: Handle multiple GICR structures

[Eric Auger]

In case multiple redistributor regions were registered,
let's add the corresponding GICR structures in the MADT
ACPI table.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index c7c6a57..aefc1b4 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -43,6 +43,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
+#include "hw/intc/arm_gicv3.h"
 #include "sysemu/numa.h"
 #include "kvm_arm.h"
 
@@ -618,14 +619,22 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     }
 
     if (vms->gic_version == 3) {
+        GICv3State *s = (GICv3State *)vms->gic;
+        int r;
+
         AcpiMadtGenericTranslator *gic_its;
-        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
-                                                         sizeof *gicr);
 
-        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
-        gicr->length = sizeof(*gicr);
-        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
-        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
+        for (r = 0; r <  s->nb_redist_regions; r++) {
+            AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
+                                                                sizeof *gicr);
+
+            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
+            gicr->length = sizeof(*gicr);
+            gicr->base_address = cpu_to_le64(s->redist_region[r].base);
+            /* count redistributors of 2 x 64kB pages */
+            gicr->range_length =
+                cpu_to_le64((uint64_t)s->redist_region[r].count << 17);
+        }
 
         if (its_class_name() && !vmc->no_its) {
             gic_its = acpi_data_push(table_data, sizeof *gic_its);
-- 
2.5.5

[Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Eric Auger]

With KVM acceleration and if KVM VGICV3 supports to set multiple
redistributor regions, we now allow up to 512 vcpus.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c         | 17 ++++++++++++++++-
 include/hw/arm/virt.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8258f6f..cdb1a75 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -149,8 +149,10 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
     [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
     [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
+    /* Allows 512 - 123 additional vcpus (each 2x64kB) */
+    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x30A0000LL },
     /* Second PCIe window, 512GB wide at the 512GB boundary */
-    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
+    [VIRT_PCIE_MMIO_HIGH] =     { 0x8000000000ULL, 0x8000000000ULL },
 };
 
 static const int a15irqmap[] = {
@@ -553,6 +555,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
         agcc->register_redist_region((GICv3State *)gicdev,
                                  vms->memmap[VIRT_GIC_REDIST].base,
                                  vms->memmap[VIRT_GIC_REDIST].size >> 17);
+        if (vms->smp_cpus > 123) {
+            agcc->register_redist_region((GICv3State *)gicdev,
+                     vms->memmap[VIRT_GIC_REDIST2].base,
+                                 vms->memmap[VIRT_GIC_REDIST2].size >> 17);
+        }
     } else {
         sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
     }
@@ -1284,6 +1291,14 @@ static void machvirt_init(MachineState *machine)
      */
     if (vms->gic_version == 3) {
         virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
+        if (kvm_max_vcpus(kvm_state) > 255) {
+            /*
+             * VGICv3 KVM device capability to set multiple redistributor
+             * was introduced at the same time KVM_MAX_VCPUS was bumped
+             * from 255 to 512
+             */
+            virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
+        }
     } else {
         virt_max_cpus = GIC_NCPU;
     }
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index d168291..801a4ad 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -60,6 +60,7 @@ enum {
     VIRT_GIC_V2M,
     VIRT_GIC_ITS,
     VIRT_GIC_REDIST,
+    VIRT_GIC_REDIST2,
     VIRT_UART,
     VIRT_MMIO,
     VIRT_RTC,
-- 
2.5.5

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Shannon Zhao]

On 2018/3/27 22:15, Eric Auger wrote:
> With KVM acceleration and if KVM VGICV3 supports to set multiple
> redistributor regions, we now allow up to 512 vcpus.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c         | 17 ++++++++++++++++-
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8258f6f..cdb1a75 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -149,8 +149,10 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
> +    /* Allows 512 - 123 additional vcpus (each 2x64kB) */
> +    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x30A0000LL },
One concern that this will limit the guest ram size to RAMLIMIT_BYTES.
If we want to support larger ram size in the future, this may be a problem.

>      /* Second PCIe window, 512GB wide at the 512GB boundary */
> -    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
> +    [VIRT_PCIE_MMIO_HIGH] =     { 0x8000000000ULL, 0x8000000000ULL },
>  };
>  
>  static const int a15irqmap[] = {
> @@ -553,6 +555,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>          agcc->register_redist_region((GICv3State *)gicdev,
>                                   vms->memmap[VIRT_GIC_REDIST].base,
>                                   vms->memmap[VIRT_GIC_REDIST].size >> 17);
> +        if (vms->smp_cpus > 123) {
> +            agcc->register_redist_region((GICv3State *)gicdev,
> +                     vms->memmap[VIRT_GIC_REDIST2].base,
> +                                 vms->memmap[VIRT_GIC_REDIST2].size >> 17);
> +        }
>      } else {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>      }
> @@ -1284,6 +1291,14 @@ static void machvirt_init(MachineState *machine)
>       */
>      if (vms->gic_version == 3) {
>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +        if (kvm_max_vcpus(kvm_state) > 255) {
> +            /*
> +             * VGICv3 KVM device capability to set multiple redistributor
> +             * was introduced at the same time KVM_MAX_VCPUS was bumped
> +             * from 255 to 512
> +             */
> +            virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
> +        }
>      } else {
>          virt_max_cpus = GIC_NCPU;
>      }
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index d168291..801a4ad 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -60,6 +60,7 @@ enum {
>      VIRT_GIC_V2M,
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
> +    VIRT_GIC_REDIST2,
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> 

-- 
Shannon

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Auger Eric]

Hi Shannon,

On 28/03/18 06:02, Shannon Zhao wrote:
> 
> 
> On 2018/3/27 22:15, Eric Auger wrote:
>> With KVM acceleration and if KVM VGICV3 supports to set multiple
>> redistributor regions, we now allow up to 512 vcpus.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c         | 17 ++++++++++++++++-
>>  include/hw/arm/virt.h |  1 +
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 8258f6f..cdb1a75 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -149,8 +149,10 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
>> +    /* Allows 512 - 123 additional vcpus (each 2x64kB) */
>> +    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x30A0000LL },
> One concern that this will limit the guest ram size to RAMLIMIT_BYTES.
> If we want to support larger ram size in the future, this may be a problem.
There is comment before  #define RAMLIMIT_GB 255
saying that RAM extension should rather happen at 2TB base.
"../.. We don't want to fill all the way up to 512GB with RAM because
 * we might want it for non-RAM purposes later"

This REDIST2 region could also be allocated at the top of the [256GB -
512GB] region.

I will start working on extending the RAM as suggested by this comment.

Thanks

Eric

> 
>>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>> -    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>> +    [VIRT_PCIE_MMIO_HIGH] =     { 0x8000000000ULL, 0x8000000000ULL },
>>  };
>>  
>>  static const int a15irqmap[] = {
>> @@ -553,6 +555,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>          agcc->register_redist_region((GICv3State *)gicdev,
>>                                   vms->memmap[VIRT_GIC_REDIST].base,
>>                                   vms->memmap[VIRT_GIC_REDIST].size >> 17);
>> +        if (vms->smp_cpus > 123) {
>> +            agcc->register_redist_region((GICv3State *)gicdev,
>> +                     vms->memmap[VIRT_GIC_REDIST2].base,
>> +                                 vms->memmap[VIRT_GIC_REDIST2].size >> 17);
>> +        }
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>>      }
>> @@ -1284,6 +1291,14 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      if (vms->gic_version == 3) {
>>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
>> +        if (kvm_max_vcpus(kvm_state) > 255) {
>> +            /*
>> +             * VGICv3 KVM device capability to set multiple redistributor
>> +             * was introduced at the same time KVM_MAX_VCPUS was bumped
>> +             * from 255 to 512
>> +             */
>> +            virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
>> +        }
>>      } else {
>>          virt_max_cpus = GIC_NCPU;
>>      }
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index d168291..801a4ad 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -60,6 +60,7 @@ enum {
>>      VIRT_GIC_V2M,
>>      VIRT_GIC_ITS,
>>      VIRT_GIC_REDIST,
>> +    VIRT_GIC_REDIST2,
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>>
> 

Re: [Qemu-devel] [RFC 2/8] hw/intc/arm_gicv3: Use an array of redistributor regions

[Peter Maydell]

On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
> In the prospect to support multiple redistributor regions,
> let's introduce a GICv3RDISTRegion struct datatype and a
> statically sized array of those. For the time being, only
> one redistributor region is used.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/intc/arm_gicv3_common.c         |  5 +++--
>  hw/intc/arm_gicv3_kvm.c            |  3 ++-
>  include/hw/intc/arm_gicv3_common.h | 13 ++++++++++++-
>  3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 7b54d52..cb4ee0e 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -199,11 +199,12 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
>
>      memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
>                            "gicv3_dist", 0x10000);
> -    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s,
> +    memory_region_init_io(&s->redist_region[0].mr, OBJECT(s),
> +                          ops ? &ops[1] : NULL, s,
>                            "gicv3_redist", 0x20000 * s->num_cpu);
>
>      sysbus_init_mmio(sbd, &s->iomem_dist);
> -    sysbus_init_mmio(sbd, &s->iomem_redist);
> +    sysbus_init_mmio(sbd, &s->redist_region[0].mr);
>  }
>
>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index ec37177..a07bc55 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -755,7 +755,8 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>
>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
> -    kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +    kvm_arm_register_device(&s->redist_region[0].mr, -1,
> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
>                              KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
>
>      if (kvm_has_gsi_routing()) {
> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index bccdfe1..3cf132f 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -137,12 +137,20 @@ typedef struct GICv3CPUState GICv3CPUState;
>  #define GICV3_S 0
>  #define GICV3_NS 1
>
> +#define GICV3_MAX_RDIST_REGIONS 8

Where does 8 come from? Is it a limit in the kernel, or just
a random number?

> +
>  typedef struct {
>      int irq;
>      uint8_t prio;
>      int grp;
>  } PendingIrq;
>
> +typedef struct GICv3RDISTRegion {

"GICv3RedistRegion"

> +    hwaddr base;
> +    uint32_t count; /* number or redistributors */

"of"

> +    MemoryRegion mr;
> +} GICv3RDISTRegion ;
> +
>  struct GICv3CPUState {
>      GICv3State *gic;
>      CPUState *cpu;
> @@ -210,7 +218,8 @@ struct GICv3State {
>      /*< public >*/
>
>      MemoryRegion iomem_dist; /* Distributor */
> -    MemoryRegion iomem_redist; /* Redistributors */
> +    GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
> +    uint32_t nb_redist_regions;
>
>      uint32_t num_cpu;
>      uint32_t num_irq;
> @@ -288,6 +297,8 @@ typedef struct ARMGICv3CommonClass {
>
>      void (*pre_save)(GICv3State *s);
>      void (*post_load)(GICv3State *s);
> +    /* register an RDIST region at @base, containing @pfns 64kB pages */
> +    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);

"pfn" is a kernelism (and I can never remember what it means) -- can you
use a more comprehensible variable name, please ?

>  } ARMGICv3CommonClass;
>
>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> --

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API

[Peter Maydell]

On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
> This patch defines and implements the register_redist_region() API
> for both arm_gicv3 and arm_gicv3_kvm. Virt machine now directly calls
> the function to set the single redistributor region. The associated
> memory region init is removed from gicv3_init_irqs_and_mmio.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c                      |  6 +++++-
>  hw/intc/arm_gicv3.c                | 21 +++++++++++++++++++++
>  hw/intc/arm_gicv3_common.c         | 10 ++++++----
>  hw/intc/arm_gicv3_kvm.c            | 38 +++++++++++++++++++++++++++++++++++---
>  include/hw/intc/arm_gicv3_common.h |  5 +++--
>  5 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 94dcb12..0eef6aa 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -526,7 +526,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
>      if (type == 3) {
> -        sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
> +        ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_GET_CLASS(gicdev);
> +
> +        agcc->register_redist_region((GICv3State *)gicdev,
> +                                 vms->memmap[VIRT_GIC_REDIST].base,
> +                                 vms->memmap[VIRT_GIC_REDIST].size >> 17);

What is this magic number 17 ?

>      } else {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>      }
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 479c667..37f7564 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -378,6 +378,26 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>      gicv3_init_cpuif(s);
>  }
>
> +static int arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
> +                                            uint32_t count)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
> +
> +    if (s->nb_redist_regions > 0) {
> +        return -EINVAL;
> +    }
> +
> +    s->redist_region[s->nb_redist_regions].base = base;
> +    s->redist_region[s->nb_redist_regions].count = count;
> +    memory_region_init_io(&s->redist_region[s->nb_redist_regions].mr, OBJECT(s),
> +                          gic_ops, s, "gicv3_redist", 0x20000 * count);
> +    sysbus_init_mmio(sbd, &s->redist_region[s->nb_redist_regions].mr);
> +    sysbus_mmio_map(sbd, 1, base);

Devices should never call sysbus_mmio_map() -- they should
just provide sysbus MMIO regions, and let the board code map
them in the right places. More generally the device code
shouldn't be told where in the memory map it is. kvm_arm_register_device()
goes to some lengths to maintain this abstraction (by setting
up a notifier to tell the kernel where things are only once
everything has been created and mapped).

Similar remarks apply to other changes in this patch (and
I suspect will need some restructuring to address).

> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
> index 3cf132f..549ccc3 100644
> --- a/include/hw/intc/arm_gicv3_common.h
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -220,6 +220,7 @@ struct GICv3State {
>      MemoryRegion iomem_dist; /* Distributor */
>      GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
>      uint32_t nb_redist_regions;
> +    bool support_multiple_redist_regions;
>
>      uint32_t num_cpu;
>      uint32_t num_irq;
> @@ -297,8 +298,8 @@ typedef struct ARMGICv3CommonClass {
>
>      void (*pre_save)(GICv3State *s);
>      void (*post_load)(GICv3State *s);
> -    /* register an RDIST region at @base, containing @pfns 64kB pages */
> -    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);
> +    /* register an RDIST region at @base, containing @count redistributors */
> +    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t count);

This looks like a change that should have been folded into a
previous patch.

>  } ARMGICv3CommonClass;
>
>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> --
> 2.5.5

thanks
-- PMM

Re: [Qemu-devel] [RFC 6/8] hw/arm/virt: Allow GICv3 DT node with multiple redistributor regions

[Peter Maydell]

On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
> This patch adds the GICState handle in the virtual machine state and
> allows to create a GIC device tree node advertising multiple redistributor
> regions.
>
> There is one range per distributor region following the GIC distributor.
> Please refer to kernel documentation for further details:
> Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

> +
> +        num_reg_values =  4 * (s->nb_redist_regions + 1);
> +        regs = g_new0(uint64_t, num_reg_values);
> +        qemu_fdt_setprop_cell(vms->fdt, "/intc", "#redistributor-regions",
> +                              s->nb_redist_regions);
> +        regs[0] = 2;
> +        regs[1] = vms->memmap[VIRT_GIC_DIST].base;
> +        regs[2] = 2;
> +        regs[3] = vms->memmap[VIRT_GIC_DIST].size;
> +
> +        for (r = 1; r <= s->nb_redist_regions; r++) {
> +            regs[4 * r] = 2;
> +            regs[4 * r  + 1] = s->redist_region[r - 1].base;
> +            regs[4 * r  + 2] = 2;
> +            /* count redistributors of 2 x 64kB pages */
> +            regs[4 * r  + 3] = (uint64_t)s->redist_region[r - 1].count << 17;

Board code shouldn't be reaching into the GIC device state struct like this.

> +        }
> +        qemu_fdt_setprop_sized_cells_from_array(vms->fdt, "/intc", "reg",
> +                                                num_reg_values / 2, regs);
> +        g_free(regs);
> +
>          if (vms->virt) {
>              qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",

thanks
-- PMM

Re: [Qemu-devel] [RFC 2/8] hw/intc/arm_gicv3: Use an array of redistributor regions

[Auger Eric]

Hi Peter,

On 13/04/18 15:27, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
>> In the prospect to support multiple redistributor regions,
>> let's introduce a GICv3RDISTRegion struct datatype and a
>> statically sized array of those. For the time being, only
>> one redistributor region is used.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/intc/arm_gicv3_common.c         |  5 +++--
>>  hw/intc/arm_gicv3_kvm.c            |  3 ++-
>>  include/hw/intc/arm_gicv3_common.h | 13 ++++++++++++-
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
>> index 7b54d52..cb4ee0e 100644
>> --- a/hw/intc/arm_gicv3_common.c
>> +++ b/hw/intc/arm_gicv3_common.c
>> @@ -199,11 +199,12 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
>>
>>      memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
>>                            "gicv3_dist", 0x10000);
>> -    memory_region_init_io(&s->iomem_redist, OBJECT(s), ops ? &ops[1] : NULL, s,
>> +    memory_region_init_io(&s->redist_region[0].mr, OBJECT(s),
>> +                          ops ? &ops[1] : NULL, s,
>>                            "gicv3_redist", 0x20000 * s->num_cpu);
>>
>>      sysbus_init_mmio(sbd, &s->iomem_dist);
>> -    sysbus_init_mmio(sbd, &s->iomem_redist);
>> +    sysbus_init_mmio(sbd, &s->redist_region[0].mr);
>>  }
>>
>>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index ec37177..a07bc55 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -755,7 +755,8 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>>
>>      kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>>                              KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
>> -    kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
>> +    kvm_arm_register_device(&s->redist_region[0].mr, -1,
>> +                            KVM_DEV_ARM_VGIC_GRP_ADDR,
>>                              KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
>>
>>      if (kvm_has_gsi_routing()) {
>> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
>> index bccdfe1..3cf132f 100644
>> --- a/include/hw/intc/arm_gicv3_common.h
>> +++ b/include/hw/intc/arm_gicv3_common.h
>> @@ -137,12 +137,20 @@ typedef struct GICv3CPUState GICv3CPUState;
>>  #define GICV3_S 0
>>  #define GICV3_NS 1
>>
>> +#define GICV3_MAX_RDIST_REGIONS 8
> 
> Where does 8 come from? Is it a limit in the kernel, or just
> a random number?
Kernel has no limit as the redist regions are handled through a list
there. 8 is purely arbitrate. I thought it was overkill to handle a list
here.
> 
>> +
>>  typedef struct {
>>      int irq;
>>      uint8_t prio;
>>      int grp;
>>  } PendingIrq;
>>
>> +typedef struct GICv3RDISTRegion {
> 
> "GICv3RedistRegion"
> 
>> +    hwaddr base;
>> +    uint32_t count; /* number or redistributors */
> 
> "of"
> 
>> +    MemoryRegion mr;
>> +} GICv3RDISTRegion ;
>> +
>>  struct GICv3CPUState {
>>      GICv3State *gic;
>>      CPUState *cpu;
>> @@ -210,7 +218,8 @@ struct GICv3State {
>>      /*< public >*/
>>
>>      MemoryRegion iomem_dist; /* Distributor */
>> -    MemoryRegion iomem_redist; /* Redistributors */
>> +    GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
>> +    uint32_t nb_redist_regions;
>>
>>      uint32_t num_cpu;
>>      uint32_t num_irq;
>> @@ -288,6 +297,8 @@ typedef struct ARMGICv3CommonClass {
>>
>>      void (*pre_save)(GICv3State *s);
>>      void (*post_load)(GICv3State *s);
>> +    /* register an RDIST region at @base, containing @pfns 64kB pages */
>> +    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);
> 
> "pfn" is a kernelism (and I can never remember what it means) -- can you
> use a more comprehensible variable name, please ?
In pratice it has become count (number of redistributors within the
region) but I forgot to update the declaration here, sorry.

Thanks

Eric
> 
>>  } ARMGICv3CommonClass;
>>
>>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
>> --
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Peter Maydell]

On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
> With KVM acceleration and if KVM VGICV3 supports to set multiple
> redistributor regions, we now allow up to 512 vcpus.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c         | 17 ++++++++++++++++-
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8258f6f..cdb1a75 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -149,8 +149,10 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
> +    /* Allows 512 - 123 additional vcpus (each 2x64kB) */
> +    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x30A0000LL },

Maybe we should make the 2nd redist region go up to a slightly
rounder address rather than making it exactly big enough to get
us up to 512 CPUs? (Why 512, by the way?)

>      /* Second PCIe window, 512GB wide at the 512GB boundary */
> -    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
> +    [VIRT_PCIE_MMIO_HIGH] =     { 0x8000000000ULL, 0x8000000000ULL },
>  };
>
>  static const int a15irqmap[] = {
> @@ -553,6 +555,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>          agcc->register_redist_region((GICv3State *)gicdev,
>                                   vms->memmap[VIRT_GIC_REDIST].base,
>                                   vms->memmap[VIRT_GIC_REDIST].size >> 17);
> +        if (vms->smp_cpus > 123) {
> +            agcc->register_redist_region((GICv3State *)gicdev,
> +                     vms->memmap[VIRT_GIC_REDIST2].base,
> +                                 vms->memmap[VIRT_GIC_REDIST2].size >> 17);
> +        }
>      } else {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>      }
> @@ -1284,6 +1291,14 @@ static void machvirt_init(MachineState *machine)
>       */
>      if (vms->gic_version == 3) {
>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +        if (kvm_max_vcpus(kvm_state) > 255) {
> +            /*
> +             * VGICv3 KVM device capability to set multiple redistributor
> +             * was introduced at the same time KVM_MAX_VCPUS was bumped
> +             * from 255 to 512
> +             */
> +            virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;

I think it would be better to explicitly check "do we have
support for split redistributors" rather than looking at
KVM_MAX_VCPUS. It's not impossible that a distro kernel
might have chosen to backport support for one but not the
other.

> +        }
>      } else {
>          virt_max_cpus = GIC_NCPU;
>      }
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index d168291..801a4ad 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -60,6 +60,7 @@ enum {
>      VIRT_GIC_V2M,
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
> +    VIRT_GIC_REDIST2,
>      VIRT_UART,
>      VIRT_MMIO,
>      VIRT_RTC,
> --
> 2.5.5

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API

[Auger Eric]

Hi Peter,
On 13/04/18 15:34, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
>> This patch defines and implements the register_redist_region() API
>> for both arm_gicv3 and arm_gicv3_kvm. Virt machine now directly calls
>> the function to set the single redistributor region. The associated
>> memory region init is removed from gicv3_init_irqs_and_mmio.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c                      |  6 +++++-
>>  hw/intc/arm_gicv3.c                | 21 +++++++++++++++++++++
>>  hw/intc/arm_gicv3_common.c         | 10 ++++++----
>>  hw/intc/arm_gicv3_kvm.c            | 38 +++++++++++++++++++++++++++++++++++---
>>  include/hw/intc/arm_gicv3_common.h |  5 +++--
>>  5 files changed, 70 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 94dcb12..0eef6aa 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -526,7 +526,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
>>      if (type == 3) {
>> -        sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_REDIST].base);
>> +        ARMGICv3CommonClass *agcc = ARM_GICV3_COMMON_GET_CLASS(gicdev);
>> +
>> +        agcc->register_redist_region((GICv3State *)gicdev,
>> +                                 vms->memmap[VIRT_GIC_REDIST].base,
>> +                                 vms->memmap[VIRT_GIC_REDIST].size >> 17);
> 
> What is this magic number 
I meant size / (64kB *2) to match the # of redistributors within the region
> 
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>>      }
>> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
>> index 479c667..37f7564 100644
>> --- a/hw/intc/arm_gicv3.c
>> +++ b/hw/intc/arm_gicv3.c
>> @@ -378,6 +378,26 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>>      gicv3_init_cpuif(s);
>>  }
>>
>> +static int arm_gicv3_register_redist_region(GICv3State *s, hwaddr base,
>> +                                            uint32_t count)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>> +
>> +    if (s->nb_redist_regions > 0) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    s->redist_region[s->nb_redist_regions].base = base;
>> +    s->redist_region[s->nb_redist_regions].count = count;
>> +    memory_region_init_io(&s->redist_region[s->nb_redist_regions].mr, OBJECT(s),
>> +                          gic_ops, s, "gicv3_redist", 0x20000 * count);
>> +    sysbus_init_mmio(sbd, &s->redist_region[s->nb_redist_regions].mr);
>> +    sysbus_mmio_map(sbd, 1, base);
> 
> Devices should never call sysbus_mmio_map() -- they should
> just provide sysbus MMIO regions, and let the board code map
> them in the right places. More generally the device code
> shouldn't be told where in the memory map it is. kvm_arm_register_device()
> goes to some lengths to maintain this abstraction (by setting
> up a notifier to tell the kernel where things are only once
> everything has been created and mapped).
> 
> Similar remarks apply to other changes in this patch (and
> I suspect will need some restructuring to address).

Actually this is an API provided to the machine and not the device
itself directly playing with the mapping.

If not acceptable, I need to match the existing notifier framework and
do something similar taking into account the new GROUP/ATTR address
semantics here.
> 
>> diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
>> index 3cf132f..549ccc3 100644
>> --- a/include/hw/intc/arm_gicv3_common.h
>> +++ b/include/hw/intc/arm_gicv3_common.h
>> @@ -220,6 +220,7 @@ struct GICv3State {
>>      MemoryRegion iomem_dist; /* Distributor */
>>      GICv3RDISTRegion redist_region[GICV3_MAX_RDIST_REGIONS];
>>      uint32_t nb_redist_regions;
>> +    bool support_multiple_redist_regions;
>>
>>      uint32_t num_cpu;
>>      uint32_t num_irq;
>> @@ -297,8 +298,8 @@ typedef struct ARMGICv3CommonClass {
>>
>>      void (*pre_save)(GICv3State *s);
>>      void (*post_load)(GICv3State *s);
>> -    /* register an RDIST region at @base, containing @pfns 64kB pages */
>> -    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t pfns);
>> +    /* register an RDIST region at @base, containing @count redistributors */
>> +    int (*register_redist_region)(GICv3State *s, hwaddr base, uint32_t count);
> 
> This looks like a change that should have been folded into a
> previous patch.
definitively

Thanks

Eric
> 
>>  } ARMGICv3CommonClass;
>>
>>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
>> --
>> 2.5.5
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 6/8] hw/arm/virt: Allow GICv3 DT node with multiple redistributor regions

[Auger Eric]

Hi Peter,

On 13/04/18 15:36, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
>> This patch adds the GICState handle in the virtual machine state and
>> allows to create a GIC device tree node advertising multiple redistributor
>> regions.
>>
>> There is one range per distributor region following the GIC distributor.
>> Please refer to kernel documentation for further details:
>> Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
>> +
>> +        num_reg_values =  4 * (s->nb_redist_regions + 1);
>> +        regs = g_new0(uint64_t, num_reg_values);
>> +        qemu_fdt_setprop_cell(vms->fdt, "/intc", "#redistributor-regions",
>> +                              s->nb_redist_regions);
>> +        regs[0] = 2;
>> +        regs[1] = vms->memmap[VIRT_GIC_DIST].base;
>> +        regs[2] = 2;
>> +        regs[3] = vms->memmap[VIRT_GIC_DIST].size;
>> +
>> +        for (r = 1; r <= s->nb_redist_regions; r++) {
>> +            regs[4 * r] = 2;
>> +            regs[4 * r  + 1] = s->redist_region[r - 1].base;
>> +            regs[4 * r  + 2] = 2;
>> +            /* count redistributors of 2 x 64kB pages */
>> +            regs[4 * r  + 3] = (uint64_t)s->redist_region[r - 1].count << 17;
> 
> Board code shouldn't be reaching into the GIC device state struct like this.

I will add a dedicated API.

Thanks

Eric
> 
>> +        }
>> +        qemu_fdt_setprop_sized_cells_from_array(vms->fdt, "/intc", "reg",
>> +                                                num_reg_values / 2, regs);
>> +        g_free(regs);
>> +
>>          if (vms->virt) {
>>              qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API

[Peter Maydell]

On 13 April 2018 at 14:44, Auger Eric <eric.auger@redhat.com> wrote:
> Actually this is an API provided to the machine and not the device
> itself directly playing with the mapping.
>
> If not acceptable, I need to match the existing notifier framework and
> do something similar taking into account the new GROUP/ATTR address
> semantics here.

I think I'd rather that we continue to tell the kernel about
the addresses of device related regions in the same general
way that we do at the moment.

thanks
-- PMM

Re: [Qemu-devel] [RFC 7/8] hw/arm/virt-acpi-build: Handle multiple GICR structures

[Andrew Jones]

On Tue, Mar 27, 2018 at 04:15:21PM +0200, Eric Auger wrote:
> In case multiple redistributor regions were registered,
> let's add the corresponding GICR structures in the MADT
> ACPI table.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c7c6a57..aefc1b4 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -43,6 +43,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> +#include "hw/intc/arm_gicv3.h"
>  #include "sysemu/numa.h"
>  #include "kvm_arm.h"
>  
> @@ -618,14 +619,22 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      }
>  
>      if (vms->gic_version == 3) {
> +        GICv3State *s = (GICv3State *)vms->gic;
> +        int r;
> +
>          AcpiMadtGenericTranslator *gic_its;
> -        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
> -                                                         sizeof *gicr);
>  
> -        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> -        gicr->length = sizeof(*gicr);
> -        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
> -        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
> +        for (r = 0; r <  s->nb_redist_regions; r++) {
                           ^ extra space
> +            AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
> +                                                                sizeof *gicr);
> +
> +            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> +            gicr->length = sizeof(*gicr);

This file has inconsistent use of () with sizeof. If you want to start
it on its path to using ()'s, like the majority of QEMU code, then you
could add them to the acpi_data_push() above.

> +            gicr->base_address = cpu_to_le64(s->redist_region[r].base);
> +            /* count redistributors of 2 x 64kB pages */
> +            gicr->range_length =
> +                cpu_to_le64((uint64_t)s->redist_region[r].count << 17);
                            ^^ range_length is only 4 bytes.

It might be nice to have a define of some sort for the 2*64K to avoid it
getting scattered everywhere. 

> +        }
>  
>          if (its_class_name() && !vmc->no_its) {
>              gic_its = acpi_data_push(table_data, sizeof *gic_its);
> -- 
> 2.5.5
> 
>

Thanks,
drew 

Re: [Qemu-devel] [RFC 7/8] hw/arm/virt-acpi-build: Handle multiple GICR structures

[Auger Eric]

Hi Drew,

On 13/04/18 15:47, Andrew Jones wrote:
> On Tue, Mar 27, 2018 at 04:15:21PM +0200, Eric Auger wrote:
>> In case multiple redistributor regions were registered,
>> let's add the corresponding GICR structures in the MADT
>> ACPI table.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index c7c6a57..aefc1b4 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -43,6 +43,7 @@
>>  #include "hw/pci/pcie_host.h"
>>  #include "hw/pci/pci.h"
>>  #include "hw/arm/virt.h"
>> +#include "hw/intc/arm_gicv3.h"
>>  #include "sysemu/numa.h"
>>  #include "kvm_arm.h"
>>  
>> @@ -618,14 +619,22 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>      }
>>  
>>      if (vms->gic_version == 3) {
>> +        GICv3State *s = (GICv3State *)vms->gic;
>> +        int r;
>> +
>>          AcpiMadtGenericTranslator *gic_its;
>> -        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
>> -                                                         sizeof *gicr);
>>  
>> -        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
>> -        gicr->length = sizeof(*gicr);
>> -        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
>> -        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
>> +        for (r = 0; r <  s->nb_redist_regions; r++) {
>                            ^ extra space
>> +            AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
>> +                                                                sizeof *gicr);
>> +
>> +            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
>> +            gicr->length = sizeof(*gicr);
> 
> This file has inconsistent use of () with sizeof. If you want to start
> it on its path to using ()'s, like the majority of QEMU code, then you
> could add them to the acpi_data_push() above.
I will align with the rest of the code
> 
>> +            gicr->base_address = cpu_to_le64(s->redist_region[r].base);
>> +            /* count redistributors of 2 x 64kB pages */
>> +            gicr->range_length =
>> +                cpu_to_le64((uint64_t)s->redist_region[r].count << 17);
>                             ^^ range_length is only 4 bytes.
> 
> It might be nice to have a define of some sort for the 2*64K to avoid it
> getting scattered everywhere. 
sure

Thanks

Eric
> 
>> +        }
>>  
>>          if (its_class_name() && !vmc->no_its) {
>>              gic_its = acpi_data_push(table_data, sizeof *gic_its);
>> -- 
>> 2.5.5
>>
>>
> 
> Thanks,
> drew 
> 

Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API

[Auger Eric]

Hi Peter,

On 13/04/18 15:46, Peter Maydell wrote:
> On 13 April 2018 at 14:44, Auger Eric <eric.auger@redhat.com> wrote:
>> Actually this is an API provided to the machine and not the device
>> itself directly playing with the mapping.
>>
>> If not acceptable, I need to match the existing notifier framework and
>> do something similar taking into account the new GROUP/ATTR address
>> semantics here.
> 
> I think I'd rather that we continue to tell the kernel about
> the addresses of device related regions in the same general
> way that we do at the moment.

OK I will rework in that direction then.

Thank you for the review!

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Auger Eric]

Hi Peter,

On 13/04/18 15:41, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger <eric.auger@redhat.com> wrote:
>> With KVM acceleration and if KVM VGICV3 supports to set multiple
>> redistributor regions, we now allow up to 512 vcpus.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c         | 17 ++++++++++++++++-
>>  include/hw/arm/virt.h |  1 +
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 8258f6f..cdb1a75 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -149,8 +149,10 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
>> +    /* Allows 512 - 123 additional vcpus (each 2x64kB) */
>> +    [VIRT_GIC_REDIST2] =        { 0x4000000000ULL, 0x30A0000LL },
> 
> Maybe we should make the 2nd redist region go up to a slightly
> rounder address rather than making it exactly big enough to get
> us up to 512 CPUs? (Why 512, by the way?)
> 
>>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>> -    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>> +    [VIRT_PCIE_MMIO_HIGH] =     { 0x8000000000ULL, 0x8000000000ULL },
>>  };
>>
>>  static const int a15irqmap[] = {
>> @@ -553,6 +555,11 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>          agcc->register_redist_region((GICv3State *)gicdev,
>>                                   vms->memmap[VIRT_GIC_REDIST].base,
>>                                   vms->memmap[VIRT_GIC_REDIST].size >> 17);
>> +        if (vms->smp_cpus > 123) {
>> +            agcc->register_redist_region((GICv3State *)gicdev,
>> +                     vms->memmap[VIRT_GIC_REDIST2].base,
>> +                                 vms->memmap[VIRT_GIC_REDIST2].size >> 17);
>> +        }
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>>      }
>> @@ -1284,6 +1291,14 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      if (vms->gic_version == 3) {
>>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
>> +        if (kvm_max_vcpus(kvm_state) > 255) {
>> +            /*
>> +             * VGICv3 KVM device capability to set multiple redistributor
>> +             * was introduced at the same time KVM_MAX_VCPUS was bumped
>> +             * from 255 to 512
>> +             */
>> +            virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
> 
> I think it would be better to explicitly check "do we have
> support for split redistributors" rather than looking at
> KVM_MAX_VCPUS. It's not impossible that a distro kernel
> might have chosen to backport support for one but not the
> other.

The issue is we have race here:
to check whether the new ATTR is supported I need to query the GICV3 KVM
device. This later is created in the GICv3 initialize. But to initialize
the VGIC I need the VCPUs to be created. And to create the VCPUs I need
to check their max number. Or do I miss somethinh?

The KVM device create dry-run mode cannot be used here. I would need to
really create the KVM device and then delete it. The issue is there is
no kvm device DELETE ioctl at kernel level since KVM devices are deleted
on VM deletion if I understand correctly. Adding a KVM device delete at
kernel level may not be straightforward as I suspect some
simplifications could be made in KVM device deletion knowing the VM was
under deletion.

Thanks

Eric
> 
>> +        }
>>      } else {
>>          virt_max_cpus = GIC_NCPU;
>>      }
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index d168291..801a4ad 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -60,6 +60,7 @@ enum {
>>      VIRT_GIC_V2M,
>>      VIRT_GIC_ITS,
>>      VIRT_GIC_REDIST,
>> +    VIRT_GIC_REDIST2,
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> --
>> 2.5.5
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Peter Maydell]

On 13 April 2018 at 15:01, Auger Eric <eric.auger@redhat.com> wrote:
> On 13/04/18 15:41, Peter Maydell wrote:
>> I think it would be better to explicitly check "do we have
>> support for split redistributors" rather than looking at
>> KVM_MAX_VCPUS. It's not impossible that a distro kernel
>> might have chosen to backport support for one but not the
>> other.
>
> The issue is we have race here:
> to check whether the new ATTR is supported I need to query the GICV3 KVM
> device. This later is created in the GICv3 initialize. But to initialize
> the VGIC I need the VCPUs to be created. And to create the VCPUs I need
> to check their max number. Or do I miss somethinh?
>
> The KVM device create dry-run mode cannot be used here. I would need to
> really create the KVM device and then delete it. The issue is there is
> no kvm device DELETE ioctl at kernel level since KVM devices are deleted
> on VM deletion if I understand correctly. Adding a KVM device delete at
> kernel level may not be straightforward as I suspect some
> simplifications could be made in KVM device deletion knowing the VM was
> under deletion.

Two possibilities:

(1) do like we do in kvm_arm_get_host_cpu_features() where we
create a scratch VM to interrogate it and then throw it away
when we have the info we need

(2) add an API to the kernel to make it easier to query it
for this sort of feature

thanks
-- PMM

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Auger Eric]

Hi Peter,

On 13/04/18 16:06, Peter Maydell wrote:
> On 13 April 2018 at 15:01, Auger Eric <eric.auger@redhat.com> wrote:
>> On 13/04/18 15:41, Peter Maydell wrote:
>>> I think it would be better to explicitly check "do we have
>>> support for split redistributors" rather than looking at
>>> KVM_MAX_VCPUS. It's not impossible that a distro kernel
>>> might have chosen to backport support for one but not the
>>> other.
>>
>> The issue is we have race here:
>> to check whether the new ATTR is supported I need to query the GICV3 KVM
>> device. This later is created in the GICv3 initialize. But to initialize
>> the VGIC I need the VCPUs to be created. And to create the VCPUs I need
>> to check their max number. Or do I miss somethinh?
>>
>> The KVM device create dry-run mode cannot be used here. I would need to
>> really create the KVM device and then delete it. The issue is there is
>> no kvm device DELETE ioctl at kernel level since KVM devices are deleted
>> on VM deletion if I understand correctly. Adding a KVM device delete at
>> kernel level may not be straightforward as I suspect some
>> simplifications could be made in KVM device deletion knowing the VM was
>> under deletion.
> 
> Two possibilities:
> 
> (1) do like we do in kvm_arm_get_host_cpu_features() where we
> create a scratch VM to interrogate it and then throw it away
> when we have the info we need
Oh OK. I did not know this was in place. At first sight, this looks
simpler than (2) to me. I will investigate this solution.

Thanks!

Eric
> 
> (2) add an API to the kernel to make it easier to query it
> for this sort of feature
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Andrew Jones]

On Fri, Apr 13, 2018 at 04:11:24PM +0200, Auger Eric wrote:
> Hi Peter,
> 
> On 13/04/18 16:06, Peter Maydell wrote:
> > On 13 April 2018 at 15:01, Auger Eric <eric.auger@redhat.com> wrote:
> >> On 13/04/18 15:41, Peter Maydell wrote:
> >>> I think it would be better to explicitly check "do we have
> >>> support for split redistributors" rather than looking at
> >>> KVM_MAX_VCPUS. It's not impossible that a distro kernel
> >>> might have chosen to backport support for one but not the
> >>> other.
> >>
> >> The issue is we have race here:
> >> to check whether the new ATTR is supported I need to query the GICV3 KVM
> >> device. This later is created in the GICv3 initialize. But to initialize
> >> the VGIC I need the VCPUs to be created. And to create the VCPUs I need
> >> to check their max number. Or do I miss somethinh?
> >>
> >> The KVM device create dry-run mode cannot be used here. I would need to
> >> really create the KVM device and then delete it. The issue is there is
> >> no kvm device DELETE ioctl at kernel level since KVM devices are deleted
> >> on VM deletion if I understand correctly. Adding a KVM device delete at
> >> kernel level may not be straightforward as I suspect some
> >> simplifications could be made in KVM device deletion knowing the VM was
> >> under deletion.
> > 
> > Two possibilities:
> > 
> > (1) do like we do in kvm_arm_get_host_cpu_features() where we
> > create a scratch VM to interrogate it and then throw it away
> > when we have the info we need
> Oh OK. I did not know this was in place. At first sight, this looks
> simpler than (2) to me. I will investigate this solution.

But (1) isn't awesome. It'd be better if we could avoid scratch VMs
altogether as they slow VM launch and create confusing traces when
debugging. Also, it's just not a pretty way to do things. If we must
keep a scratch VM, then let's at least make sure we only have one,
i.e. put all probing into a single scratch VM.

> 
> Thanks!
> 
> Eric
> > 
> > (2) add an API to the kernel to make it easier to query it
> > for this sort of feature

This is the cleaner choice, but I wasn't opposed to the max-vcpus
query getting double-duty though. While the backporting argument is
a good point, I'm not sure we've ever really cared about what
issues backporters have had to endure when they cherry-pick features
before.

Thanks,
drew


> > 
> > thanks
> > -- PMM
> > 
> 

Re: [Qemu-devel] [RFC 8/8] hw/arm/virt: Allow up to 512 vcpus along with KVM VGICv3

[Peter Maydell]

On 16 April 2018 at 10:19, Andrew Jones <drjones@redhat.com> wrote:
> This is the cleaner choice, but I wasn't opposed to the max-vcpus
> query getting double-duty though. While the backporting argument is
> a good point, I'm not sure we've ever really cared about what
> issues backporters have had to endure when they cherry-pick features
> before.

Well, mostly I didn't like the entangling of two different
features, so we look for the presence of one by checking
for the other. That's confusing going forward as well as
awkward for backporting. Plus if we have a difficulty with
detection of new kernel features like this we're probably going
to run into it again in the future, so we might as well sort
it out now...

thanks
-- PMM