Re: [PATCH for-5.0 8/8] acpi: cpuhp: spec: document procedure for enabling modern CPU hotplug

[Igor Mammedov]

On Thu, 5 Dec 2019 15:07:53 +0100
Laszlo Ersek <address@hidden> wrote:

> On 12/04/19 18:05, Igor Mammedov wrote:
> > Describe how to enable and detect modern CPU hotplug interface.
> > Detection part is based on new CPHP_GET_CPU_ID_CMD command,
> > introduced by "acpi: cpuhp: add CPHP_GET_CPU_ID_CMD command" patch.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  docs/specs/acpi_cpu_hotplug.txt | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)  
> 
> Could we make this usecase / workflow independent of the new
> CPHP_GET_CPU_ID_CMD command please?
> 
> I'd like to suggest the following:
> 
> > diff --git a/docs/specs/acpi_cpu_hotplug.txt 
> > b/docs/specs/acpi_cpu_hotplug.txt
> > index bb33144..667b264 100644
> > --- a/docs/specs/acpi_cpu_hotplug.txt
> > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > @@ -15,14 +15,14 @@ CPU present bitmap for:
> >    PIIX-PM  (IO port 0xaf00-0xaf1f, 1-byte access)
> >    One bit per CPU. Bit position reflects corresponding CPU APIC ID. 
> > Read-only.
> >    The first DWORD in bitmap is used in write mode to switch from legacy
> > -  to new CPU hotplug interface, write 0 into it to do switch.
> > +  to modern CPU hotplug interface, write 0 into it to do switch.
> >  ---------------------------------------------------------------
> >  QEMU sets corresponding CPU bit on hot-add event and issues SCI
> >  with GPE.2 event set. CPU present map is read by ACPI BIOS GPE.2 handler
> >  to notify OS about CPU hot-add events. CPU hot-remove isn't supported.
> >
> >  =====================================
> > -ACPI CPU hotplug interface registers:
> > +Modern ACPI CPU hotplug interface registers:
> >  -------------------------------------
> >  Register block base address:
> >      ICH9-LPC IO port 0x0cd8
> > @@ -105,6 +105,24 @@ write access:
> >                other values: reserved
> >
> >      Typical usecases:
> > +        - (x86) Detecting and enabling modern CPU hotplug interface.  
> 
> (1) I think we can drop the (x86) restriction. (Because, we don't need
> to depend on APIC ID specifics; see below.)
> 
> > +          QEMU starts with legacy CPU hotplug interface enabled. Detecting 
> > and
> > +          switching to modern interface is based on the 2 legacy CPU 
> > hotplug features:
> > +            1. Writes into CPU bitmap are ignored.
> > +            2. CPU bitmap always has bit#0 set, corresponding to boot CPU.
> > +
> > +          Use following steps to detect and enable modern CPU hotplug 
> > interface:
> > +            1. Store 0x0 to the 'CPU selector' register,
> > +               attempting to switch to modern mode
> > +            2. Store 0x0 to the 'CPU selector' register,
> > +               to ensure valid selector value  
> 
> OK thus far.
> 
> > +            3. Store 0x3 to the 'Command field' register,
> > +               sets the 'Command data 2' register into architecture 
> > specific
> > +               CPU identifier mode  
> 
> (2) Can we please store command 0 here
> (CPHP_GET_NEXT_CPU_WITH_EVENT_CMD)?

that could work too, as far "Command data 2" is defined before
we use it here.
Point is to define "Command data 2" state with command 0 and leave our hands
free when it comes to reserved (so it won't get in a way in the future
if we need to 'unreserve' it in some context)

> 
> That might change the selector value, yes. (Even if that happens, the
> new selector will be *valid*.)
> 
> But the main point is that, with 0 stored to the command register, one
> of the following *four* cases will hold, subsequently:
> 
> (2a) if register block is totally missing:
> 
> --> Offset#0 will read as all-bits-one (unassigned read)  
> 
> (2b) if register block is legacy-only:
> 
> --> Offset#0 will read as nonzero, due to CPU#0 being always present  
> 
> (2c) if the modern register block is active, but the "Command data 2"
> register is *not* yet described in the spec file:
> 
> --> Offset#0 will read as zero, because it is *reserved*:  
> 
> > read access:
> >     offset:
> >     [0x0-0x3] reserved <---- HERE  
> 
> (2d) if the modern register block is active, and the "Command data 2"
> register *is* described in the spec file:
> 
> --> the "Command data 2" register (at offset#0) will read as zero,  
> because:
> 
> > read access:
> >     offset:
> >     [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 <------ HERE  
> 
> and then step#4 applies just the same:
> 
> On 12/04/19 18:05, Igor Mammedov wrote:
> > +            4. Read the 'Command data 2' register.
> > +               If read value is 0x0, the modern interface is enabled.
> > +               Otherwise legacy or no CPU hotplug interface available
> > +  
> 
> because "read value is 0x0" corresponds to the *union* of cases (2c) and
> (2d) -- namely, "the modern register block is active".
> 
> My proposal above is what I implemented for OVMF in October:
> 
>   [edk2-devel] [PATCH v2 3/3]
>   OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
> 
>   http://mid.mail-archive.com/address@hidden
> 
> and it works very well.
> 
> So the benefits would be:
> 
> - I wouldn't have to rewrite that (complex!) patch :) ,
> 
> - the logic would not store the new CPHP_GET_CPU_ID_CMD command, only
>   read offset#0,
> 
> - the logic would not be x86 specific (it would not have to rely on the
>   most significant 32 bits of the 64-bit arch-specific CPU identifier --
>   here: the APIC ID -- being zero).
> 
> Furthermore,
> 
> (3) In step#4, I suggest replacing 'Command data 2' with "offset 0",
Spec uses field names so I'd rather use 'Command data 2' here instead of
direct offset to be consistent with the rest of the spec.

 
> (4) finally, I'd suggest squashing this patch (updated as requested
> above) into patch "acpi: cpuhp: spec: add typical usecases".
> 
> Using my suggestion (2), you can define the "modern interface
> enablement" workflow as well, without any dependency on
> CPHP_GET_CPU_ID_CMD. The only thing that's necessary is the small update
> from (3), and then you can describe all three important use cases in one
> go, in patch#6.

I'd add extra patch that defines 'Command data 2' for command 0
and after that it should be possible to squash detection usecase
into 6/8.

> And then you can introduce CPHP_GET_CPU_ID_CMD in the last patch
> (patch#7), while staying compatible with the previously-documented
> workflows.
> 
> Pretty please? :)
> 
> Thanks!
> Laszlo
> 
> >          - Get a cpu with pending event
> >            1. Store 0x0 to the 'CPU selector' register.
> >            2. Store 0x0 to the 'Command field' register.
> >