diff mbox series

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

Message ID 20201020131440.1090-8-fangying1@huawei.com
State Superseded
Headers show
Series hw/arm/virt: Introduce cpu and cache topology support | expand

Commit Message

Ying Fang Oct. 20, 2020, 1:14 p.m. UTC 
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>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt-acpi-build.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Andrew Jones Oct. 29, 2020, 5:20 p.m. UTC | #1 
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

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

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

        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
Ying Fang Nov. 2, 2020, 3:13 a.m. UTC | #2 
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

> 

> .

>
Ying Fang Nov. 3, 2020, 2:46 a.m. UTC | #3 
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

> 

> I think it changed enough you could just drop my authorship. A

> based-on comment in the commit message would be more than enough.

> 

> 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

> 

>          if (possible_cpus->cpus[i].cpu != NULL) {

> 

>> +            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);

>> +        }


I now realized that I switched to use current cpu number as the limit to
make GIC flags enabled here.
However to judge NULL is much more suitable here.

Thanks,
Ying.

>>   

>>           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

> 

> .

>
diff mbox series

Patch

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) {
+            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) {