Re: [Qemu-devel] [PATCH 15/33] docs: update ACPI CPU hotplug spec with new protocol

[Michael S. Tsirkin]

On Tue, May 31, 2016 at 05:07:41PM +0200, Igor Mammedov wrote:
> On Tue, 31 May 2016 07:49:16 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Tue, May 17, 2016 at 04:43:07PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > >  docs/specs/acpi_cpu_hotplug.txt | 88 
> > > +++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 76 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/docs/specs/acpi_cpu_hotplug.txt 
> > > b/docs/specs/acpi_cpu_hotplug.txt
> > > index 340b751..c5bce6a 100644
> > > --- a/docs/specs/acpi_cpu_hotplug.txt
> > > +++ b/docs/specs/acpi_cpu_hotplug.txt
> > > @@ -4,21 +4,85 @@ QEMU<->ACPI BIOS CPU hotplug interface
> > >  QEMU supports CPU hotplug via ACPI. This document
> > >  describes the interface between QEMU and the ACPI BIOS.
> > >  
> > > -ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
> > > ------------------------------------------
> > > -
> > > -Generic ACPI GPE block. Bit 2 (GPE.2) used to notify CPU
> > > -hot-add/remove event to ACPI BIOS, via SCI interrupt.
> > > +ACPI BIOS GPE.2 handler is dedicated for notifying OS about CPU hot-add
> > > +and hot-remove events.
> > >  
> > > +============================================
> > > +Legacy ACPI CPU hotplug interface registers:
> > > +--------------------------------------------
> > >  CPU present bitmap for:
> > > +  One bit per CPU. Bit position reflects corresponding CPU APIC ID. 
> > > Read-only.
> > >    ICH9-LPC (IO port 0x0cd8-0xcf7, 1-byte access)
> > >    PIIX-PM  (IO port 0xaf00-0xaf1f, 1-byte access)
> > >  ---------------------------------------------------------------
> > > -One bit per CPU. Bit position reflects corresponding CPU APIC ID.
> > > -Read-only.
> > > +QEMU sets corresponding CPU bit on hot-add event and issues SCI
> > > +with GPE.2 event set. CPU present map 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:
> > > +-------------------------------------
> > > +Register block base address:
> > > +    ICH9-LPC IO port 0x0cd8
> > > +    PIIX-PM  IO port 0xaf00  
> > 
> > OK but this means we either use legacy or new one,
> > bot both, which is problematic for people using old seabios
> > without acpi loading support and with -M pc.
> > 
> > I don't say we must support them with >255 CPUs,
> > but I do say we should make an effort for simple
> > setups with <255 CPUs.
> 
> 
> it works with QEMU provided(shipped) BIOS,
> it works for migration case as legacy stays enables because of -M 
> src_legacy_machine
> 
> more that 255 cpus will break old BIOS in different
> ways /corrupt/hang/ depending on BIOS version.
> 
> I'm not sure we should care about using old BIOS
> with new QEMU+new machine type though and allow
> expectations to be beyond what hw vendors usually set.
> It's the same as real hw does i.e. new hardware
> shipped with new BIOS. 
> If user insist on old BIOS+new machine type he still has
> property knob to force legacy mode.
> 
> on the fist glance, it's probably not that very hard
> to switch IO ports handling from legacy to new interface
> by sending from new cpu-hotplug AML a command to do so,
> my concerns here is:
>  * +1 more state to migrate
>  * probably issues with migration as target started with
>    different IO layout
>  * IO window freed after switching from legacy to new,
>    will not be available to guest as it started with
>    legacy window consumed by CPUS.CRS.
>  * that legacy switching business is only PC specific
>     means having a knob to turn it on so it won't pollute ARM
> 
> all in all it's probably too much headache to make sure
> that improbable usecase would work, so after considering
> this idea I've dropped it and did it the way it's now.

Well I think it's worth the effort. I agree it's tricky
to implement but we do maintain compatibility for years.
Being orthogonal with bios version is very helpful for
a variety of reasons such as debugging.


> > > +Register block size:
> > > +    ACPI_CPU_HOTPLUG_REG_LEN = 12
> > > +
> > > +read access:  
> > 
> > So this implies acpi must scan all cpus on each event, and
> > this seems too aggressive.
> > I think we need something hierarchical where
> > you read one level and know which cpus to probe.
> That's what we do for mem-hotplug as it's not
> performance critical path.
> 
> In addition to that depending on guest OS/version
> it will anyway do enumeration of all CPUs after
> our hotplug AML method scanned all CPUs and
> sent notifies.
> 
> not that I'm in favor of complicating this protocol,
> but I wouldn't do it hierarchical,
> that's what Notify(BUS_CHECK) is supposed to do
> but it's broken on some guests.

