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

[Qemu-devel] [RFC v2 0/7] 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] [PATCH v6 00/12] KVM: arm/arm64: Allow multiple GICv3 redistributor
    regions
https://github.com/eauger/linux/tree/v4.17-rc2-rdist-regions-v6

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

History:
v1 -> v2:
- Do not use KVM_MAX_VCPUS anymore
- In case the multiple redistributor region capability is not
  supported by the host kernel, the GICv3 device realize() fails
  with a hint for the end-user.
- use properties to set the redistributor region count
- sysbus_mmio_map is kept in virt and machine init done notifier
  mechanism is used with an address fixup addition.
- I have not yet extended the second redist region as Peter suggested.
  We can easily add another 3d region later on if requested. But
  if mandated, I will fix that in next release.


Eric Auger (7):
  linux-headers: Partial update for KVM/ARM multiple redistributor
    region registration
  target/arm: Allow KVM device address overwriting
  hw/intc/arm_gicv3: Introduce redist-region-count array property
  hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions
  hw/arm/virt: GICv3 DT node with one or two redistributor regions
  hw/arm/virt-acpi-build: Advertise one or two GICR structures
  hw/arm/virt: Register two redistributor regions when necessary

 hw/arm/virt-acpi-build.c           |  9 +++++++
 hw/arm/virt.c                      | 48 ++++++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_kvm.c              |  4 ++--
 hw/intc/arm_gicv3.c                | 11 ++++++++-
 hw/intc/arm_gicv3_common.c         | 35 +++++++++++++++++++++++----
 hw/intc/arm_gicv3_its_kvm.c        |  2 +-
 hw/intc/arm_gicv3_kvm.c            | 38 ++++++++++++++++++++++++++----
 include/hw/arm/virt.h              | 12 ++++++++++
 include/hw/intc/arm_gicv3_common.h |  6 +++--
 linux-headers/asm-arm/kvm.h        |  1 +
 linux-headers/asm-arm64/kvm.h      |  1 +
 target/arm/kvm.c                   | 10 +++++++-
 target/arm/kvm_arm.h               |  3 ++-
 13 files changed, 158 insertions(+), 22 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC v2 1/7] 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/v4.17-rc2-rdist-regions-v6

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

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index 4392955..81ae4e5 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -91,6 +91,7 @@ struct kvm_regs {
 #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..d41f39a 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -91,6 +91,7 @@ struct kvm_regs {
 #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 v2 2/7] target/arm: Allow KVM device address overwriting

[Eric Auger]

for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute, the attribute
data pointed to by kvm_device_attr.addr is a OR of the
redistributor region address and other fields such as the index
of the redistributor region and the number of redistributors the
region can contain.

The existing machine init done notifier framework sets the address
field to the actual address of the device and does not allow to OR
this value with other fields.

This patch extends the KVMDevice struct with a new kda_addr_fixup
member. Its value is passed at registration time and OR'ed with the
resolved address on kvm_arm_set_device_addr().

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gic_kvm.c       |  4 ++--
 hw/intc/arm_gicv3_its_kvm.c |  2 +-
 hw/intc/arm_gicv3_kvm.c     |  4 ++--
 target/arm/kvm.c            | 10 +++++++++-
 target/arm/kvm_arm.h        |  3 ++-
 5 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 6f467e6..eb9664e 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -558,7 +558,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
                             | KVM_VGIC_V2_ADDR_TYPE_DIST,
                             KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V2_ADDR_TYPE_DIST,
-                            s->dev_fd);
+                            s->dev_fd, 0);
     /* CPU interface for current core. Unlike arm_gic, we don't
      * provide the "interface for core #N" memory regions, because
      * cores with a VGIC don't have those.
@@ -568,7 +568,7 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
                             | KVM_VGIC_V2_ADDR_TYPE_CPU,
                             KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V2_ADDR_TYPE_CPU,
-                            s->dev_fd);
+                            s->dev_fd, 0);
 
     if (kvm_has_gsi_routing()) {
         /* set up irq routing */
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index eea6a73..271ebe4 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -103,7 +103,7 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
 
     /* register the base address */
     kvm_arm_register_device(&s->iomem_its_cntrl, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_ITS_ADDR_TYPE, s->dev_fd);
+                            KVM_VGIC_ITS_ADDR_TYPE, s->dev_fd, 0);
 
     gicv3_its_init_mmio(s, NULL);
 
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index ec37177..93ac293 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -754,9 +754,9 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
                       KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true, &error_abort);
 
     kvm_arm_register_device(&s->iomem_dist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd);
+                            KVM_VGIC_V3_ADDR_TYPE_DIST, s->dev_fd, 0);
     kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd);
