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:
> On Mon, 16 Mar 2015 16:58:18 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> This patch adds a new bit to memory hotplug IO port indicating that
> actually bit was added in 2/6 where is_removing had been added.
>
>> EJ0 has been evaluated by guest OS. And call pc-dimm unplug cb to do
>> the real removal.
>>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> ---
>>   docs/specs/acpi_mem_hotplug.txt   | 11 +++++++++--
>>   hw/acpi/memory_hotplug.c          | 21 +++++++++++++++++++--
>>   hw/core/qdev.c                    |  2 +-
>>   hw/i386/acpi-build.c              |  9 +++++++++
>>   hw/i386/acpi-dsdt-mem-hotplug.dsl | 10 ++++++++++
>>   include/hw/acpi/pc-hotplug.h      |  2 ++
>>   include/hw/qdev-core.h            |  1 +
>>   trace-events                      |  1 +
>>   8 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/specs/acpi_mem_hotplug.txt b/docs/specs/acpi_mem_hotplug.txt
>> index 1290994..85cd4b8 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 check event to OSPM was issued.
>> +              3-7: reserved and should be ignored by OSPM
>>         [0x15-0x17] reserved
>>   
>>     write access:
>> @@ -35,7 +37,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 check event for the
>> +                 selected memory device. if guest fails to eject device, it
>> +                 should send OST event about it and forget about device
>> +                 removal.
>> +              3-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 687b2f1..d6b8c89 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;
>> @@ -122,19 +125,33 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>           mdev = &mem_st->devs[mem_st->selector];
>>           mdev->ost_status = data;
>>           trace_mhp_acpi_write_ost_status(mem_st->selector, mdev->ost_status);
>> -        /* TODO: implement memory removal on guest signal */
>>   
>>           info = acpi_memory_device_status(mem_st->selector, mdev);
>>           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) { /* request removal of device */
> fix comment to match docs above.
>
>> +            mdev->is_removing = false;
>> +            trace_mhp_acpi_clear_remove_evt(mem_st->selector);
> just clear event here and don't do removal part as it doesn't match
> documentation you've written above regarding this field.
>
> It would be better to move is_removing handling from here to 2/6
> + related ASL code from DSDT which should clear it after sending device check.
>
>> +            /*
>> +             * QEMU memory hot unplug is an asynchronous procedure. QEMU first
>> +             * calls pc-dimm unplug request cb to send a SCI to guest. When the
>> +             * guest OS finished handling the SCI, it evaluates ACPI EJ0, and
>> +             * QEMU calls pc-dimm unplug cb to remove memory device.
>> +             */
> something like this comment, should be in acpi_mem_hotplug.txt not here.
>
>
> There is 'is_enabled' field, which is 1 if device is present, we can use it
> for triggering actual ejecting in QEMU from EJ0(), something like:
>
> } else if (data & 1) { /* eject device */

I think this is not correct. When you clear insert event, the 
'is_enabled' filed was also 1.
And when we hot remove memory, the addr 0x14 will be written only once.

Thanks,
Zhu

>> +            dev = DEVICE(mdev->dimm);
> potential NULL dereference, dimm could be NULL if guest does eject twice
> or does eject of empty slot.
> Perhaps add check before accessing dimm.
>
>   if(!mdev->is_enabled) {
>      trace_..._ejecting_invalid_slot(...)
>      break;
>   }
>
>> +            hotplug_ctrl = qdev_get_hotplug_handler(dev);
>> +            /* Call pc-dimm unplug cb. */
>> +            hotplug_handler_unplug(hotplug_ctrl, dev, NULL);
> It's not that we can do anything about error at this point
> but instead of forgetting it silently at least log error in trace,
> the best would be in addition to that send QMP event to notify mgmt
> about it. (sending QMP event could be a separate patch)
>
>
>>           }
>>           break;
>> +    default:
>> +        break;
>>       }
>>   
>>   }
>>
[...]