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.au...@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
>
