Re: [Qemu-devel] [PATCH 13/27] acpi: memory hotplug ACPI hardware implementation

[Igor Mammedov]

On Thu, 21 Nov 2013 16:38:47 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Nov 21, 2013 at 03:21:37PM +0100, Igor Mammedov wrote:
> > On Thu, 21 Nov 2013 11:42:02 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
[...]
> > > > +      [0x14] highest memory hot-plug interface version supported by 
> > > > QEMU
> > > 
> > > So this can make guest fail gracefully but it appears that
> > > detecting guest version would be nicer?
> > > It would let us actually support old guests ...
> > 
> > my idea of how to it was,
> > guest writes its version into [0x14] register and reads QEMU version
> > from it back, if they do not match then than BIOS ignores GPE.3 event
> > effectively disabling hotplug on guest side.
> > I haven't thought about supporting multiple implementations in QEMU though.
> > Do we really want it?
> 
> I'm talking about old bios which does not read acpi from qemu.
> We want it to work even if it can't see hotplugged memory.
to sum up cases that were discussed on IRC.

 * migration from NEW to OLD machine leads us to state when we run new BIOS with
   ACPI tables supporting memory hotplug on OLD machine.
   In general should work since NEW machine has to be started without memory
   hotplug enabled, which leads to disabling related ASL handler in ACPI tables.
   Case where NEW machine with memory hotplug enabled migrating to OLD machine
   without memory hotplug is invalid since migration will fail early due to
   missing devices (DimmBus/DIMM devices)

 * running OLD BIOS on new machine with memory hotplug turned on at QEMU CLI
        qemu -m X,slots=Y,maxmem=Z -bios old-bios-image -M pc-1.8

   Problem here is that OLD BIOS doesn't know about 'etc/reserved-memory-end' 
fwcfg,
   so it can start mapping PCI BARs right after high memory (i.e. in memory 
hotplug
   reserved space)

   That's fine as far as there is no DIMM devices since access will fall through
   hotplug-mem container to PCI address space.

   But if DIMM device added at startup or runtime, its MemoryRegion will 
overshadow
   PCI BARs mapped at its range.

   If it were only hotadd case, then QEMU could start with enabled but not 
active
   memory hotplug and require BIOS to acknowledge (activate) hotlug so that 
adding
   DIMM devices wouldn't be possible without hotplug enabled BIOS. 
   That would guarantee empty hotplug-mem container for OLD BIOS since it can't
   activate hotplug and avoid conflict.

   But if there are cold-plugged DIMM devices on QEMU CLI, that wouldn't work
   because OLD BIOS has no idea about them (i.e. it reads above4gb memory CMOS 
value)
   and it is bound to map 64-bit BARs in invalid place.
 
   It would be nice to run OLD BIOS on new machine and it will even work IF 
memory
   hotplug is not enabled.
   But I'm not sure we should care about case when not compatible BIOS is 
running
   on machine with active memory hotplug.

> 
> > > 
> > > > +      [0x15] Memory device status fields
> > > > +          bits:
> > > > +              1: device is enabled and may be used by guest
> > > > +              2: device insert event, used by ACPI BIOS to distinguish
> > > > +                 device for which no device check event to OSPM was 
> > > > issued
> > > 
> > > what does the above mean?
> > After OSPM issued device check on selected device it clears this bit to 
> > mark event
> > as handled.
> > It allows to avoid keeping this state in ASL (as it's done for CPU hotplug, 
> > see CPON)
> 
> That's fine.
> 
> > > what if device is not present?
> > ASL will issue device check and clear bit, it might be a bug since _STA 
> > would report
> > not present but no eject event was issued.
> > 
> > Papering over it ASL could check present bit first and issue device check 
> > only if
> > it's present.
> 
> Is this a problem? If yes - that will still be racy won't it?
I thought about it some more and I don't see why it would be racy.
bit 0x15.1 is set when there is initialized DIMM device in slot so 0x15.2 
couldn't
be set if 0x15.1 == 0.

May be following description would be better:

   2: device insert event, used by ACPI BIOS to distinguish
      device for which no device check event to OSPM was issued.
      Bit is valid only when 0x15.1 bit is set.

It's possible to remove is_inserting bit altogether and just send device check 
to
each present memory device. Device check will be NOP for memory devices that 
OSPM
processed.
That will work fine with small amount of slots but would increase load on guest
in case of 1000s devices. That's the main reason for dedicated is_inserting bit,
allowing to handle hotadd effectively.

      
> 
> 
> Also, should guest reset eject memory that we requested unplug for?
I'm not sure what you saying here in general.
In particular unplug is not part of this series.


[...]
> > > selected?
> > see "write access: [0x0-0x3]"
> 
> yes but you have a typo above
Ahh, blind me. Thanks, I'll fix it.

> 
> > > 
> > > How about we actually require guest to enable memory?
> > > 
> > > This way if we hotplug, but old guest does not enable
> > > and puts a PCI device there, it just works.
> > I've lost you here, could you elaborate pls?
> 
> 
> Assume qemu adds memory by hotplug.
> Is it immediately enabled?
> I suggest it's not enabled, and only enable after ACPI
> enables it (or after reboot?)
I guess answer is in "running OLD BIOS on new machine with memory hotplug 
turned on"
case above. i.e. Don't run not compatible BIOS with turned on memory hotplug.
I think simpler interface/work-flow is better than a more complicated one,
that is required to support not totally sane configuration that is bound to be 
broken anyway.

> 
> > > 
> > > > +
> > > > +Selecting memory device slot beyond present range has no effect on 
> > > > platform:
> > > > +   - not documented above write accesses to memory hot-plug registers
> > > > +     are ignored;
> > > > +   - not documented above read accesses to memory hot-plug registers 
> > > > return 0xFF
> > > 
> > > 00 would be cleaner (matches not enabled - no event)?
> > I'm following pattern, where reading from not present IO port returns 0xFF 
> > on hardware.
> > Fact that ASL reads 0xFF could be used as not supported indication.
> 
> But isn't this a valid pattern when all bits are set?
It's to the same extent as 0x0. I'll change it to 0x0 if you still think it's 
better.

[...]