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

[Igor Mammedov]

On Thu, 21 Nov 2013 11:42:02 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Nov 21, 2013 at 03:38:34AM +0100, Igor Mammedov wrote:
> > - implements QEMU hardware part of memory hotplug protocol
> >   described at "docs/specs/acpi_mem_hotplug.txt"
> > - handles only memory add notification event for now
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  docs/specs/acpi_mem_hotplug.txt |   38 ++++++++++
> >  hw/acpi/core.c                  |  144 
> > +++++++++++++++++++++++++++++++++++++++
> >  include/hw/acpi/acpi.h          |   24 +++++++
> >  3 files changed, 206 insertions(+), 0 deletions(-)
> >  create mode 100644 docs/specs/acpi_mem_hotplug.txt
> > 
> > diff --git a/docs/specs/acpi_mem_hotplug.txt 
> > b/docs/specs/acpi_mem_hotplug.txt
> > new file mode 100644
> > index 0000000..fc34142
> > --- /dev/null
> > +++ b/docs/specs/acpi_mem_hotplug.txt
> > @@ -0,0 +1,38 @@
> > +QEMU<->ACPI BIOS memory hotplug interface
> > +--------------------------------------
> > +
> > +ACPI BIOS GPE.3 handler is dedicated for notifying OS about memory hot-add
> > +or hot-remove events.
> > +
> > +Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte access):
> > +---------------------------------------------------------------
> > +0xa00:
> > +  read access:
> > +      [0x0-0x3] Lo part of memory device phys address
> > +      [0x4-0x7] Hi part of memory device phys address
> > +      [0x8-0xb] Lo part of memory device size in bytes
> > +      [0xc-0xf] Hi part of memory device size in bytes
> > +      [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?

> 
> > +      [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)

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

> 
> > +      [0x16-0x17] reserved
> > +
> > +  write access:
> > +      [0x0-0x3] Memory device slot selector, selects active memory device.
> > +                All following accesses to other registers in 0xaf80-0xaf97
> > +                region will read/store data from/to selected memory device.
> > +      [0x4-0x7] OST event code reported by OSPM
> > +      [0x8-0xb] OST status code reported by OSPM
> > +      [0x15] Memory device status fields
> 
> this is control, not status?
Thanks, I'll fix it.

> 
> > +          bits:
> > +              2: if set to 1 clears device insert event, set by ACPI BIOS
> > +                 after it has sent device check event to OSPM for
> > +                 seleted memory device
> 
> selected?
see "write access: [0x0-0x3]"

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

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

> 
> > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > index 8c0d48c..18e169c 100644
> > --- a/hw/acpi/core.c
> > +++ b/hw/acpi/core.c
> > @@ -680,3 +680,147 @@ void acpi_update_sci(ACPIREGS *regs, qemu_irq irq, 
> > uint32_t gpe0_sts_mask)
> >                         (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
> >                         !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
> >  }
> > +
> > +static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr,
> > +                                         unsigned int size)
> > +{
> > +    uint32_t val = 0;
> > +    MemHotplugState *mem_st = opaque;
> > +    MemStatus *mdev;
> > +
> > +    if (mem_st->selector >= mem_st->dev_count) {
> > +        return 0;
> > +    }
> > +
> > +    mdev = &mem_st->devs[mem_st->selector];
> > +    switch (addr) {
> > +    case 0x0: /* Lo part of phys address where DIMM is mapped */
> > +        val = object_property_get_int(OBJECT(mdev->dimm), "start", NULL);
> > +        break;
> > +    case 0x4: /* Hi part of phys address where DIMM is mapped */
> > +        val = object_property_get_int(OBJECT(mdev->dimm), "start", NULL) 
> > >> 32;
> > +        break;
> > +    case 0x8: /* Lo part of DIMM size */
> > +        val = object_property_get_int(OBJECT(mdev->dimm), "size", NULL);
> > +        break;
> > +    case 0xc: /* Hi part of DIMM size */
> > +        val = object_property_get_int(OBJECT(mdev->dimm), "size", NULL) >> 
> > 32;
> > +        break;
> > +    case 0x10: /* node proximity for _PXM method */
> > +        val = object_property_get_int(OBJECT(mdev->dimm), "node", NULL);
> > +        break;
> > +    case 0x14: /* intf version */
> > +        val = 1;
> > +        break;
> > +    case 0x15: /* pack and return is_* fields */
> > +        val |= mdev->is_enabled   ? 1 : 0;
> > +        val |= mdev->is_inserting ? 2 : 0;
> > +        break;
> > +    }
> > +    return val;
> > +}
> > +
> > +static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t 
> > data,
> > +                                      unsigned int size)
> > +{
> > +    MemHotplugState *mem_st = opaque;
> > +    MemStatus *mdev;
> > +
> > +    if (!mem_st->dev_count) {
> > +        return;
> > +    }
> > +
> > +    if (addr) {
> > +        if (mem_st->selector >= mem_st->dev_count) {
> > +            return;
> > +        }
> > +    }
> > +
> > +    switch (addr) {
> > +    case 0x0: /* DIMM slot selector */
> > +        mem_st->selector = data;
> > +        break;
> > +    case 0x4: /* _OST event  */
> > +        mdev = &mem_st->devs[mem_st->selector];
> > +        if (data == 1) {
> > +            /* TODO: handle device insert OST event */
> > +        } else if (data == 3) {
> > +            /* TODO: handle device remove OST event */
> > +        }
> > +        mdev->ost_event = data;
> > +        break;
> > +    case 0x8: /* _OST status */
> > +        mdev = &mem_st->devs[mem_st->selector];
> > +        mdev->ost_status = data;
> > +        /* TODO: report async error */
> > +        /* TODO: implement memory removal on guest signal */
> > +        break;
> > +    case 0x15:
> > +        mdev = &mem_st->devs[mem_st->selector];
> > +        if (data & 2) { /* clear insert event */
> > +            mdev->is_inserting  = false;
> > +        }
> > +        break;
> > +    }
> > +
> > +}
> > +static const MemoryRegionOps acpi_memory_hotplug_ops = {
> > +    .read = acpi_memory_hotplug_read,
> > +    .write = acpi_memory_hotplug_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid = {
> > +        .min_access_size = 1,
> > +        .max_access_size = 4,
> > +    },
> > +};
> > +
> > +void acpi_memory_hotplug_init(Object *owner, MemoryRegion *io,
> > +                              MemHotplugState *state)
> > +{
> > +    QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL);
> > +    g_assert(opts);
> > +
> > +    state->dev_count = qemu_opt_get_number(opts, "slots", 0);
> > +
> > +    if (!state->dev_count) {
> > +        return;
> > +    }
> > +
> > +    state->devs = g_malloc0(sizeof(*state->devs) * state->dev_count);
> > +    memory_region_init_io(io, owner, &acpi_memory_hotplug_ops, state,
> > +                          "apci-mem-hotplug", ACPI_MEMORY_HOTPLUG_IO_LEN);
> > +}
> > +
> > +int acpi_memory_hotplug_cb(ACPIREGS *regs, MemHotplugState *mem_st,
> > +                           DeviceState *dev, HotplugState state)
> > +{
> > +    MemStatus *mdev;
> > +    Error *local_err = NULL;
> > +    int slot = object_property_get_int(OBJECT(dev), "slot", &local_err);
> > +
> > +    if (error_is_set(&local_err)) {
> > +        qerror_report_err(local_err);
> > +        error_free(local_err);
> > +        return -1;
> > +    }
> > +
> > +    if (slot >= mem_st->dev_count) {
> > +        char *dev_path = object_get_canonical_path(OBJECT(dev));
> > +        qerror_report(ERROR_CLASS_GENERIC_ERROR, "acpi_memory_hotplug_cb: "
> > +                      "device [%s] returned invalid memory slot[%d]",
> > +                      dev_path, slot);
> > +        g_free(dev_path);
> > +        return -1;
> > +    }
> > +
> > +    mdev = &mem_st->devs[slot];
> > +    if (state == HOTPLUG_ENABLED) {
> > +        mdev->dimm = dev;
> > +        mdev->is_enabled = true;
> > +        mdev->is_inserting = true;
> > +    }
> > +
> > +    /* do ACPI magic */
> > +    regs->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS;
> > +    return 0;
> > +}
> > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > index c4ae7d7..e5717df 100644
> > --- a/include/hw/acpi/acpi.h
> > +++ b/include/hw/acpi/acpi.h
> > @@ -168,6 +168,30 @@ uint32_t acpi_gpe_ioport_readb(ACPIREGS *ar, uint32_t 
> > addr);
> >  
> >  void acpi_update_sci(ACPIREGS *acpi_regs, qemu_irq irq, uint32_t 
> > gpe0_sts_mask);
> >  
> > +#define ACPI_MEMORY_HOTPLUG_IO_LEN 24
> > +#define ACPI_MEMORY_HOTPLUG_BASE 0x0a00
> > +
> > +#define ACPI_MEMORY_HOTPLUG_STATUS 8
> > +
> > +typedef struct MemStatus {
> > +    DeviceState *dimm;
> > +    bool is_enabled;
> > +    bool is_inserting;
> > +    uint32_t ost_event;
> > +    uint32_t ost_status;
> > +} MemStatus;
> > +
> > +typedef struct MemHotplugState {
> > +    uint32_t selector;
> > +    uint32_t dev_count;
> > +    MemStatus *devs;
> > +} MemHotplugState;
> > +
> > +void acpi_memory_hotplug_init(Object *owner, MemoryRegion *io,
> > +                              MemHotplugState *state);
> > +
> > +int acpi_memory_hotplug_cb(ACPIREGS *regs, MemHotplugState *mem_st,
> > +                           DeviceState *dev, HotplugState state);
> >  /* acpi.c */
> >  extern int acpi_enabled;
> >  extern char unsigned *acpi_tables;
> > -- 
> > 1.7.1