Re: [Qemu-arm] [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 <address@hidden> 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 <address@hidden>
>> ---
>>  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
>