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

[Igor Mammedov]

On Tue, 24 Mar 2015 17:38:53 +0800
Zhu Guihua <address@hidden> wrote:

> 
> On 03/16/2015 10:59 PM, Igor Mammedov wrote:
> > On Mon, 16 Mar 2015 16:58:18 +0800
> > Zhu Guihua <address@hidden> 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 <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              |  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.
It's not clear to me what a problem you see here.
Could you give more extended explanation, pls?

> 
> 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;
> >>       }
> >>   
> >>   }
> >>
> [...]