Re: [RFC 3/3] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command

[Philippe Mathieu-Daudé]

Hi Igor,

On 10/9/19 3:22 PM, Igor Mammedov wrote:
> Extend CPU hotplug interface to return architecture specific
> identifier for current CPU (in case of x86, it's APIC ID).
>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> TODO:
>    * cripple it to behave old way on old machine types so that
>      new firmware started on new QEMU won't see a difference
>      when migrated to an old QEMU (i.e. QEMU that doesn't support
>      this command)
> ---
>   docs/specs/acpi_cpu_hotplug.txt | 10 +++++++++-
>   hw/acpi/cpu.c                   | 15 +++++++++++++++
>   hw/acpi/trace-events            |  1 +
>   3 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 43c5a193f0..0438678249 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -32,7 +32,9 @@ Register block size:
>   
>   read access:
>       offset:
> -    [0x0-0x3] reserved
> +    [0x0-0x3] Command data 2: (DWORD access)
> +              upper 32 bit of 'Command data' if returned data value is 64 bit.
> +              in case of error or unsupported command reads is 0x0
>       [0x4] CPU device status fields: (1 byte access)
>           bits:
>              0: Device is enabled and may be used by guest
> @@ -87,6 +89,8 @@ write access:
>                 2: stores value into OST status register, triggers
>                    ACPI_DEVICE_OST QMP event from QEMU to external applications
>                    with current values of OST event and status registers.
> +              3: OSPM reads architecture specific value identifying CPU
> +                 (x86: APIC ID)
>               other values: reserved
>   
>   Selecting CPU device beyond possible range has no effect on platform:
> @@ -115,3 +119,7 @@ Typical usecases:
>        5.2 if 'Command data' register has not changed, there is not CPU
>            corresponding to iterator value and the last valid iterator value
>            equals to 'max_cpus' + 1
> +   - Get architecture specific id for a CPU
> +     1. pick a CPU to read from using 'CPU selector' register
> +     2. write 0x3 int0 'Command field' register
> +     3. read architecture specific id from 'Command data' register
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 87f30a31d7..701542d860 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -12,11 +12,13 @@
>   #define ACPI_CPU_FLAGS_OFFSET_RW 4
>   #define ACPI_CPU_CMD_OFFSET_WR 5
>   #define ACPI_CPU_CMD_DATA_OFFSET_RW 8
> +#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0

This part confuses me:

#define ACPI_CPU_SELECTOR_OFFSET_WR 0
#define ACPI_CPU_CMD_DATA2_OFFSET_RW 0

What about:

#define ACPI_CPU_SELECTOR_OFFSET_W 0
#define ACPI_CPU_CMD_DATA2_OFFSET_R 0

>   
>   enum {
>       CPHP_GET_NEXT_CPU_WITH_EVENT_CMD = 0,
>       CPHP_OST_EVENT_CMD = 1,
>       CPHP_OST_STATUS_CMD = 2,
> +    CPHP_GET_CPU_ID_CMD = 3,
>       CPHP_CMD_MAX
>   };
>   
> @@ -74,11 +76,24 @@ static uint64_t cpu_hotplug_rd(void *opaque, hwaddr addr, unsigned size)
>           case CPHP_GET_NEXT_CPU_WITH_EVENT_CMD:
>              val = cpu_st->selector;
>              break;
> +        case CPHP_GET_CPU_ID_CMD:
> +           val = cpu_to_le64(cdev->arch_id) & 0xFFFFFFFF;
> +           break;
>           default:
>              break;
>           }
>           trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val);
>           break;
> +    case ACPI_CPU_CMD_DATA2_OFFSET_RW:
> +        switch (cpu_st->command) {
> +        case CPHP_GET_CPU_ID_CMD:
> +           val = cpu_to_le64(cdev->arch_id) >> 32;
> +           break;
> +        default:
> +           break;
> +        }
> +        trace_cpuhp_acpi_read_cmd_data2(cpu_st->selector, val);
> +        break;
>       default:
>           break;
>       }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index 96b8273297..afbc77de1c 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -23,6 +23,7 @@ cpuhp_acpi_read_flags(uint32_t idx, uint8_t flags) 
> "idx[0x%"PRIx32"] flags: 0x%"
>   cpuhp_acpi_write_idx(uint32_t idx) "set active cpu idx: 0x%"PRIx32
>   cpuhp_acpi_write_cmd(uint32_t idx, uint8_t cmd) "idx[0x%"PRIx32"] cmd: 
> 0x%"PRIx8
>   cpuhp_acpi_read_cmd_data(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 
> 0x%"PRIx32
> +cpuhp_acpi_read_cmd_data2(uint32_t idx, uint32_t data) "idx[0x%"PRIx32"] data: 
> 0x%"PRIx32
>   cpuhp_acpi_cpu_has_events(uint32_t idx, bool ins, bool rm) "idx[0x%"PRIx32"] 
> inserting: %d, removing: %d"
>   cpuhp_acpi_clear_inserting_evt(uint32_t idx) "idx[0x%"PRIx32"]"
>   cpuhp_acpi_clear_remove_evt(uint32_t idx) "idx[0x%"PRIx32"]"