+                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
 
     if (kvm_has_gsi_routing()) {
         /* set up irq routing */
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 5141d0a..7eed8d4 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -184,10 +184,15 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
  * We use a MemoryListener to track mapping and unmapping of
  * the regions during board creation, so the board models don't
  * need to do anything special for the KVM case.
+ *
+ * Sometimes the address must be OR'ed with some other fields
+ * (for example for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION).
+ * @kda_addr_fixup aims at storing the value of those fields.
  */
 typedef struct KVMDevice {
     struct kvm_arm_device_addr kda;
     struct kvm_device_attr kdattr;
+    uint64_t kda_addr_fixup;
     MemoryRegion *mr;
     QSLIST_ENTRY(KVMDevice) entries;
     int dev_fd;
@@ -234,6 +239,8 @@ static void kvm_arm_set_device_addr(KVMDevice *kd)
      */
     if (kd->dev_fd >= 0) {
         uint64_t addr = kd->kda.addr;
+
+        addr |= kd->kda_addr_fixup;
         attr->addr = (uintptr_t)&addr;
         ret = kvm_device_ioctl(kd->dev_fd, KVM_SET_DEVICE_ATTR, attr);
     } else {
@@ -266,7 +273,7 @@ static Notifier notify = {
 };
 
 void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
-                             uint64_t attr, int dev_fd)
+                             uint64_t attr, int dev_fd, uint64_t addr_fixup)
 {
     KVMDevice *kd;
 
@@ -286,6 +293,7 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
     kd->kdattr.group = group;
     kd->kdattr.attr = attr;
     kd->dev_fd = dev_fd;
+    kd->kda_addr_fixup = addr_fixup;
     QSLIST_INSERT_HEAD(&kvm_devices_head, kd, entries);
     memory_region_ref(kd->mr);
 }
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 1e23640..4050017 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -34,6 +34,7 @@ int kvm_arm_vcpu_init(CPUState *cs);
  * @group: device control API group for setting addresses
  * @attr: device control API address type
  * @dev_fd: device control device file descriptor (or -1 if not supported)
+ * @addr_fixup: value to be OR'ed with resolved address
  *
  * Remember the memory region @mr, and when it is mapped by the
  * machine model, tell the kernel that base address using the
@@ -45,7 +46,7 @@ int kvm_arm_vcpu_init(CPUState *cs);
  * address at the point where machine init is complete.
  */
 void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
-                             uint64_t attr, int dev_fd);
+                             uint64_t attr, int dev_fd, uint64_t addr_fixup);
 
 /**
  * kvm_arm_init_cpreg_list:
-- 
2.5.5

[Qemu-devel] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Eric Auger]

To prepare for multiple redistributor regions, we introduce
an array of uint32_t properties that stores the redistributor
count of each redistributor region.

Non accelerated VGICv3 only supports a single redistributor region.
The capacity of all redist regions is checked against the number of
vcpus.

Machvirt is updated to set the count to 123 vcpus for the unique
redistributor region we currently have.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 11b9f59..c9d842d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -525,6 +525,12 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     if (!kvm_irqchip_in_kernel()) {
         qdev_prop_set_bit(gicdev, "has-security-extensions", vms->secure);
     }
+
+    if (type == 3) {
+        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
+        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
+                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
+    }
     qdev_init_nofail(gicdev);
     gicbusdev = SYS_BUS_DEVICE(gicdev);
     sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
index 479c667..38d57ac 100644
--- a/hw/intc/arm_gicv3.c
+++ b/hw/intc/arm_gicv3.c
@@ -373,7 +373,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops);
+    if (s->nb_redist_regions != 1) {
+        error_setg(errp, "VGICv3 redist region number(%d) not equal to 1",
+                   s->nb_redist_regions);
+    }
+
+    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     gicv3_init_cpuif(s);
 }
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 7b54d52..f405ae1 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -169,9 +169,10 @@ static const VMStateDescription vmstate_gicv3 = {
 };
 
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
-                              const MemoryRegionOps *ops)
+                              const MemoryRegionOps *ops, Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(s);
+    int rdist_capacity = 0;
     int i;
 
     /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
@@ -199,11 +200,25 @@ 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,
-                          "gicv3_redist", 0x20000 * s->num_cpu);
-
     sysbus_init_mmio(sbd, &s->iomem_dist);
-    sysbus_init_mmio(sbd, &s->iomem_redist);
+
+    s->iomem_redist = g_new0(MemoryRegion, s->nb_redist_regions);
+    for (i = 0; i < s->nb_redist_regions; i++) {
+        char *name = g_strdup_printf("gicv3_redist_region[%d]", i);
+
+        memory_region_init_io(&s->iomem_redist[i], OBJECT(s),
+                              ops ? &ops[1] : NULL, s,
+                              name, 0x20000 * s->redist_region_count[i]);
+        sysbus_init_mmio(sbd, &s->iomem_redist[i]);
+        rdist_capacity += s->redist_region_count[i];
+        g_free(name);
+    }
+    if (rdist_capacity < s->num_cpu) {
+        error_setg(errp, "Capacity of the redist regions(%d) "
+                   "is less than number of vcpus(%d)",
+                   rdist_capacity, s->num_cpu);
+    }
+
 }
 
 static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
@@ -285,6 +300,13 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
     }
 }
 
+static void arm_gicv3_finalize(Object *obj)
+{
+    GICv3State *s = ARM_GICV3_COMMON(obj);
+
+    g_free(s->redist_region_count);
+}
+
 static void arm_gicv3_common_reset(DeviceState *dev)
 {
     GICv3State *s = ARM_GICV3_COMMON(dev);
@@ -388,6 +410,8 @@ static Property arm_gicv3_common_properties[] = {
     DEFINE_PROP_UINT32("num-irq", GICv3State, num_irq, 32),
     DEFINE_PROP_UINT32("revision", GICv3State, revision, 3),
     DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0),
+    DEFINE_PROP_ARRAY("redist-region-count", GICv3State, nb_redist_regions,
+                      redist_region_count, qdev_prop_uint32, uint32_t),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -409,6 +433,7 @@ static const TypeInfo arm_gicv3_common_type = {
     .instance_size = sizeof(GICv3State),
     .class_size = sizeof(ARMGICv3CommonClass),
     .class_init = arm_gicv3_common_class_init,
+    .instance_finalize = arm_gicv3_finalize,
     .abstract = true,
     .interfaces = (InterfaceInfo []) {
         { TYPE_ARM_LINUX_BOOT_IF },
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 93ac293..7e76b87 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -731,7 +731,11 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL);
+    gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     for (i = 0; i < s->num_cpu; i++) {
         ARMCPU *cpu = ARM_CPU(qemu_get_cpu(i));
@@ -755,7 +759,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, 0);
-    kvm_arm_register_device(&s->iomem_redist, -1, KVM_DEV_ARM_VGIC_GRP_ADDR,
+    kvm_arm_register_device(&s->iomem_redist[0], -1,
+                            KVM_DEV_ARM_VGIC_GRP_ADDR,
                             KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
 
     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..538dcc5 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -210,7 +210,9 @@ struct GICv3State {
     /*< public >*/
 
     MemoryRegion iomem_dist; /* Distributor */
