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

[Zhu Guihua <zhugh.fnst@cn.fujitsu.com>]

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