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

[Laszlo Ersek]

On 12/06/19 11:40, Igor Mammedov wrote:
> 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.

Sounds good!

Thanks!
Laszlo

> 
>> 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.
>>>  
>