[RFC,7/8] hw/arm/virt-acpi-build: Handle multiple GICR structures

In case multiple redistributor regions were registered,
let's add the corresponding GICR structures in the MADT
ACPI table.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

On Tue, Mar 27, 2018 at 04:15:21PM +0200, Eric Auger wrote:
> In case multiple redistributor regions were registered,
> let's add the corresponding GICR structures in the MADT
> ACPI table.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index c7c6a57..aefc1b4 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -43,6 +43,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> +#include "hw/intc/arm_gicv3.h"
>  #include "sysemu/numa.h"
>  #include "kvm_arm.h"
>  
> @@ -618,14 +619,22 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      }
>  
>      if (vms->gic_version == 3) {
> +        GICv3State *s = (GICv3State *)vms->gic;
> +        int r;
> +
>          AcpiMadtGenericTranslator *gic_its;
> -        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
> -                                                         sizeof *gicr);
>  
> -        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> -        gicr->length = sizeof(*gicr);
> -        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
> -        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
> +        for (r = 0; r <  s->nb_redist_regions; r++) {
                           ^ extra space
> +            AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
> +                                                                sizeof *gicr);
> +
> +            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
> +            gicr->length = sizeof(*gicr);

This file has inconsistent use of () with sizeof. If you want to start
it on its path to using ()'s, like the majority of QEMU code, then you
could add them to the acpi_data_push() above.

> +            gicr->base_address = cpu_to_le64(s->redist_region[r].base);
> +            /* count redistributors of 2 x 64kB pages */
> +            gicr->range_length =
> +                cpu_to_le64((uint64_t)s->redist_region[r].count << 17);
                            ^^ range_length is only 4 bytes.

It might be nice to have a define of some sort for the 2*64K to avoid it
getting scattered everywhere. 

> +        }
>  
>          if (its_class_name() && !vmc->no_its) {
>              gic_its = acpi_data_push(table_data, sizeof *gic_its);
> -- 
> 2.5.5
> 
>

Thanks,
drew

Hi Drew,

On 13/04/18 15:47, Andrew Jones wrote:
> On Tue, Mar 27, 2018 at 04:15:21PM +0200, Eric Auger wrote:
>> In case multiple redistributor regions were registered,
>> let's add the corresponding GICR structures in the MADT
>> ACPI table.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index c7c6a57..aefc1b4 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -43,6 +43,7 @@
>>  #include "hw/pci/pcie_host.h"
>>  #include "hw/pci/pci.h"
>>  #include "hw/arm/virt.h"
>> +#include "hw/intc/arm_gicv3.h"
>>  #include "sysemu/numa.h"
>>  #include "kvm_arm.h"
>>  
>> @@ -618,14 +619,22 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>      }
>>  
>>      if (vms->gic_version == 3) {
>> +        GICv3State *s = (GICv3State *)vms->gic;
>> +        int r;
>> +
>>          AcpiMadtGenericTranslator *gic_its;
>> -        AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
>> -                                                         sizeof *gicr);
>>  
>> -        gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
>> -        gicr->length = sizeof(*gicr);
>> -        gicr->base_address = cpu_to_le64(memmap[VIRT_GIC_REDIST].base);
>> -        gicr->range_length = cpu_to_le32(memmap[VIRT_GIC_REDIST].size);
>> +        for (r = 0; r <  s->nb_redist_regions; r++) {
>                            ^ extra space
>> +            AcpiMadtGenericRedistributor *gicr = acpi_data_push(table_data,
>> +                                                                sizeof *gicr);
>> +
>> +            gicr->type = ACPI_APIC_GENERIC_REDISTRIBUTOR;
>> +            gicr->length = sizeof(*gicr);
> 
> This file has inconsistent use of () with sizeof. If you want to start
> it on its path to using ()'s, like the majority of QEMU code, then you
> could add them to the acpi_data_push() above.
I will align with the rest of the code
> 
>> +            gicr->base_address = cpu_to_le64(s->redist_region[r].base);
>> +            /* count redistributors of 2 x 64kB pages */
>> +            gicr->range_length =
>> +                cpu_to_le64((uint64_t)s->redist_region[r].count << 17);
>                             ^^ range_length is only 4 bytes.
> 
> It might be nice to have a define of some sort for the 2*64K to avoid it
> getting scattered everywhere. 
sure

Thanks

Eric
> 
>> +        }
>>  
>>          if (its_class_name() && !vmc->no_its) {
>>              gic_its = acpi_data_push(table_data, sizeof *gic_its);
>> -- 
>> 2.5.5
>>
>>
> 
> Thanks,
> drew 
>