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

2020-11-02 Thread Ying Fang 




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 


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

.

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

2020-11-01 Thread Ying Fang 




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 


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

.

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

2020-10-29 Thread Andrew Jones 


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 

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

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

2020-10-20 Thread Ying Fang 
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 
Signed-off-by: Ying Fang 
---
 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) {
+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