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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 22 Oct 2019 17:49:06 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

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

sounds good,
I'll respin patches taking this into account.

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