Re: [Qemu-devel] [PATCH v7 6/8] acpi: fix "Memory device control fields" register

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 04:47:20PM +0800, Zhu Guihua wrote:
> 0 bit in Memory device control fields must be cleared before writing to
> register. But now this field isn't cleared when other fields are written.
> 
> To solve this bug, This patch fixes UpdateRule to WriteAsZeros in "Memory
> device control fields" register.
> 
> Reviewed-by: Igor Mammedov <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>

Applied, thanks!
Please check pci branch in my tree as I had to
resolve some trivial conflicts.

> ---
>  docs/specs/acpi_mem_hotplug.txt | 5 ++++-
>  hw/i386/acpi-build.c            | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index dcc4808..68daa14 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -31,7 +31,10 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
> access):
>        [0xc-0x13] reserved, writes into it are ignored
>        [0x14] Memory device control fields
>            bits:
> -              0: reserved, OSPM must clear it before writing to register
> +              0: reserved, OSPM must clear it before writing to register.
> +                 Due to BUG in versions prior 2.4 that field isn't cleared
> +                 when other fields are written. Keep it reserved and don't
> +                 try to reuse it.
>                1: if set to 1 clears device insert event, set by OSPM
>                   after it has emitted device check event for the
>                   selected memory device
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index dc1b8e8..eb66fef 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -924,7 +924,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(scope, field);
>  
>          field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_byte_acc,
> -                          aml_preserve);
> +                          aml_write_as_zeros);
>          aml_append(field, aml_reserved_field(160 /* bits, Offset(20) */));
>          aml_append(field, /* 1 if enabled, read only */
>              aml_named_field(stringify(MEMORY_SLOT_ENABLED), 1));
> -- 
> 1.9.3

