Hi Peter,
On 05/22/2018 02:27 PM, Peter Maydell wrote:
> On 13 May 2018 at 15:35, Eric Auger <eric.au...@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.au...@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
>
