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

[Eduardo Habkost]

On Thu, Oct 10, 2019 at 11:06:29AM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 10, 2019 at 04:56:18PM +0200, Laszlo Ersek wrote:
> > On 10/09/19 15:22, 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
> > 
> > How about
> > 
> >     [0x0] Command data 2: (DWORD access, little endian)
> >           If the "CPU selector" value last stored by the guest refers to
> >           an impossible CPU, then 0.
> >           Otherwise, if the "Command field" value last stored by the
> >           guest differs from 3, then 0.
> >           Otherwise, the most significant 32 bits of the selected CPU's
> >           architecture specific ID.
> > 
> >     [0x8] Command data: (DWORD access, little endian)
> >           If the "CPU selector" value last stored by the guest refers to
> >           an impossible CPU, then 0.
> >           Otherwise,
> >           - if the "Command field" value last stored by the guest is 0,
> >             then the selector of the currently selected CPU;
> >           - if the "Command field" value last stored by the guest is 3,
> >             then the least significant 32 bits of the selected CPU's
> >             architecture specific ID;
> >           - otherwise, 0.
> > 
> > >      [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
> > >  
> > 
> > Seems OK.
> > 
> > >  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
> > 
> > Looks good, except for:
> > 
> > - typo: "int0"
> > 
> > - in step 3, we should reference 'Command data 2' as well.
> > 
> > 
> > In fact, in
> > <http://mid.mail-archive.com/address@hidden>,
> > I wrote, for the "Get a cpu with pending event" use case:
> > 
> > > 1. Store 0x0 to the 'CPU selector' register.
> > > 2. Store 0x0 to the 'Command field' register.
> > > 3. Read the 'CPU device status fields' register.
> > > 4. If both bit#1 and bit#2 are clear in the value read, there is no
> > >    CPU with a pending event.
> > > 5. Otherwise, read the 'Command data' register. The value read is the
> > >    selector of the CPU with the pending event (which is already
> > >    selected).
> > 
> > and your steps #2 and #3, for getting the arch specific ID, can be
> > directly appended as steps 6. and 7.!
> > 
> > 
> > > 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
> > >  
> > >  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"]"
> > > 
> > 
> > Looks plausible to me, thanks (discounting the TODO item).
> > 
> > Right now, I can't offer testing for patch#3 (I'm quite far from the
> > point where I'll be actually looking for a hotplugged CPU :) ), but
> > based on the docs patches #1 and #2, and my proposed updates, I can
> > rework my "possible CPU count detection" in OVMF.
> > 
> > Do I need to check in OVMF specifically whether the "modern" CPU hotplug
> > register block is available? Can you tell me what the oldest machine
> > types are that support the modern interface?
> > 
> > Hmm... Commit abd49bc2ed2f ("docs: update ACPI CPU hotplug spec with new
> > protocol", 2016-06-24) seems relevant. First released in v2.7.0. I think
> > I should detect whether this interface is available.
> > 
> > Can I use the following sequence to detect whether the interface is
> > available?
> > 
> > 1. Store 0x0 to command register.
> > 2. Store 0x0 to selector register.
> > 3. Read 'command data' register.
> > 4. If value read is 0, the interface is available.
> > 
> > (Because I assume that unmapped IO ports read as all-bits-one. Is that
> > right?)
> > 
> > BTW, can I dynamically detect support for the GET_CPU_ID command too?
> > I'm thinking, when I enumerate / count all possible CPUs, I can at once
> > fetch the arch IDs for all of them. If I only get zeros from the command
> > data registers, across all CPUs, in response to GET_CPU_ID, then the
> > command is not available.
> > 
> > Thanks
> > Laszlo
> 
> Laszlo, won't we need to add topology info anyway?
> if yes then this patch is just a stopgap, so let's do
> fw cfg and be done with it?

Topology info is already available on CPUID.

-- 
Eduardo