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

[Igor Mammedov <imammedo@redhat.com>]

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:  
> >> On Thu, 10 Oct 2019 16:56:18 +0200
> >> Laszlo Ersek <lersek@redhat.com> wrote:  
> 
> [...]
> 
> >>> 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.  
> >>
> >> By default legacy register block layout is in place
> >> (i.e. present cpus bitmap) where 1st byte is guarantied to be ">0" as it has
> >> at least the boot CPU bit set and writes to legacy bitmap are ignored.
> >>
> >> Currently AML code code does switching to modern interface, see
> >> docs/specs/acpi_cpu_hotplug.txt:
> >> "
> >>   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.
> >> "
> >> related code "if (opts.has_legacy_cphp) {" and cpu_status_write()
> >>
> >> 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.  
> > 
> > Perfect!
> > 
> > Basically this is prepending two "write 0 to selector register" steps to
> > what I suspected. I certainly couldn't figure out the "switch to modern"
> > step, and whether initializing the selector to something valid was
> > needed at boot. Now I know. :) Thanks!
> >   
> >>  
> >>> (Because I assume that unmapped IO ports read as all-bits-one. Is that
> >>> right?)  
> >> that's right but ports are mapped to legacy CPU bitmap, you can't count on all-bits-one case here.  
> 
> It seems I rejoiced too soon.
> 
> 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'/

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).
(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?

> Thanks,
> Laszlo