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

[Laszlo Ersek <lersek@redhat.com>]

On 10/22/19 16:42, Igor Mammedov wrote:
> On Tue, 22 Oct 2019 14:39:24 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/21/19 15:06, Laszlo Ersek wrote:
>>> On 10/18/19 18:18, Igor Mammedov wrote:  

>>>> Considering firmware runs the first, it should enable modern interface
>>>> on its own
>>>>   1. Store 0x0 to selector register (actually it's store into bitmap to attempt switch). 
>>>> and to check if interface is present
>>>>   2. Store 0x0 to selector register (to ensure valid selector value (otherwise command is ignored))
>>>>   3. Store 0x0 to command register (to be able to read back selector from command data)
>>>>   4. Store 0x0 to selector register (because #3 can select the a cpu with events if any)
>>>>       be aware libvirt may start QEMU in paused mode (hotplug context) and hotplugs extra CPUs
>>>>       with device_add and then let guest run. So firmware may see present CPUs with events
>>>>       at boot time.
>>>>   5. Read 'command data' register.
>>>>   6. If value read is 0, the interface is available.  

>> When we read the command data register in the last step, that is at
>> offset 0x8 in the register block. Considering the legacy "CPU present
>> bitmap", if no CPU is present in that range, then the firmware could
>> read a zero value. I got confused because I thought we were reading at
>> offset 0, which would always have bit0 set (for CPU#0).
>>
>> Can we detect the modern interface like this:
>>
>> 1. store 0x0 to selector register (attempt to switch)
>> 2. read one byte at offset 0 in the register block
>> 3. if bit#0 is set, the modern interface is unavailable;
>>    otherwise (= bit#0 clear), the modern interface is available
>>
>> Here's why:
>>
>> - if even the legacy interface is missing, then step 2 is an unassigned
>>   read, hence the value read is all-bits-one; bit#0 is set
>>
>> - if only the legacy interface is available, then bit#0 stands for
>>   CPU#0, it will be set
>>
>> - if the switch-over in step 1 is successful, then offset 0 is reserved,
>>   hence it returns all-bits-zero.
>>
>> With this, if we ever assigned offset 0 for reading, then we'd have to
>> define it with bit#0 constantly clear.
> 
> There is no need to reserve bit#0 if in step #5 we use s/'command data'/'Command data 2'/

Good idea. We can drop step 4 too:

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

This is skipped by step 2.

          Otherwise, if the "Command field" value last stored by the
          guest differs from 3, then 0.

This is triggered by step 3.

So step 4 does not look necessary. (As long as the guest is OK with the
selector ending up with a changed value.)

          Otherwise, the most significant 32 bits of the selected CPU's
          architecture specific ID.

Not relevant for this use case.

> Alternatively we can reserve bit#0 and sequentially read upper half from 'Command data'
> (one a new flag to show that there is more data to read).

I like the "Command data 2" register more. The "temporal domain" is
always a complication in register definitions.

> (Upper half currently is not necessary, it's there for future ARM's MPIDR).
> 
> One more thing, this behavior is based on artifacts of x86 machine and AllOnes fallback.
> Obviously it won't work with arm/virt, do we care about AVMF at this point?

No, in the firmware, all this is strictly x86 code. The ArmVirtQemu
guest firmware has no support for multiprocessing at this time, to my
understanding.

(Nonetheless, if the register block got placed at an MMIO base address
on arm/virt, I think "unassigned_mem_ops" would apply there just the same.)

Thanks!
Laszlo