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

Message ID 1522160122-10744-8-git-send-email-eric.auger@redhat.com
State New
Headers show
Series
  • KVM/ARM: Relax the max 123 vcpus limitation along with KVM GICv3
Related show

Commit Message

Auger Eric March 27, 2018, 2:15 p.m. UTC
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(-)

Comments

Andrew Jones April 13, 2018, 1:47 p.m. UTC | #1
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
Auger Eric April 13, 2018, 1:55 p.m. UTC | #2
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 
>

Patch
diff mbox series

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++) {
+            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(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);
+        }
 
         if (its_class_name() && !vmc->no_its) {
             gic_its = acpi_data_push(table_data, sizeof *gic_its);