Re: [PATCH for-5.0 7/8] acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command

[Laszlo Ersek <lersek@redhat.com>]

On 12/04/19 18:05, Igor Mammedov wrote:
> Extend CPU hotplug interface to return architecture specific
> identifier for current CPU in 2 registers:
>  - lower 32 bits existing ACPI_CPU_CMD_DATA_OFFSET_RW
>  - upper 32 bits in new ACPI_CPU_CMD_DATA2_OFFSET_R at
>    offset 0.

OK.

> Target user is UEFI firmware, which needs a way to enumerate
> all CPUs (including possible CPUs) to allocate and initialize
> CPU structures on boot.

(1) This is correct in general, but if we want to keep this description,
then it should be moved to the commit message of the previous patch.
CPHP_GET_CPU_ID_CMD is not needed for the purpose described above -- it
will be necessary for handling the hotplug SMI.

For the boot time allocation / initialization, the "enumerating present
and possible CPUs" workflow is necessary, and that is documented in the
previous patch in this series.

So if we want to keep this paragraph, we should move it to the previous
patch's commit message.

> (for x86: it needs APIC ID and later command will be used to
> retrieve ARM's MPIDR which serves the similar to APIC ID purpose)

(2) I would suggest some punctuation, to make this clearer. How about:

> On x86, guest UEFI firmware will use CPHP_GET_CPU_ID_CMD for fetching
> the APIC ID when handling the hotplug SMI.
>
> Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
> which serves a purpose similar to the x86 APIC ID.

Back to the patch:

On 12/04/19 18:05, Igor Mammedov wrote:
>
> The new ACPI_CPU_CMD_DATA2_OFFSET_R register will also be used
> to detect presence of modern CPU hotplug, which will be described
> in follow up patch.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v1:
>  - s/ACPI_CPU_CMD_DATA2_OFFSET_RW/ACPI_CPU_CMD_DATA2_OFFSET_R/.
> ---
>  docs/specs/acpi_cpu_hotplug.txt | 10 ++++++++--
>  hw/acpi/cpu.c                   | 15 +++++++++++++++
>  hw/acpi/trace-events            |  1 +
>  3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/docs/specs/acpi_cpu_hotplug.txt b/docs/specs/acpi_cpu_hotplug.txt
> index 58c16c6..bb33144 100644
> --- a/docs/specs/acpi_cpu_hotplug.txt
> +++ b/docs/specs/acpi_cpu_hotplug.txt
> @@ -44,7 +44,11 @@ keeps the current value.
>
>  read access:
>      offset:
> -    [0x0-0x3] reserved
> +    [0x0-0x3] Command data 2: (DWORD access)
> +              if last stored 'Command field' value:
> +                  3: upper 32 bits of architecture specific identifying CPU value
> +                     (n x86 case: 0x0)
> +                  other values: reserved
>      [0x4] CPU device status fields: (1 byte access)
>          bits:
>             0: Device is enabled and may be used by guest

OK.

> @@ -96,7 +100,9 @@ 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.
> -            other values: reserved
> +              3: lower 32 bit of architecture specific identifier
> +                 (in x86 case: APIC ID)
> +              other values: reserved
>
>      Typical usecases:
>          - Get a cpu with pending event

(3) This fragment has been pasted in the wrong spot. We should add this
under:

> read access:
> ...
>     [0x8] Command data: (DWORD access)
>           contains 0 unless last stored in 'Command field' value is one of:
>               0: contains 'CPU selector' value of a CPU with pending event[s]

But right now, the addition is in the context of:

> write access:
> ...
>     [0x8] Command data: (DWORD access)
>           if last stored 'Command field' value:
>               1: stores value into OST event register
>               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: lower 32 bit of architecture specific identifier
>                  (in x86 case: APIC ID)
>               other values: reserved

and it does not make sense there.

(4) Furthermore, for consistency, please write "32 bits" (plural).

Back to the patch:

On 12/04/19 18:05, Igor Mammedov wrote:
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 87f30a3..87813ce 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_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 = cdev->arch_id & 0xFFFFFFFF;
> +           break;
>          default:
>             break;
>          }
>          trace_cpuhp_acpi_read_cmd_data(cpu_st->selector, val);
>          break;
> +    case ACPI_CPU_CMD_DATA2_OFFSET_R:
> +        switch (cpu_st->command) {
> +        case CPHP_GET_CPU_ID_CMD:
> +           val = 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 96b8273..afbc77d 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"]"
>

The code looks good to me.

Thanks!
Laszlo