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

[Michael S. Tsirkin]

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

> +      [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?
what if device is not 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?

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

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.


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

> 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