-    MemoryRegion iomem_redist; /* Redistributors */
+    MemoryRegion *iomem_redist; /* Redistributor Regions */
+    uint32_t *redist_region_count; /* redistributor count within each region */
+    uint32_t nb_redist_regions; /* number of redist regions */
 
     uint32_t num_cpu;
     uint32_t num_irq;
@@ -291,6 +293,6 @@ typedef struct ARMGICv3CommonClass {
 } ARMGICv3CommonClass;
 
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
-                              const MemoryRegionOps *ops);
+                              const MemoryRegionOps *ops, Error **errp);
 
 #endif
-- 
2.5.5

[Qemu-devel] [RFC v2 4/7] hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions

[Eric Auger]

Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
If not, we check the number of redist region is equal to 1 and use the
legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
the new attribute and allow to register multiple regions to the
KVM device.

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

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 7e76b87..1b32804 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -714,6 +714,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 {
     GICv3State *s = KVM_ARM_GICV3(dev);
     KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
+    bool multiple_redist_region_allowed;
     Error *local_err = NULL;
     int i;
 
@@ -750,6 +751,18 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    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 (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
+        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
+                   "supported by this host kernel");
+        error_append_hint(errp, "A maximum of %d VCPUs can be used",
+                          s->redist_region_count[0]);
+        return;
+    }
+
     kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
                       0, &s->num_irq, true, &error_abort);
 
@@ -759,9 +772,21 @@ 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, 0);
-    kvm_arm_register_device(&s->iomem_redist[0], -1,
-                            KVM_DEV_ARM_VGIC_GRP_ADDR,
-                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
+
+    if (!multiple_redist_region_allowed) {
+        kvm_arm_register_device(&s->iomem_redist[0], -1,
+                                KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
+    } else {
+        for (i = s->nb_redist_regions; i >= 0; i--) {
+            uint64_t val = i | ((uint64_t)s->redist_region_count[i] << 52);
+
+            kvm_arm_register_device(&s->iomem_redist[i], -1,
+                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
+                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
+                                    s->dev_fd, val);
+        }
+    }
 
     if (kvm_has_gsi_routing()) {
         /* set up irq routing */
-- 
2.5.5

[Qemu-devel] [RFC v2 5/7] hw/arm/virt: GICv3 DT node with one or two redistributor regions

[Eric Auger]

This patch allows the creation of a GICv3 node with 1 or 2
redistributor regions depending on the number of smu_cpus.
The second redistributor region is located just after the
existing RAM region, at 256GB and contains up to (512 - 123) vcpus.

Please refer to kernel documentation for further node details:
Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c9d842d..f35962a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -151,6 +151,8 @@ 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 },
 };
@@ -404,13 +406,30 @@ 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) {
+        int nb_redist_regions = virt_gicv3_redist_region_count(vms);
+
         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);
