Re: [Qemu-devel] [PATCH v3] i386: Add _PXM ACPI method to CPU objects

[Igor Mammedov]

On Wed, 27 Nov 2013 19:38:44 +0100
Vasilis Liaskovitis <address@hidden> wrote:

> This patch adds a _PXM method to ACPI CPU objects for the pc machine. The _PXM
> value is derived from the passed in guest info, same way as CPU SRAT entries.
> 
> Currently, CPU SRAT entries are only enabled for cpus that are already present
> in the system. The SRAT entries for hotpluggable processors are disabled 
> (flags
> bit 0 set to 0 in hw/i385/acpi-build.c:build_srat). Section 5.2.16.1 of ACPI
> spec mentions "If the Local APIC ID of a dynamically added processor is not
> present in the SRAT, a _PXM object must exist for the processor’s device or 
> one
> of its ancestors in the ACPI Namespace." Since SRAT entries are not available
> for the hot-pluggable processors, a _PXM method must exist for them. 
> Otherwise,
> the CPU is hot-added in the wrong NUMA node (default node 0).
> 
> Even if CPU SRAT entries are enabled, _PXM method is what the linux kernel
> consults on hot-add time. Section 17.2.1 of ACPI spec mentions " OSPM will
> consume the SRAT only at boot time. OSPM should use _PXM for any devices that
> are hot-added into the system after boot up." To be more precise if SRAT
> information is available to the guest kernel, it is used.  However, parsed 
> SRAT
> info is reset and lost after hot-remove operations, see kernel commit 
> c4c60524.
> This means that on a hot-unplug / hot-replug scenario, and without a _PXM
> method, the kernel may put a CPU on different nodes because SRAT info has been
> reset by a previous hot-remove operation.
> 
> The above hot-remove/hot-add scenario has been tested on master, plus cpu-del
> patches from:
> https://lists.gnu.org/archive/html/qemu-devel/2013-10/msg01085.html
> With the curret _PXM patch, hot-added CPUs are always placed into the correct
> NUMA node, regardless of kernel behaviour.
> 
> v1->v2:
>    Make method return a DWORD integer
>    Tested on qemu master + cpu-del patches
> v2->v3:
>    Add changed hw/i386/sdt-proc.hex.generated file
>    Change PXM constant name to CPXM
> 
> Signed-off-by: Vasilis Liaskovitis <address@hidden>
> Reviewed-by: Thilo Fromm <address@hidden>
> 
> ---
>  hw/i386/acpi-build.c            |    5 ++++
>  hw/i386/ssdt-proc.dsl           |    5 ++++
>  hw/i386/ssdt-proc.hex.generated |   57 
> ++++++++++++++++++++++++++++-----------
>  3 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b48c930..387a869 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -605,6 +605,7 @@ static inline char acpi_get_hex(uint32_t val)
>  #define ACPI_PROC_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2)
>  #define ACPI_PROC_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4)
>  #define ACPI_PROC_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start)
> +#define ACPI_PROC_OFFSET_CPUPXM (*ssdt_proc_pxm - *ssdt_proc_start)
>  #define ACPI_PROC_SIZEOF (*ssdt_proc_end - *ssdt_proc_start)
>  #define ACPI_PROC_AML (ssdp_proc_aml + *ssdt_proc_start)
>  
> @@ -726,6 +727,10 @@ build_ssdt(GArray *table_data, GArray *linker,
>              proc[ACPI_PROC_OFFSET_CPUHEX+1] = acpi_get_hex(i);
>              proc[ACPI_PROC_OFFSET_CPUID1] = i;
>              proc[ACPI_PROC_OFFSET_CPUID2] = i;
> +            proc[ACPI_PROC_OFFSET_CPUPXM] = guest_info->node_cpu[i];
> +            proc[ACPI_PROC_OFFSET_CPUPXM + 1] = 0;
> +            proc[ACPI_PROC_OFFSET_CPUPXM + 2] = 0;
> +            proc[ACPI_PROC_OFFSET_CPUPXM + 3] = 0;
>          }
>  
>          /* build this code:
> diff --git a/hw/i386/ssdt-proc.dsl b/hw/i386/ssdt-proc.dsl
> index 8229bfd..52b44e3 100644
> --- a/hw/i386/ssdt-proc.dsl
> +++ b/hw/i386/ssdt-proc.dsl
> @@ -47,6 +47,8 @@ DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", 
> "BXSSDT", 0x1)
>   * also updating the C code.
>   */
>          Name(_HID, "ACPI0007")
> +        ACPI_EXTRACT_NAME_DWORD_CONST ssdt_proc_pxm
> +        Name(CPXM, 0xAAAAAAAA)
>          External(CPMA, MethodObj)
>          External(CPST, MethodObj)
>          External(CPEJ, MethodObj)
> @@ -59,5 +61,8 @@ DefinitionBlock ("ssdt-proc.aml", "SSDT", 0x01, "BXPC", 
> "BXSSDT", 0x1)
>          Method(_EJ0, 1, NotSerialized) {
>              CPEJ(ID, Arg0)
>          }
> +        Method(_PXM, 0) {
> +            Return (CPXM)
> +        }
>      }
>  }
> diff --git a/hw/i386/ssdt-proc.hex.generated b/hw/i386/ssdt-proc.hex.generated
> index bb9920d..8497866 100644
> --- a/hw/i386/ssdt-proc.hex.generated
> +++ b/hw/i386/ssdt-proc.hex.generated
> @@ -1,17 +1,26 @@
> +static unsigned char ssdt_proc_end[] = {
> +0x8e
> +};
>  static unsigned char ssdt_proc_name[] = {
>  0x28
>  };
> +static unsigned char ssdt_proc_pxm[] = {
> +0x4e
> +};
> +static unsigned char ssdt_proc_id[] = {
> +0x38
> +};
>  static unsigned char ssdp_proc_aml[] = {
>  0x53,
>  0x53,
>  0x44,
>  0x54,
> -0x78,
> +0x8e,
>  0x0,
>  0x0,
>  0x0,
>  0x1,
> -0xb8,
> +0x19,
>  0x42,
>  0x58,
>  0x50,
> @@ -34,21 +43,21 @@ static unsigned char ssdp_proc_aml[] = {
>  0x4e,
>  0x54,
>  0x4c,
> -0x23,
> -0x8,
> -0x13,
> +0x28,
> +0x5,
> +0x10,
>  0x20,
>  0x5b,
>  0x83,
> -0x42,
> -0x5,
> +0x48,
> +0x6,
>  0x43,
>  0x50,
>  0x41,
>  0x41,
>  0xaa,
> -0x10,
> -0xb0,
> +0x0,
> +0x0,
>  0x0,
>  0x0,
>  0x0,
> @@ -74,6 +83,16 @@ static unsigned char ssdp_proc_aml[] = {
>  0x30,
>  0x37,
>  0x0,
> +0x8,
> +0x43,
> +0x50,
> +0x58,
> +0x4d,
> +0xc,
> +0xaa,
> +0xaa,
> +0xaa,
> +0xaa,
>  0x14,
>  0xf,
>  0x5f,
> @@ -121,13 +140,19 @@ static unsigned char ssdp_proc_aml[] = {
>  0x44,
>  0x5f,
>  0x5f,
> -0x68
> -};
> -static unsigned char ssdt_proc_id[] = {
> -0x38
> -};
> -static unsigned char ssdt_proc_end[] = {
> -0x78
> +0x68,
> +0x14,
> +0xb,
> +0x5f,
> +0x50,
> +0x58,
> +0x4d,
> +0x0,
> +0xa4,
> +0x43,
> +0x50,
> +0x58,
> +0x4d
>  };
>  static unsigned char ssdt_proc_start[] = {
>  0x24

Reviewed-By: Igor Mammedov <address@hidden>