Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API
Hi Peter, On 13/04/18 15:46, Peter Maydell wrote: > On 13 April 2018 at 14:44, Auger Eric wrote: >> 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. > > I think I'd rather that we continue to tell the kernel about > the addresses of device related regions in the same general > way that we do at the moment. OK I will rework in that direction then. Thank you for the review! Eric > > thanks > -- PMM >
Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API
On 13 April 2018 at 14:44, Auger Eric wrote: > 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. I think I'd rather that we continue to tell the kernel about the addresses of device related regions in the same general way that we do at the moment. thanks -- PMM
Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API
Hi Peter,
On 13/04/18 15:34, Peter Maydell wrote:
> On 27 March 2018 at 15:15, Eric Auger 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
>> ---
>> 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", 0x2 * 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
>
Re: [Qemu-devel] [RFC 4/8] hw/intc/arm_gicv3: Implement register_redist_region API
On 27 March 2018 at 15:15, Eric Auger 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
> ---
> 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 17 ?
> } 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", 0x2 * 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).
> 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.
> } ARMGICv3CommonClass;
>
> void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
> --
> 2.5.5
thanks
-- PMM