+
+        qemu_fdt_setprop_cell(vms->fdt, "/intc",
+                              "#redistributor-regions", nb_redist_regions);
+
+        if (nb_redist_regions == 1) {
+            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);
+        } else {
+            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,
+                                         2, vms->memmap[VIRT_GIC_REDIST2].base,
+                                         2, vms->memmap[VIRT_GIC_REDIST2].size);
+        }
+
         if (vms->virt) {
             qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GICV3_MAINT_IRQ,
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 886372c..ba52716 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 "sysemu/kvm.h"
 
 #define NUM_GICV2M_SPIS       64
 #define NUM_VIRTIO_TRANSPORTS 32
@@ -60,6 +61,7 @@ enum {
     VIRT_GIC_V2M,
     VIRT_GIC_ITS,
     VIRT_GIC_REDIST,
+    VIRT_GIC_REDIST2,
     VIRT_SMMU,
     VIRT_UART,
     VIRT_MMIO,
@@ -129,4 +131,14 @@ typedef struct {
 
 void virt_acpi_setup(VirtMachineState *vms);
 
+/* Return the number of used redistributor regions  */
+static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
+{
+    uint32_t rdist1_count = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
+
+    assert(vms->gic_version == 3);
+
+    return kvm_irqchip_in_kernel() && vms->smp_cpus > rdist1_count ? 2 : 1;
+}
+
 #endif /* QEMU_ARM_VIRT_H */
-- 
2.5.5

[Qemu-devel] [RFC v2 6/7] hw/arm/virt-acpi-build: Advertise one or two GICR structures

[Eric Auger]

Depending on the number of smp_cpus we now register one or two
GICR structures.

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

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 92ceee9..6a4340a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -660,6 +660,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 
     if (vms->gic_version == 3) {
         AcpiMadtGenericTranslator *gic_its;
+        int nb_redist_regions = virt_gicv3_redist_region_count(vms);
         AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
                                                          sizeof *gicr);
 
@@ -668,6 +669,14 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
         gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
 
+        if (nb_redist_regions == 2) {
+            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_REDIST2].base);
+            gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST2].size);
+        }
+
         if (its_class_name() && !vmc->no_its) {
             gic_its = acpi_data_push(table_data, sizeof *gic_its);
             gic_its->type = ACPI_APIC_GENERIC_TRANSLATOR;
-- 
2.5.5

[Qemu-devel] [RFC v2 7/7] hw/arm/virt: Register two redistributor regions when necessary

[Eric Auger]

With a VGICv3 KVM device, if the number of vcpus exceeds the
capacity of the legacy redistributor region (123 redistributors),
we now attempt to register the second redistributor region. This
extends the number of vcpus to 512 assuming the host kernel supports:
- up to 512 vcpus
- VGICv3 KVM device group/attribute:
  KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION

In case the host kernel does not support the registration of several
redistributor regions, the GICv3 device initialization fails
with a proper error message and qemu exits.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f35962a..8d43c51 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     SysBusDevice *gicbusdev;
     const char *gictype;
     int type = vms->gic_version, i;
+    uint32_t nb_redist_regions = 0;
 
     gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
 
@@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
     }
 
     if (type == 3) {
-        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
+        nb_redist_regions = virt_gicv3_redist_region_count(vms);
+
+        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
+                             nb_redist_regions);
         qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
                              vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
+
+        if (nb_redist_regions == 2) {
+            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",
+                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);
+        }
     }
     qdev_init_nofail(gicdev);
     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);
+        if (nb_redist_regions == 2) {
+            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
+        }
     } else {
         sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
     }
@@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
      */
     if (vms->gic_version == 3) {
         virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
+        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
     } else {
         virt_max_cpus = GIC_NCPU;
     }
-- 
2.5.5

Re: [Qemu-devel] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> To prepare for multiple redistributor regions, we introduce
> an array of uint32_t properties that stores the redistributor
> count of each redistributor region.
>
> Non accelerated VGICv3 only supports a single redistributor region.
> The capacity of all redist regions is checked against the number of
> vcpus.
>
> Machvirt is updated to set the count to 123 vcpus for the unique
> redistributor region we currently have.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c                      |  6 ++++++
>  hw/intc/arm_gicv3.c                | 11 ++++++++++-
>  hw/intc/arm_gicv3_common.c         | 35 ++++++++++++++++++++++++++++++-----
>  hw/intc/arm_gicv3_kvm.c            |  9 +++++++--
>  include/hw/intc/arm_gicv3_common.h |  6 ++++--
>  5 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 11b9f59..c9d842d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -525,6 +525,12 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      if (!kvm_irqchip_in_kernel()) {
>          qdev_prop_set_bit(gicdev, "has-security-extensions", vms->secure);
>      }
> +
> +    if (type == 3) {
> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
> +        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
> +                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);

We used to create a region which had num_cpu redistributors in it;
won't this cause us to create one which has as many redistributors
as will fit in the space ?

> +    }
>      qdev_init_nofail(gicdev);
>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 479c667..38d57ac 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -373,7 +373,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops);
> +    if (s->nb_redist_regions != 1) {
> +        error_setg(errp, "VGICv3 redist region number(%d) not equal to 1",
> +                   s->nb_redist_regions);

Missing 'return' ?

> +    }
> +
> +    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>
>      gicv3_init_cpuif(s);
>  }
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 7b54d52..f405ae1 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -169,9 +169,10 @@ static const VMStateDescription vmstate_gicv3 = {
>  };
>
>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> -                              const MemoryRegionOps *ops)
> +                              const MemoryRegionOps *ops, Error **errp)
>  {
>      SysBusDevice *sbd = SYS_BUS_DEVICE(s);
> +    int rdist_capacity = 0;
>      int i;
>
>      /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
> @@ -199,11 +200,25 @@ 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,
> -                          "gicv3_redist", 0x20000 * s->num_cpu);
> -
>      sysbus_init_mmio(sbd, &s->iomem_dist);
> -    sysbus_init_mmio(sbd, &s->iomem_redist);
> +
> +    s->iomem_redist = g_new0(MemoryRegion, s->nb_redist_regions);
> +    for (i = 0; i < s->nb_redist_regions; i++) {
> +        char *name = g_strdup_printf("gicv3_redist_region[%d]", i);
> +
> +        memory_region_init_io(&s->iomem_redist[i], OBJECT(s),
> +                              ops ? &ops[1] : NULL, s,
> +                              name, 0x20000 * s->redist_region_count[i]);
> +        sysbus_init_mmio(sbd, &s->iomem_redist[i]);
> +        rdist_capacity += s->redist_region_count[i];
> +        g_free(name);
> +    }
> +    if (rdist_capacity < s->num_cpu) {
> +        error_setg(errp, "Capacity of the redist regions(%d) "
> +                   "is less than number of vcpus(%d)",
> +                   rdist_capacity, s->num_cpu);

It would be better to make this check before we create a lot of
memory regions, I think. (Though I'm very unsure of the rules about
how to unwind what you've done in a realize method that's about to fail;
we probably get it wrong a lot and don't care because realize failure
is fatal for most uses of most devices.)

> +    }
> +
>  }
>
>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 2/7] target/arm: Allow KVM device address overwriting

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute, the attribute
> data pointed to by kvm_device_attr.addr is a OR of the
> redistributor region address and other fields such as the index
> of the redistributor region and the number of redistributors the
> region can contain.
>
> The existing machine init done notifier framework sets the address
> field to the actual address of the device and does not allow to OR
> this value with other fields.
>
> This patch extends the KVMDevice struct with a new kda_addr_fixup
> member. Its value is passed at registration time and OR'ed with the
> resolved address on kvm_arm_set_device_addr().


> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 1e23640..4050017 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -34,6 +34,7 @@ int kvm_arm_vcpu_init(CPUState *cs);
>   * @group: device control API group for setting addresses
>   * @attr: device control API address type
>   * @dev_fd: device control device file descriptor (or -1 if not supported)
> + * @addr_fixup: value to be OR'ed with resolved address

I think addr_fixup is a bit vague; how about
 addr_fixed_bits ?
(here and in the function argument name). Or something else if
somebody has a better idea.

>   *
>   * Remember the memory region @mr, and when it is mapped by the
>   * machine model, tell the kernel that base address using the
> @@ -45,7 +46,7 @@ int kvm_arm_vcpu_init(CPUState *cs);
>   * address at the point where machine init is complete.
>   */
>  void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
> -                             uint64_t attr, int dev_fd);
> +                             uint64_t attr, int dev_fd, uint64_t addr_fixup);
>
>  /**
>   * kvm_arm_init_cpreg_list:
> --
> 2.5.5

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 4/7] hw/intc/arm_gicv3_kvm: Get prepared to handle multiple redist regions

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> Let's check if KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION is supported.
> If not, we check the number of redist region is equal to 1 and use the
> legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute. Otherwise we use
> the new attribute and allow to register multiple regions to the
> KVM device.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/intc/arm_gicv3_kvm.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 7e76b87..1b32804 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -714,6 +714,7 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>  {
>      GICv3State *s = KVM_ARM_GICV3(dev);
>      KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
> +    bool multiple_redist_region_allowed;
>      Error *local_err = NULL;
>      int i;
>
> @@ -750,6 +751,18 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> +    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 (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
> +        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
> +                   "supported by this host kernel");
> +        error_append_hint(errp, "A maximum of %d VCPUs can be used",
> +                          s->redist_region_count[0]);
> +        return;
> +    }
> +
>      kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
>                        0, &s->num_irq, true, &error_abort);
>
> @@ -759,9 +772,21 @@ 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, 0);
> -    kvm_arm_register_device(&s->iomem_redist[0], -1,
> -                            KVM_DEV_ARM_VGIC_GRP_ADDR,
> -                            KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> +
> +    if (!multiple_redist_region_allowed) {
> +        kvm_arm_register_device(&s->iomem_redist[0], -1,
> +                                KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                                KVM_VGIC_V3_ADDR_TYPE_REDIST, s->dev_fd, 0);
> +    } else {
> +        for (i = s->nb_redist_regions; i >= 0; i--) {
> +            uint64_t val = i | ((uint64_t)s->redist_region_count[i] << 52);

A comment here about what this value is would be useful.

> +
> +            kvm_arm_register_device(&s->iomem_redist[i], -1,
> +                                    KVM_DEV_ARM_VGIC_GRP_ADDR,
> +                                    KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION,
> +                                    s->dev_fd, val);
> +        }
> +    }
>
>      if (kvm_has_gsi_routing()) {
>          /* set up irq routing */

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 5/7] hw/arm/virt: GICv3 DT node with one or two redistributor regions

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> This patch allows the creation of a GICv3 node with 1 or 2
> redistributor regions depending on the number of smu_cpus.
> The second redistributor region is located just after the
> existing RAM region, at 256GB and contains up to (512 - 123) vcpus.
>
> Please refer to kernel documentation for further node details:
> Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c         | 29 ++++++++++++++++++++++++-----
>  include/hw/arm/virt.h | 12 ++++++++++++
>  2 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c9d842d..f35962a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -151,6 +151,8 @@ 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 },

This is a weird size.