Interesting. Which ones? Would they be easy to detect?

> So if I'd do a more complicated protocol,
> I'd do polling from AML side telling QEMU
>   if (cpu = give_me_next_cpu_with_event())
>      switch_to_cpu(cpu)
>      ...
> 
> that means adding extra state to migrate,
> probably ther might be other issues I'm not aware of yet.
> 
> On the other hand mem-hotplug like interface
> so far hasn't caused any issues
> (that of cause doesn't mean that there aren't any).
> 
> > > +    offset:
> > > +    [0x0-0x3] reserved
> > > +    [0x4] CPU device status fields: (1 byte access)
> > > +        bits:
> > > +           0: Device is enabled and may be used by guest
> > > +           1: Device insert event, used to distinguish device for which
> > > +              no device check event to OSPM was issued.
> > > +              It's valid only when bit 0 is set.
> > > +           2: Device remove event, used to distinguish device for which
> > > +              no device eject request to OSPM was issued.
> > > +           3-7: reserved and should be ignored by OSPM
> > > +    [0x5-0x7] reserved
> > > +    [0x8] Command data: (DWORD access)
> > > +          in case of error or unsupported command reads is 0xFFFFFFFF
> > > +          current 'Command field' value:
> > > +              0: returns PXM value corresponding to device
> > > +
> > > +write access:
> > > +    offset:
> > > +    [0x0-0x3] CPU selector: (DWORD access)
> > > +              selects active CPU device. All following accesses to other
> > > +              registers will read/store data from/to selected CPU.  
> > 
> > I've been thinking - is it time to add an EmbeddedControl interface?
> > This way we have another namespace.
> 
> There were patches on list with it some time ago
> 
>     "[PATCH RFC v2 0/7] coordinate cpu hotplug/unplug bewteen QEMU and kernel 
> by EC"
> 
> but we dropped it as bloatware because there weren't any
> real need for it as current IO port interfaces aren't
> going away any time soon and work just fine without EC.

Right. But now you are removing them for >255 CPUs
anyway.

> And if we were to move current interfaces into EC namespace
> we would have to do it only for new machines types
> and keep old code in place as well and a bunch of compat
> code for a company.

Hmm. Exactly like this current one?

> > Not insisting on it, just an idea.
> I wouldn't make it a necessary dependency and block this series.
> Might be worth to look at again when there is usecases for it
> without legacy stuff attached.

and then we'll have to maintain 3 interfaces?

> > 
> > > +    [0x4] CPU device control fields: (1 byte access)
> > > +        bits:
> > > +            0: reserved, OSPM must clear it before writing to register.
> > > +            1: if set to 1 clears device insert event, set by OSPM
> > > +               after it has emitted device check event for the
> > > +               selected CPU device
> > > +            2: if set to 1 clears device remove event, set by OSPM
> > > +               after it has emitted device eject request for the
> > > +               selected CPU device
> > > +            3: if set to 1 initiates device eject, set by OSPM when it
> > > +               triggers CPU device removal and calls _EJ0 method
> > > +            4-7: reserved, OSPM must clear them before writing to 
> > > register
> > > +    [0x5] Command field: (1 byte access)
> > > +          value:
> > > +            0: following reads from 'Command data' register returns PXM
> > > +               value of device
> > > +            1: following writes to 'Command data' register set OST event
> > > +               register in QEMU
> > > +            2: following writes to 'Command data' register set OST status
> > > +               register in QEMU
> > > +            other values: reserved
> > > +    [0x6-0x7] reserved
> > > +    [0x8] Command data: (DWORD access)
> > > +          current 'Command field' value:
> > > +              1: stores value into OST event register
> > > +              2: stores value into OST status register, triggers
> > > +                 ACPI_DEVICE_OST QMP event from QEMU to external 
> > > applications
> > > +                 with current values of OST event and status registers.
> > > +            other values: reserved
> > >  
> > > -CPU hot-add/remove notification:
> > > ------------------------------------------------------
> > > -QEMU sets/clears corresponding CPU bit on hot-add/remove event.
> > > -CPU present map read by ACPI BIOS GPE.2 handler to notify OS of CPU
> > > -hot-(un)plug events.
> > > +Selecting CPU device beyond possible range has no effect on platform:
> > > +   - write accesses to CPU hot-plug registers not documented above are
> > > +     ignored
> > > +   - read accesses to CPU hot-plug registers not documented above return
> > > +     all bits set to 1.  
> > 
> > why not 0?
> isn't hardware usually return 1s on unhandled IO reads?
> (I even thought that's it was you who told me it)

PCI does but it's special.

> 
> that's what has been done for memory hotplug, so I'm doing the same here.
> 
> > 
> > > -- 
> > > 1.8.3.1  
> >