On Mon, Apr 27, 2015 at 04:47:21PM +0800, Zhu Guihua wrote:
> - implements QEMU hardware part of memory hot unplug protocol
>   described at "docs/spec/acpi_mem_hotplug.txt"
> - handles memory remove notification event
> - handles device eject notification
> 
> Reviewed-by: Igor Mammedov <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>  hw/acpi/memory_hotplug.c          | 21 ++++++++++++++++++++-
>  hw/core/qdev.c                    |  2 +-
>  hw/i386/acpi-build.c              | 12 ++++++++++++
>  hw/i386/acpi-dsdt-mem-hotplug.dsl | 13 ++++++++++++-
>  include/hw/acpi/pc-hotplug.h      |  3 +++
>  include/hw/qdev-core.h            |  1 +
>  trace-events                      |  3 +++
>  8 files changed, 61 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
> index 68daa14..3df3620 100644
> --- a/docs/specs/acpi_mem_hotplug.txt
> +++ b/docs/specs/acpi_mem_hotplug.txt
> @@ -19,7 +19,9 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
> access):
>                1: Device insert event, used to distinguish device for which
>                   no device check event to OSPM was issued.
>                   It's valid only when bit 1 is set.
> -              2-7: reserved and should be ignored by OSPM
> +              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
>        [0x15-0x17] reserved
>  
>    write access:
> @@ -38,7 +40,12 @@ Memory hot-plug interface (IO port 0xa00-0xa17, 1-4 byte 
> access):
>                1: if set to 1 clears device insert event, set by OSPM
>                   after it has emitted device check event for the
>                   selected memory device
> -              2-7: reserved, OSPM must clear them before writing to register
> +              2: if set to 1 clears device remove event, set by OSPM
> +                 after it has emitted device eject request for the
> +                 selected memory device
> +              3: if set to 1 initiates device eject, set by OSPM when it
> +                 triggers memory device removal and calls _EJ0 method
> +              4-7: reserved, OSPM must clear them before writing to register
>  
>  Selecting memory device slot beyond present range has no effect on platform:
>     - write accesses to memory hot-plug registers not documented above are
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 07e281f..35bbfeb 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -2,6 +2,7 @@
>  #include "hw/acpi/pc-hotplug.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/boards.h"
> +#include "hw/qdev-core.h"
>  #include "trace.h"
>  #include "qapi-event.h"
>  
> @@ -91,6 +92,8 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
> addr, uint64_t data,
>      MemHotplugState *mem_st = opaque;
>      MemStatus *mdev;
>      ACPIOSTInfo *info;
> +    DeviceState *dev = NULL;
> +    HotplugHandler *hotplug_ctrl = NULL;
>  
>      if (!mem_st->dev_count) {
>          return;
> @@ -128,13 +131,29 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>          qapi_event_send_acpi_device_ost(info, &error_abort);
>          qapi_free_ACPIOSTInfo(info);
>          break;
> -    case 0x14:
> +    case 0x14: /* set is_* fields  */
>          mdev = &mem_st->devs[mem_st->selector];
>          if (data & 2) { /* clear insert event */
>              mdev->is_inserting  = false;
>              trace_mhp_acpi_clear_insert_evt(mem_st->selector);
> +        } else if (data & 4) {
> +            mdev->is_removing = false;
> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> +        } else if (data & 8) {
> +            if (!mdev->is_enabled) {
> +                trace_mhp_acpi_ejecting_invalid_slot(mem_st->selector);
> +                break;
> +            }
> +
> +            dev = DEVICE(mdev->dimm);
> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +            /* call pc-dimm unplug cb */
> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> +            trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
>          }
>          break;
> +    default:
> +        break;
>      }
>  
>  }
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6e6a65d..b0f0f84 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -273,7 +273,7 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int 
> alias_id,
>      dev->alias_required_for_version = required_for_version;
>  }
>  
> -static HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
>  {
>      HotplugHandler *hotplug_ctrl = NULL;
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index eb66fef..96c1db3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -931,6 +931,12 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(field,
>              /*(read) 1 if has a insert event. (write) 1 to clear event */
>              aml_named_field(stringify(MEMORY_SLOT_INSERT_EVENT), 1));
> +        aml_append(field,
> +            /* (read) 1 if has a remove event. (write) 1 to clear event */
> +            aml_named_field(stringify(MEMORY_SLOT_REMOVE_EVENT), 1));
> +        aml_append(field,
> +            /* initiates device eject, write only */
> +            aml_named_field(stringify(MEMORY_SLOT_EJECT), 1));
>          aml_append(scope, field);
>  
>          field = aml_field(stringify(MEMORY_HOTPLUG_IO_REGION), aml_dword_acc,
> @@ -975,6 +981,12 @@ build_ssdt(GArray *table_data, GArray *linker,
>              )));
>              aml_append(dev, method);
>  
> +            method = aml_method("_EJ0", 1);
> +            s = BASEPATH stringify(MEMORY_SLOT_EJECT_METHOD);
> +            aml_append(method, aml_return(aml_call2(
> +                       s, aml_name("_UID"), aml_arg(0))));
> +            aml_append(dev, method);
> +
>              aml_append(sb_scope, dev);
>          }
>  
> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl 
> b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 1e9ec39..c2bb6a1 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -29,6 +29,8 @@
>              External(MEMORY_SLOT_PROXIMITY, FieldUnitObj) // read only
>              External(MEMORY_SLOT_ENABLED, FieldUnitObj) // 1 if enabled, 
> read only
>              External(MEMORY_SLOT_INSERT_EVENT, FieldUnitObj) // (read) 1 if 
> has a insert event. (write) 1 to clear event
> +            External(MEMORY_SLOT_REMOVE_EVENT, FieldUnitObj) // (read) 1 if 
> has a remove event. (write) 1 to clear event
> +            External(MEMORY_SLOT_EJECT, FieldUnitObj) // initiates device 
> eject, write only
>              External(MEMORY_SLOT_SLECTOR, FieldUnitObj) // DIMM selector, 
> write only
>              External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event 
> code, write only
>              External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status 
> code, write only
> @@ -55,8 +57,10 @@
>                      If (LEqual(MEMORY_SLOT_INSERT_EVENT, One)) { // Memory 
> device needs check
>                          MEMORY_SLOT_NOTIFY_METHOD(Local0, 1)
>                          Store(1, MEMORY_SLOT_INSERT_EVENT)
> +                    } Elseif (LEqual(MEMORY_SLOT_REMOVE_EVENT, One)) { // 
> Ejection request
> +                        MEMORY_SLOT_NOTIFY_METHOD(Local0, 3)
> +                        Store(1, MEMORY_SLOT_REMOVE_EVENT)
>                      }
> -                    // TODO: handle memory eject request
>                      Add(Local0, One, Local0) // goto next DIMM
>                  }
>                  Release(MEMORY_SLOT_LOCK)
> @@ -156,5 +160,12 @@
>                  Store(Arg2, MEMORY_SLOT_OST_STATUS)
>                  Release(MEMORY_SLOT_LOCK)
>              }
> +
> +            Method(MEMORY_SLOT_EJECT_METHOD, 2) {
> +                Acquire(MEMORY_SLOT_LOCK, 0xFFFF)
> +                Store(ToInteger(Arg0), MEMORY_SLOT_SLECTOR) // select DIMM
> +                Store(1, MEMORY_SLOT_EJECT)
> +                Release(MEMORY_SLOT_LOCK)
> +            }
>          } // Device()
>      } // Scope()
> diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> index efa6ed7..0513c1c 100644
> --- a/include/hw/acpi/pc-hotplug.h
> +++ b/include/hw/acpi/pc-hotplug.h
> @@ -43,6 +43,8 @@
>  #define MEMORY_SLOT_PROXIMITY        MPX
>  #define MEMORY_SLOT_ENABLED          MES
>  #define MEMORY_SLOT_INSERT_EVENT     MINS
> +#define MEMORY_SLOT_REMOVE_EVENT     MRMV
> +#define MEMORY_SLOT_EJECT            MEJ
>  #define MEMORY_SLOT_SLECTOR          MSEL
>  #define MEMORY_SLOT_OST_EVENT        MOEV
>  #define MEMORY_SLOT_OST_STATUS       MOSC
> @@ -51,6 +53,7 @@
>  #define MEMORY_SLOT_CRS_METHOD       MCRS
>  #define MEMORY_SLOT_OST_METHOD       MOST
>  #define MEMORY_SLOT_PROXIMITY_METHOD MPXM
> +#define MEMORY_SLOT_EJECT_METHOD     MEJ0
>  #define MEMORY_SLOT_NOTIFY_METHOD    MTFY
>  #define MEMORY_SLOT_SCAN_METHOD      MSCN
>  
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4e673f9..5b7acf1 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -266,6 +266,7 @@ int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
>  void qdev_init_nofail(DeviceState *dev);
>  void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
>                                   int required_for_version);
> +HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
>  void qdev_unplug(DeviceState *dev, Error **errp);
>  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>                                    DeviceState *dev, Error **errp);
> diff --git a/trace-events b/trace-events
> index 30eba92..46f6ef0 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1562,6 +1562,7 @@ vfio_put_base_device(int fd) "close vdev->fd=%d"
>  
>  #hw/acpi/memory_hotplug.c
>  mhp_acpi_invalid_slot_selected(uint32_t slot) "0x%"PRIx32
> +mhp_acpi_ejecting_invalid_slot(uint32_t slot) "0x%"PRIx32
>  mhp_acpi_read_addr_lo(uint32_t slot, uint32_t addr) "slot[0x%"PRIx32"] addr 
> lo: 0x%"PRIx32
>  mhp_acpi_read_addr_hi(uint32_t slot, uint32_t addr) "slot[0x%"PRIx32"] addr 
> hi: 0x%"PRIx32
>  mhp_acpi_read_size_lo(uint32_t slot, uint32_t size) "slot[0x%"PRIx32"] size 
> lo: 0x%"PRIx32
> @@ -1572,6 +1573,8 @@ mhp_acpi_write_slot(uint32_t slot) "set active slot: 
> 0x%"PRIx32
>  mhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "slot[0x%"PRIx32"] OST 
> EVENT: 0x%"PRIx32
>  mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "slot[0x%"PRIx32"] OST 
> STATUS: 0x%"PRIx32
>  mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert 
> event"
> +mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove 
> event"
> +mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
>  
>  # hw/i386/pc.c
>  mhp_pc_dimm_assigned_slot(int slot) "0x%d"
> -- 
> 1.9.3