>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>      [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>  };
> @@ -404,13 +406,30 @@ 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) {
> +        int nb_redist_regions = virt_gicv3_redist_region_count(vms);
> +
>          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);
> +
> +        qemu_fdt_setprop_cell(vms->fdt, "/intc",
> +                              "#redistributor-regions", nb_redist_regions);
> +
> +        if (nb_redist_regions == 1) {
> +            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);
> +        } else {
> +            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,
> +                                         2, vms->memmap[VIRT_GIC_REDIST2].base,
> +                                         2, vms->memmap[VIRT_GIC_REDIST2].size);
> +        }
> +
>          if (vms->virt) {
>              qemu_fdt_setprop_cells(vms->fdt, "/intc", "interrupts",
>                                     GIC_FDT_IRQ_TYPE_PPI, ARCH_GICV3_MAINT_IRQ,
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 886372c..ba52716 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 "sysemu/kvm.h"
>
>  #define NUM_GICV2M_SPIS       64
>  #define NUM_VIRTIO_TRANSPORTS 32
> @@ -60,6 +61,7 @@ enum {
>      VIRT_GIC_V2M,
>      VIRT_GIC_ITS,
>      VIRT_GIC_REDIST,
> +    VIRT_GIC_REDIST2,
>      VIRT_SMMU,
>      VIRT_UART,
>      VIRT_MMIO,
> @@ -129,4 +131,14 @@ typedef struct {
>
>  void virt_acpi_setup(VirtMachineState *vms);
>
> +/* Return the number of used redistributor regions  */
> +static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
> +{
> +    uint32_t rdist1_count = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +
> +    assert(vms->gic_version == 3);
> +
> +    return kvm_irqchip_in_kernel() && vms->smp_cpus > rdist1_count ? 2 : 1;

Why does kvm_irqchip_in_kernel() matter here? If we have more CPUs than
we can fit in the first region, we need 2 regardless. If we're using
the TCG GICv3 then hopefully we'll bail out with a useful error
message when we try to create multiple redistributor regions for it
(until we add support for that).

> +}

I notice there's a lot of 0x20000 constants floating about in this
patch set; could we use a suitable #define ?

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 7/7] hw/arm/virt: Register two redistributor regions when necessary

[Peter Maydell]

On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
> With a VGICv3 KVM device, if the number of vcpus exceeds the
> capacity of the legacy redistributor region (123 redistributors),
> we now attempt to register the second redistributor region. This
> extends the number of vcpus to 512 assuming the host kernel supports:
> - up to 512 vcpus
> - VGICv3 KVM device group/attribute:
>   KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>
> In case the host kernel does not support the registration of several
> redistributor regions, the GICv3 device initialization fails
> with a proper error message and qemu exits.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f35962a..8d43c51 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      SysBusDevice *gicbusdev;
>      const char *gictype;
>      int type = vms->gic_version, i;
> +    uint32_t nb_redist_regions = 0;
>
>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
>
> @@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>      }
>
>      if (type == 3) {
> -        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
> +        nb_redist_regions = virt_gicv3_redist_region_count(vms);
> +
> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
> +                             nb_redist_regions);
>          qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>                               vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
> +
> +        if (nb_redist_regions == 2) {
> +            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",

Stray extra space before comma.

> +                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);

This is going to ask for more redistributors than we want (cf
earlier review comment).

> +        }
>      }
>      qdev_init_nofail(gicdev);
>      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);
> +        if (nb_redist_regions == 2) {
> +            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
> +        }
>      } else {
>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>      }
> @@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
>       */
>      if (vms->gic_version == 3) {
>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
> +        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;

Slight pity that we can't determine whether we can support multiple
redistributor regions here, but I guess it's not a big deal.

>      } else {
>          virt_max_cpus = GIC_NCPU;
>      }
> --
> 2.5.5

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 2/7] target/arm: Allow KVM device address overwriting

[Peter Maydell]

On 22 May 2018 at 13:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>> for KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION attribute, the attribute
>> data pointed to by kvm_device_attr.addr is a OR of the
>> redistributor region address and other fields such as the index
>> of the redistributor region and the number of redistributors the
>> region can contain.
>>
>> The existing machine init done notifier framework sets the address
>> field to the actual address of the device and does not allow to OR
>> this value with other fields.
>>
>> This patch extends the KVMDevice struct with a new kda_addr_fixup
>> member. Its value is passed at registration time and OR'ed with the
>> resolved address on kvm_arm_set_device_addr().
>
>
>> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
>> index 1e23640..4050017 100644
>> --- a/target/arm/kvm_arm.h
>> +++ b/target/arm/kvm_arm.h
>> @@ -34,6 +34,7 @@ int kvm_arm_vcpu_init(CPUState *cs);
>>   * @group: device control API group for setting addresses
>>   * @attr: device control API address type
>>   * @dev_fd: device control device file descriptor (or -1 if not supported)
>> + * @addr_fixup: value to be OR'ed with resolved address
>
> I think addr_fixup is a bit vague; how about
>  addr_fixed_bits ?
> (here and in the function argument name). Or something else if
> somebody has a better idea.

Thomas Huth suggested addr_ormask.

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Auger Eric]

