Re: [PATCH v3 23/35] acpi: x86: set enabled when composing _MAT entries

[Eric Auger]

On 9/7/21 4:48 PM, Igor Mammedov wrote:
> Instead of composing disabled _MAT entry and then later on
> patching it to enabled for hotpluggbale CPUs in DSDT,
> set it to enabled at the time _MAT entry is built.
> 
> It will allow to drop usage of packed structures in
> following patches when build_madt() is switched to use
> build_append_int_noprefix() API.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> CC: marcel.apfelbaum@gmail.com
> ---
>  include/hw/acpi/acpi_dev_interface.h |  3 ++-
>  include/hw/i386/pc.h                 |  6 +++---
>  hw/acpi/acpi-x86-stub.c              |  3 ++-
>  hw/acpi/cpu.c                        | 17 ++---------------
>  hw/i386/acpi-common.c                | 18 ++++++------------
>  5 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi_dev_interface.h 
> b/include/hw/acpi/acpi_dev_interface.h
> index 769ff55c7e..ea6056ab92 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -53,6 +53,7 @@ struct AcpiDeviceIfClass {
>      void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>      void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
>      void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
> -                     const CPUArchIdList *apic_ids, GArray *entry);
> +                     const CPUArchIdList *apic_ids, GArray *entry,
> +                     bool force_enabled);
>  };
>  #endif
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 4d2e35a152..82cf7b7e30 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -187,10 +187,10 @@ bool pc_system_ovmf_table_find(const char *entry, 
> uint8_t **data,
>                                 int *data_len);
>  void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
>  
> -
> -/* acpi-build.c */
> +/* hw/i386/acpi-common.c */
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> -                       const CPUArchIdList *apic_ids, GArray *entry);
> +                       const CPUArchIdList *apic_ids, GArray *entry,
> +                       bool force_enabled);
>  
>  extern GlobalProperty pc_compat_6_1[];
>  extern const size_t pc_compat_6_1_len;
> diff --git a/hw/acpi/acpi-x86-stub.c b/hw/acpi/acpi-x86-stub.c
> index e9e46c5c5f..3df1e090f4 100644
> --- a/hw/acpi/acpi-x86-stub.c
> +++ b/hw/acpi/acpi-x86-stub.c
> @@ -3,7 +3,8 @@
>  #include "hw/i386/acpi-build.h"
>  
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> -                       const CPUArchIdList *apic_ids, GArray *entry)
> +                       const CPUArchIdList *apic_ids, GArray *entry,
> +                       bool force_enabled)
>  {
>  }
>  
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index f82e9512fd..b20903ea30 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -669,21 +669,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, 
> CPUHotplugFeatures opts,
>  
>              /* build _MAT object */
>              assert(adevc && adevc->madt_cpu);
> -            adevc->madt_cpu(adev, i, arch_ids, madt_buf);
> -            switch (madt_buf->data[0]) {
> -            case ACPI_APIC_PROCESSOR: {
> -                AcpiMadtProcessorApic *apic = (void *)madt_buf->data;
> -                apic->flags = cpu_to_le32(1);
> -                break;
> -            }
> -            case ACPI_APIC_LOCAL_X2APIC: {
> -                AcpiMadtProcessorX2Apic *apic = (void *)madt_buf->data;
> -                apic->flags = cpu_to_le32(1);
> -                break;
> -            }
> -            default:
> -                assert(0);
> -            }
> +            adevc->madt_cpu(adev, i, arch_ids, madt_buf,
> +                            true); /* set enabled flag */
>              aml_append(dev, aml_name_decl("_MAT",
>                  aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
>              g_array_free(madt_buf, true);
> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> index a0cde1d874..7983a13a93 100644
> --- a/hw/i386/acpi-common.c
> +++ b/hw/i386/acpi-common.c
> @@ -34,9 +34,11 @@
>  #include "acpi-common.h"
>  
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> -                       const CPUArchIdList *apic_ids, GArray *entry)
> +                       const CPUArchIdList *apic_ids, GArray *entry,
> +                       bool force_enabled)
>  {
>      uint32_t apic_id = apic_ids->cpus[uid].arch_id;
> +    uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ? 1 : 
> 0;
>  
>      /* ACPI spec says that LAPIC entry for non present
>       * CPU may be omitted from MADT or it must be marked
> @@ -51,11 +53,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>          apic->length = sizeof(*apic);
>          apic->processor_id = uid;
>          apic->local_apic_id = apic_id;
> -        if (apic_ids->cpus[uid].cpu != NULL) {
> -            apic->flags = cpu_to_le32(1);
> -        } else {
> -            apic->flags = cpu_to_le32(0);
> -        }
> +        apic->flags = cpu_to_le32(flags);
>      } else {
>          AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic);
>  
> @@ -63,11 +61,7 @@ void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>          apic->length = sizeof(*apic);
>          apic->uid = cpu_to_le32(uid);
>          apic->x2apic_id = cpu_to_le32(apic_id);
> -        if (apic_ids->cpus[uid].cpu != NULL) {
> -            apic->flags = cpu_to_le32(1);
> -        } else {
> -            apic->flags = cpu_to_le32(0);
> -        }
> +        apic->flags = cpu_to_le32(flags);
>      }
>  }
>  
> @@ -96,7 +90,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>      build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags 
> */
>  
>      for (i = 0; i < apic_ids->len; i++) {
> -        adevc->madt_cpu(adev, i, apic_ids, table_data);
> +        adevc->madt_cpu(adev, i, apic_ids, table_data, false);
>          if (apic_ids->cpus[i].arch_id > 254) {
>              x2apic_mode = true;
>          }
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric