Re: [RFC PATCH v2 07/13] hw/arm/virt-acpi-build: distinguish possible and present cpus Message

From: Ying Fang <fangying1@huawei.com>
To: Andrew Jones <drjones@redhat.com>
Cc: peter.maydell@linaro.org, zhang.zhanghailiang@huawei.com,
	qemu-devel@nongnu.org, alex.chen@huawei.com,
	shannon.zhaosl@gmail.com, qemu-arm@nongnu.org,
	alistair.francis@wdc.com, imammedo@redhat.com
Subject: Re: [RFC PATCH v2 07/13] hw/arm/virt-acpi-build: distinguish possible and present cpus Message
Date: Mon, 2 Nov 2020 11:13:24 +0800	[thread overview]
Message-ID: <da7abc9a-873f-b968-e366-3bdcedbf81ef@huawei.com> (raw)
In-Reply-To: <20201029172016.rsgo4stjrkdr7j2r@kamzik.brq.redhat.com>

On 10/30/2020 1:20 AM, Andrew Jones wrote:
> 
> You need to remove 'Message' from the summary.
> 
> On Tue, Oct 20, 2020 at 09:14:34PM +0800, Ying Fang wrote:
>> When building ACPI tables regarding CPUs we should always build
>> them for the number of possible CPUs, not the number of present
>> CPUs. We then ensure only the present CPUs are enabled.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> I guess my s-o-b is here because this is a rework of
> 
> https://github.com/rhdrjones/qemu/commit/b18d7a889f424b8a8679c43d7f4804fdeeeaf3fd

The s-o-b is given since this one is based on your branch.

> 
> I think it changed enough you could just drop my authorship. A
> based-on comment in the commit message would be more than enough.

Thanks. Will fix it. Hope it won't make you confused.

> 
> Comment on the patch below.
> 
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index a222981737..fae5a26741 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -57,14 +57,18 @@
>>   
>>   #define ARM_SPI_BASE 32
>>   
>> -static void acpi_dsdt_add_cpus(Aml *scope, int cpus)
>> +static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
>>   {
>>       uint16_t i;
>> +    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
>>   
>> -    for (i = 0; i < cpus; i++) {
>> +    for (i = 0; i < possible_cpus->len; i++) {
>>           Aml *dev = aml_device("C%.03X", i);
>>           aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>>           aml_append(dev, aml_name_decl("_UID", aml_int(i)));
>> +        if (possible_cpus->cpus[i].cpu == NULL) {
>> +            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
>> +        }
>>           aml_append(scope, dev);
>>       }
>>   }
>> @@ -470,6 +474,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       const int *irqmap = vms->irqmap;
>>       AcpiMadtGenericDistributor *gicd;
>>       AcpiMadtGenericMsiFrame *gic_msi;
>> +    int possible_cpus = MACHINE(vms)->possible_cpus->len;
>>       int i;
>>   
>>       acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>> @@ -480,7 +485,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>       gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
>>       gicd->version = vms->gic_version;
>>   
>> -    for (i = 0; i < MACHINE(vms)->smp.cpus; i++) {
>> +    for (i = 0; i < possible_cpus; i++) {
>>           AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
>>                                                              sizeof(*gicc));
>>           ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
>> @@ -495,7 +500,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>           gicc->cpu_interface_number = cpu_to_le32(i);
>>           gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
>>           gicc->uid = cpu_to_le32(i);
>> -        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>> +        if (i < MACHINE(vms)->smp.cpus) {
> 
> Shouldn't this be

Yes, Stupid mistake. Maybe it was lost when I am doing the rebase.
Will fix that. Thanks for your patience in the reply and review.

Ying Fang.
> 
>          if (possible_cpus->cpus[i].cpu != NULL) {
> 
>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
>> +        }
>>   
>>           if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
>>               gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
>> @@ -599,7 +606,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>        * the RTC ACPI device at all when using UEFI.
>>        */
>>       scope = aml_scope("\\_SB");
>> -    acpi_dsdt_add_cpus(scope, ms->smp.cpus);
>> +    acpi_dsdt_add_cpus(scope, vms);
>>       acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>                          (irqmap[VIRT_UART] + ARM_SPI_BASE));
>>       if (vmc->acpi_expose_flash) {
>> -- 
>> 2.23.0
>>
>>
> 
> Thanks,
> drew
> 
> .
> 