Hi Peter,
On 05/22/2018 02:27 PM, Peter Maydell wrote:
> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>> To prepare for multiple redistributor regions, we introduce
>> an array of uint32_t properties that stores the redistributor
>> count of each redistributor region.
>>
>> Non accelerated VGICv3 only supports a single redistributor region.
>> The capacity of all redist regions is checked against the number of
>> vcpus.
>>
>> Machvirt is updated to set the count to 123 vcpus for the unique
>> redistributor region we currently have.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c                      |  6 ++++++
>>  hw/intc/arm_gicv3.c                | 11 ++++++++++-
>>  hw/intc/arm_gicv3_common.c         | 35 ++++++++++++++++++++++++++++++-----
>>  hw/intc/arm_gicv3_kvm.c            |  9 +++++++--
>>  include/hw/intc/arm_gicv3_common.h |  6 ++++--
>>  5 files changed, 57 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 11b9f59..c9d842d 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -525,6 +525,12 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      if (!kvm_irqchip_in_kernel()) {
>>          qdev_prop_set_bit(gicdev, "has-security-extensions", vms->secure);
>>      }
>> +
>> +    if (type == 3) {
>> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
>> +        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>> +                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
> 
> We used to create a region which had num_cpu redistributors in it;
> won't this cause us to create one which has as many redistributors
> as will fit in the space ?
Is that an issue? From a machine perspective the whole region is
reserved for rdist. dt and ACPI will expose this whole region and the
device will use a subset of it? I agree I need to document this change
in the commit message though.
> 
>> +    }
>>      qdev_init_nofail(gicdev);
>>      gicbusdev = SYS_BUS_DEVICE(gicdev);
>>      sysbus_mmio_map(gicbusdev, 0, vms->memmap[VIRT_GIC_DIST].base);
>> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
>> index 479c667..38d57ac 100644
>> --- a/hw/intc/arm_gicv3.c
>> +++ b/hw/intc/arm_gicv3.c
>> @@ -373,7 +373,16 @@ static void arm_gic_realize(DeviceState *dev, Error **errp)
>>          return;
>>      }
>>
>> -    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops);
>> +    if (s->nb_redist_regions != 1) {
>> +        error_setg(errp, "VGICv3 redist region number(%d) not equal to 1",
>> +                   s->nb_redist_regions);
> 
> Missing 'return' ?
yes!
> 
>> +    }
>> +
>> +    gicv3_init_irqs_and_mmio(s, gicv3_set_irq, gic_ops, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>
>>      gicv3_init_cpuif(s);
>>  }
>> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
>> index 7b54d52..f405ae1 100644
>> --- a/hw/intc/arm_gicv3_common.c
>> +++ b/hw/intc/arm_gicv3_common.c
>> @@ -169,9 +169,10 @@ static const VMStateDescription vmstate_gicv3 = {
>>  };
>>
>>  void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
>> -                              const MemoryRegionOps *ops)
>> +                              const MemoryRegionOps *ops, Error **errp)
>>  {
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>> +    int rdist_capacity = 0;
>>      int i;
>>
>>      /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
>> @@ -199,11 +200,25 @@ 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,
>> -                          "gicv3_redist", 0x20000 * s->num_cpu);
>> -
>>      sysbus_init_mmio(sbd, &s->iomem_dist);
>> -    sysbus_init_mmio(sbd, &s->iomem_redist);
>> +
>> +    s->iomem_redist = g_new0(MemoryRegion, s->nb_redist_regions);
>> +    for (i = 0; i < s->nb_redist_regions; i++) {
>> +        char *name = g_strdup_printf("gicv3_redist_region[%d]", i);
>> +
>> +        memory_region_init_io(&s->iomem_redist[i], OBJECT(s),
>> +                              ops ? &ops[1] : NULL, s,
>> +                              name, 0x20000 * s->redist_region_count[i]);
>> +        sysbus_init_mmio(sbd, &s->iomem_redist[i]);
>> +        rdist_capacity += s->redist_region_count[i];
>> +        g_free(name);
>> +    }
>> +    if (rdist_capacity < s->num_cpu) {
>> +        error_setg(errp, "Capacity of the redist regions(%d) "
>> +                   "is less than number of vcpus(%d)",
>> +                   rdist_capacity, s->num_cpu);
> 
> It would be better to make this check before we create a lot of
> memory regions, I think. (Though I'm very unsure of the rules about
> how to unwind what you've done in a realize method that's about to fail;
> we probably get it wrong a lot and don't care because realize failure
> is fatal for most uses of most devices.)
I moved the rdist_capacity computation and check at the beginning of the
function. Anyway if the num_cpu rdists cannot fit in the rdist regions,
this is fatal. virt calls qdev_init_nofail(gicdev);

Thanks

Eric
> 
>> +    }
>> +
>>  }
>>
>>  static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Peter Maydell]

On 29 May 2018 at 10:08, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
> On 05/22/2018 02:27 PM, Peter Maydell wrote:
>> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>>> To prepare for multiple redistributor regions, we introduce
>>> an array of uint32_t properties that stores the redistributor
>>> count of each redistributor region.
>>>
>>> Non accelerated VGICv3 only supports a single redistributor region.
>>> The capacity of all redist regions is checked against the number of
>>> vcpus.
>>>
>>> Machvirt is updated to set the count to 123 vcpus for the unique
>>> redistributor region we currently have.