On Mon, Apr 27, 2015 at 04:47:22PM +0800, Zhu Guihua wrote:
> When memory hot unplug fails, this patch adds support to send
> QMP event to notify mgmt about this failure.
> 
> Reviewed-by: Igor Mammedov <address@hidden>
> Signed-off-by: Zhu Guihua <address@hidden>
> ---
>  docs/qmp/qmp-events.txt  | 17 +++++++++++++++++
>  hw/acpi/memory_hotplug.c | 10 +++++++++-
>  qapi/event.json          | 14 ++++++++++++++
>  trace-events             |  1 +
>  4 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index d759d19..3be468f 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -226,6 +226,23 @@ Example:
>  { "event": "GUEST_PANICKED",
>       "data": { "action": "pause" } }
>  
> +MEM_HOT_UNPLUG_ERROR
> +--------------------
> +Emitted when memory hot unplug error occurs.
> +
> +Data:
> +
> +- "device": device name (json-string)
> +- "msg": Informative message (e.g., reason for the error) (json-string)
> +
> +Example:
> +
> +{ "event": "MEM_HOT_UNPLUG_ERROR"
> +  "data": { "device": "dimm1",
> +            "msg": "acpi: device unplug for unsupported device"
> +  },
> +  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +
>  NIC_RX_FILTER_CHANGED
>  ---------------------
>  
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 35bbfeb..34cef1e 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -94,6 +94,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
> addr, uint64_t data,
>      ACPIOSTInfo *info;
>      DeviceState *dev = NULL;
>      HotplugHandler *hotplug_ctrl = NULL;
> +    Error *local_err = NULL;
>  
>      if (!mem_st->dev_count) {
>          return;
> @@ -148,7 +149,14 @@ static void acpi_memory_hotplug_write(void *opaque, 
> hwaddr addr, uint64_t data,
>              dev = DEVICE(mdev->dimm);
>              hotplug_ctrl = qdev_get_hotplug_handler(dev);
>              /* call pc-dimm unplug cb */
> -            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> +            hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
> +            if (local_err) {
> +                trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> +                qapi_event_send_mem_unplug_error(dev->id,
> +                                                 error_get_pretty(local_err),
> +                                                 &error_abort);
> +                break;
> +            }
>              trace_mhp_acpi_pc_dimm_deleted(mem_st->selector);
>          }
>          break;
> diff --git a/qapi/event.json b/qapi/event.json
> index c51dc49..378dda5 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -330,3 +330,17 @@
>  ##
>  { 'event': 'VSERPORT_CHANGE',
>    'data': { 'id': 'str', 'open': 'bool' } }
> +
> +##
> +# @MEM_UNPLUG_ERROR
> +#
> +# Emitted when memory hot unplug error occurs.
> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 2.4
> +##
> +{ 'event': 'MEM_UNPLUG_ERROR',
> +  'data': { 'device': 'str', 'msg': 'str' } }
> diff --git a/trace-events b/trace-events
> index 46f6ef0..11387c3 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1575,6 +1575,7 @@ mhp_acpi_write_ost_status(uint32_t slot, uint32_t st) 
> "slot[0x%"PRIx32"] OST STA
>  mhp_acpi_clear_insert_evt(uint32_t slot) "slot[0x%"PRIx32"] clear insert 
> event"
>  mhp_acpi_clear_remove_evt(uint32_t slot) "slot[0x%"PRIx32"] clear remove 
> event"
>  mhp_acpi_pc_dimm_deleted(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm deleted"
> +mhp_acpi_pc_dimm_delete_failed(uint32_t slot) "slot[0x%"PRIx32"] pc-dimm 
> delete failed"
>  
>  # hw/i386/pc.c
>  mhp_pc_dimm_assigned_slot(int slot) "0x%d"
> -- 
> 1.9.3