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

[Laszlo Ersek]

On 12/06/19 16:15, Igor Mammedov wrote:
> On Thu, 5 Dec 2019 14:03:01 +0100
> Laszlo Ersek <address@hidden> wrote:
> 
>> 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.
> 
> How about following commit message:
> 
>     Firmware can enumerate present at boot APs by broadcasting wakeup IPI,
>     so that woken up secondary CPUs could register them-selves.
>     However in CPU hotplug case, it would need to know architecture
>     specific CPU IDs for possible and hotplugged CPUs so it could
>     prepare enviroment for and wake hotplugged AP.
>     
>     Reuse and extend existing CPU hotplug interface to return architecture
>     specific ID for currently selected CPU in 2 registers:
>      - lower 32 bits in ACPI_CPU_CMD_DATA_OFFSET_RW
>      - upper 32 bits in ACPI_CPU_CMD_DATA2_OFFSET_R
>     
>     On x86, firmware will use CPHP_GET_CPU_ID_CMD for fetching the APIC ID
>     when handling hotplug SMI.
>     
>     Later, CPHP_GET_CPU_ID_CMD will be used on ARM to retrieve MPIDR,
>     which serves the similar to APIC ID purpose.
> 
> [...]
> 

Looks fine to me, thank you!
Laszlo