>>> +
>>> +    if (type == 3) {
>>> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
>>> +        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>>> +                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
>>
>> We used to create a region which had num_cpu redistributors in it;
>> won't this cause us to create one which has as many redistributors
>> as will fit in the space ?
> Is that an issue? From a machine perspective the whole region is
> reserved for rdist. dt and ACPI will expose this whole region and the
> device will use a subset of it? I agree I need to document this change
> in the commit message though.

It's a difference, and a guest-visible difference too. This
patchset is supposed to be introducing split-redistributor-regions,
not changing the behaviour of other configs. Also, I don't think
it makes sense to model a GIC with more redistributors than CPUs:
real hardware doesn't look like that AFAIK.

thanks
-- PMM

Re: [Qemu-devel] [RFC v2 7/7] hw/arm/virt: Register two redistributor regions when necessary

[Auger Eric]

Hi,

On 05/22/2018 02:43 PM, Peter Maydell wrote:
> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>> With a VGICv3 KVM device, if the number of vcpus exceeds the
>> capacity of the legacy redistributor region (123 redistributors),
>> we now attempt to register the second redistributor region. This
>> extends the number of vcpus to 512 assuming the host kernel supports:
>> - up to 512 vcpus

I just noticed virt_machine_class_init still clamps the max_cpus to 255.
This is checked in vl.c

Do we want the 3.0 machine expose more than 255 vcpus, 512 for instance?
I understand this mc->max_cpus is something rather static, that is
refined afterwards.

Note the comment in virt_machine_class_init suggest 255 is the max QEMU
supports but I can see that pc_q35.c sets the max_cpus to 288 already.

Thanks

Eric

>> - VGICv3 KVM device group/attribute:
>>   KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION
>>
>> In case the host kernel does not support the registration of several
>> redistributor regions, the GICv3 device initialization fails
>> with a proper error message and qemu exits.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f35962a..8d43c51 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -531,6 +531,7 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      SysBusDevice *gicbusdev;
>>      const char *gictype;
>>      int type = vms->gic_version, i;
>> +    uint32_t nb_redist_regions = 0;
>>
>>      gictype = (type == 3) ? gicv3_class_name() : gic_class_name();
>>
>> @@ -546,15 +547,26 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>>      }
>>
>>      if (type == 3) {
>> -        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
>> +        nb_redist_regions = virt_gicv3_redist_region_count(vms);
>> +
>> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count",
>> +                             nb_redist_regions);
>>          qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>>                               vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
>> +
>> +        if (nb_redist_regions == 2) {
>> +            qdev_prop_set_uint32(gicdev , "redist-region-count[1]",
> 
> Stray extra space before comma.
> 
>> +                                 vms->memmap[VIRT_GIC_REDIST2].size / 0x20000);
> 
> This is going to ask for more redistributors than we want (cf
> earlier review comment).
> 
>> +        }
>>      }
>>      qdev_init_nofail(gicdev);
>>      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);
>> +        if (nb_redist_regions == 2) {
>> +            sysbus_mmio_map(gicbusdev, 2, vms->memmap[VIRT_GIC_REDIST2].base);
>> +        }
>>      } else {
>>          sysbus_mmio_map(gicbusdev, 1, vms->memmap[VIRT_GIC_CPU].base);
>>      }
>> @@ -1346,6 +1358,7 @@ static void machvirt_init(MachineState *machine)
>>       */
>>      if (vms->gic_version == 3) {
>>          virt_max_cpus = vms->memmap[VIRT_GIC_REDIST].size / 0x20000;
>> +        virt_max_cpus += vms->memmap[VIRT_GIC_REDIST2].size / 0x20000;
> 
> Slight pity that we can't determine whether we can support multiple
> redistributor regions here, but I guess it's not a big deal.
> 
>>      } else {
>>          virt_max_cpus = GIC_NCPU;
>>      }
>> --
>> 2.5.5
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC v2 3/7] hw/intc/arm_gicv3: Introduce redist-region-count array property

[Auger Eric]

Hi Peter,

On 05/29/2018 11:13 AM, Peter Maydell wrote:
> On 29 May 2018 at 10:08, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Peter,
>> On 05/22/2018 02:27 PM, Peter Maydell wrote:
>>> On 13 May 2018 at 15:35, Eric Auger <eric.auger@redhat.com> wrote:
>>>> To prepare for multiple redistributor regions, we introduce
>>>> an array of uint32_t properties that stores the redistributor
>>>> count of each redistributor region.
>>>>
>>>> Non accelerated VGICv3 only supports a single redistributor region.
>>>> The capacity of all redist regions is checked against the number of
>>>> vcpus.
>>>>
>>>> Machvirt is updated to set the count to 123 vcpus for the unique
>>>> redistributor region we currently have.
> 
>>>> +
>>>> +    if (type == 3) {
>>>> +        qdev_prop_set_uint32(gicdev, "len-redist-region-count", 1);
>>>> +        qdev_prop_set_uint32(gicdev , "redist-region-count[0]",
>>>> +                             vms->memmap[VIRT_GIC_REDIST].size / 0x20000);
>>>
>>> We used to create a region which had num_cpu redistributors in it;
>>> won't this cause us to create one which has as many redistributors
>>> as will fit in the space ?
>> Is that an issue? From a machine perspective the whole region is
>> reserved for rdist. dt and ACPI will expose this whole region and the
>> device will use a subset of it? I agree I need to document this change
>> in the commit message though.
> 
> It's a difference, and a guest-visible difference too. This
> patchset is supposed to be introducing split-redistributor-regions,
> not changing the behaviour of other configs. Also, I don't think
> it makes sense to model a GIC with more redistributors than CPUs:
> real hardware doesn't look like that AFAIK.

OK I updated the series accordingly.

Thanks

Eric
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC v2 7/7] hw/arm/virt: Register two redistributor regions when necessary

[Peter Maydell]

On 29 May 2018 at 14:43, Auger Eric <eric.auger@redhat.com> wrote:
> I just noticed virt_machine_class_init still clamps the max_cpus to 255.
> This is checked in vl.c
>
> Do we want the 3.0 machine expose more than 255 vcpus, 512 for instance?
> I understand this mc->max_cpus is something rather static, that is
> refined afterwards.

Mmm. It's mostly used by the option checking code in vl.c, which
runs before the board init function. So really what we're doing by
setting it to some high number is disabling those checks; we then
have to mess around with checking manually later. Unfortunately
I don't think we have enough info to set the real maximum early
enough to avoid this.

We should probably set max_cpus here to whatever is the maximum the
board can handle in any configuration.

> Note the comment in virt_machine_class_init suggest 255 is the max QEMU
> supports but I can see that pc_q35.c sets the max_cpus to 288 already.

I think that's out of date, yes -- probably when the code was written
255 was the maximum.

thanks
-- PMM