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