Re: [Qemu-devel] [RESEND PATCH v4 6/6] acpi: Add hardware implementation for memory hot unplug

[Zhu Guihua]

On 03/16/2015 10:59 PM, Igor Mammedov wrote:
[...]
>   
> diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> index 1e9ec39..ef847e2 100644
> --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> @@ -29,6 +29,7 @@
>               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_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,6 +56,8 @@
>                       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)
> clear removing field here.

You mean clear remove event here?

> >                       }
> >                       // TODO: handle memory eject request
> >                       Add(Local0, One, Local0) // goto next DIMM
> > @@ -156,5 +159,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(One, MEMORY_SLOT_REMOVE_EVENT)
> redo it using enabled field. Otherwise it could cause guest/QEMU crash:
>
> - if 1st memory was asked to be removed
> - then OSPM processes it but has not called _EJ0 yet leaving is_removed set
> - then QEMU adds/removes another DIMM triggering slots scan
>     which would issue second Notify(remove) since is_removed is still set
> - as result it will cause failure in OSPM or in QEMU if OSPM issues double EJ0()

If OSPM processed the ejection request but not called _EJ0, the device 
will still be present in qemu,
we must handle this. So I think OSPM issues double EJ0 maybe reasonable 
in this situation.
What's your opinion?

Thanks,
Zhu

> > +                Release(MEMORY_SLOT_LOCK)
> > +            }
> >           } // Device()
> >       } // Scope()
> > diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
> > index efa6ed7..680810b 100644
> > --- a/include/hw/acpi/pc-hotplug.h
> > +++ b/include/hw/acpi/pc-hotplug.h
> > @@ -43,6 +43,7 @@
> >   #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_SLECTOR          MSEL
> >   #define MEMORY_SLOT_OST_EVENT        MOEV
> >   #define MEMORY_SLOT_OST_STATUS       MOSC
> > @@ -51,6 +52,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..e552355 100644
> > --- a/trace-events
> > +++ b/trace-events
> > @@ -1572,6 +1572,7 @@ 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"
> >   
> >   # hw/i386/pc.c
> >   mhp_pc_dimm_assigned_slot(int slot) "0x%d"
> .