Hi Peter,
On 13/04/18 15:27, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger <eric.au...@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.au...@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
>
