| Message ID | 1409126919-22233-7-git-send-email-tangchen@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 27 Aug 2014 16:08:37 +0800 Tang Chen <tangchen@cn.fujitsu.com> wrote: > This patch adds a bool member named "is_removing" to MemStatus indicating if > the memory device is being removed. It is set to true in acpi_memory_unplug_cb() > when doing memory hot-remove with device_del command, or write an > ACPI_EJECTION_IN_PROGRESS status to ACPI register when triggering memory > hot-remove in guest. > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> > --- > hw/acpi/memory_hotplug.c | 62 ++++++++++++++++++++++++++++++++++++++-- > include/hw/acpi/acpi.h | 14 +++++++++ > include/hw/acpi/memory_hotplug.h | 1 + > 3 files changed, 74 insertions(+), 3 deletions(-) > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index c310b44..9074e7c 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -75,6 +75,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr, > case 0x14: /* pack and return is_* fields */ > val |= mdev->is_enabled ? 1 : 0; > val |= mdev->is_inserting ? 2 : 0; > + val |= mdev->is_removing ? 4 : 0; update docs/specs/acpi_mem_hotplug.txt with new bit field > trace_mhp_acpi_read_flags(mem_st->selector, val); > break; > default: > @@ -84,6 +85,51 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr, > return val; > } > > +static void acpi_handle_eject(MemStatus *mdev) > +{ > + switch (mdev->ost_status) { > + case ACPI_SUCCESS: > + object_unparent(OBJECT(mdev->dimm)); > + mdev->is_removing = false; > + mdev->dimm = NULL; > + break; > + case ACPI_EJECT_IN_PROGRESS: > + /* For ejection triggered by hardware (device_del command), > + * mdev->is_removing is set to true by acpi_memory_unplug_cb() > + * before sending SCI. So we only handle ejection triggered by > + * guest OS. > + */ > + if (mdev->ost_event == ACPI_OSPM_EJECT) { > + mdev->is_enabled = false; > + mdev->is_removing = true; > + } > + break; > + case ACPI_FAILURE: > + case ACPI_UNRECOGNIZED_NOTIFY: > + case ACPI_EJECT_NOT_SUPPORTED: > + case ACPI_EJECT_DEVICE_IN_USE: > + case ACPI_EJECT_DEVICE_BUSY: > + case ACPI_EJECT_DEPENDENCY_BUSY: > + mdev->is_removing = false; > + mdev->is_enabled = true; > + break; > + default: > + break; > + } > +} > + > +static void acpi_handle_ost_event(MemStatus *mdev) > +{ > + switch (mdev->ost_event) { > + case ACPI_NOTIFY_EJECT_REQUEST: /* Ejection triggered by hardware. */ > + case ACPI_OSPM_EJECT: /* Ejection triggered by guest OS. */ > + acpi_handle_eject(mdev); > + break; > + default: > + break; > + } > +} > + > static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, > unsigned int size) > { > @@ -121,21 +167,27 @@ 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); > + > + acpi_handle_ost_event(mdev); _OST is optional and OSPM doesn't have to call it at all, it was already discussed on list and using _OST for device removal was evaluated as not usable. We use _OST here as supplementary status information for management tools and no more /i.e. as LEDs on real hardware/. See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec and note below it. > 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 */ > + mdev->is_enabled = false; device tear-down should be initiated here, when OSPM calls _EJ0 method and when we return from this function it should be destroyed. > } > + > + break; > + default: > break; > } > - > } > static const MemoryRegionOps acpi_memory_hotplug_ops = { > .read = acpi_memory_hotplug_read, > @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > DeviceState *dev, Error **errp) > { > + MemStatus *mdev; > Error *local_err = NULL; > int slot = object_property_get_int(OBJECT(dev), "slot", &local_err); > > @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > return; > } > > + mdev = &mem_st->devs[slot]; > + mdev->is_removing = true; > + > /* do ACPI magic */ > ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS; > acpi_update_sci(ar, irq); > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h > index 1f678b4..e105e45 100644 > --- a/include/hw/acpi/acpi.h > +++ b/include/hw/acpi/acpi.h > @@ -91,6 +91,20 @@ > /* PM2_CNT */ > #define ACPI_BITMASK_ARB_DISABLE 0x0001 > > +/* OST_EVENT */ > +#define ACPI_NOTIFY_EJECT_REQUEST 0x03 > +#define ACPI_OSPM_EJECT 0x103 > + > +/* OST_STATUS */ > +#define ACPI_SUCCESS 0x0 > +#define ACPI_FAILURE 0x1 > +#define ACPI_UNRECOGNIZED_NOTIFY 0x2 > +#define ACPI_EJECT_NOT_SUPPORTED 0x80 > +#define ACPI_EJECT_DEVICE_IN_USE 0x81 > +#define ACPI_EJECT_DEVICE_BUSY 0x82 > +#define ACPI_EJECT_DEPENDENCY_BUSY 0x83 > +#define ACPI_EJECT_IN_PROGRESS 0x84 > + > /* structs */ > typedef struct ACPIPMTimer ACPIPMTimer; > typedef struct ACPIPM1EVT ACPIPM1EVT; > diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h > index fc6b868..fe41268 100644 > --- a/include/hw/acpi/memory_hotplug.h > +++ b/include/hw/acpi/memory_hotplug.h > @@ -11,6 +11,7 @@ typedef struct MemStatus { > DeviceState *dimm; > bool is_enabled; > bool is_inserting; > + bool is_removing; > uint32_t ost_event; > uint32_t ost_status; > } MemStatus;
Hi Igor, On 09/04/2014 10:20 PM, Igor Mammedov wrote: > ...... > + > + acpi_handle_ost_event(mdev); > _OST is optional and OSPM doesn't have to call it at all, > it was already discussed on list and using _OST for device removal > was evaluated as not usable. > We use _OST here as supplementary status information for management tools > and no more /i.e. as LEDs on real hardware/. > > See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec > and note below it. OK, I agree we should not make memory hotplug process depend on _OST. But we do need to handle one problem: When guest OS failed to remove device, we should not clear QEmu data of that device. As you and ACPI spec said, Platform (which is QEmu here) should reply _OST rised by guest OS except things such as LEDs on real hardware. But _OST is the only way I can find to get status from guest OS. So, let's do it in the following way: 1. Make device hotplug process independent from _OST for the compatibility reason. 2. For OSPMs that support _OST, QEmu use _OST to catch error from guest OS. May be report an error message only, and stop QEmu from remove related data. (Please see/review patch-set: https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html) 3. For OSPMs that do not support _OST, do not handle guest OS error for now. How do you think ? Thanks. > > >> 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 */ >> + mdev->is_enabled = false; > device tear-down should be initiated here, when OSPM calls _EJ0 method > and when we return from this function it should be destroyed. > >> } >> + >> + break; >> + default: >> break; >> } >> - >> } >> static const MemoryRegionOps acpi_memory_hotplug_ops = { >> .read = acpi_memory_hotplug_read, >> @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> DeviceState *dev, Error **errp) >> { >> + MemStatus *mdev; >> Error *local_err = NULL; >> int slot = object_property_get_int(OBJECT(dev), "slot", &local_err); >> >> @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, >> return; >> } >> >> + mdev = &mem_st->devs[slot]; >> + mdev->is_removing = true; >> + >> /* do ACPI magic */ >> ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS; >> acpi_update_sci(ar, irq); >> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h >> index 1f678b4..e105e45 100644 >> --- a/include/hw/acpi/acpi.h >> +++ b/include/hw/acpi/acpi.h >> @@ -91,6 +91,20 @@ >> /* PM2_CNT */ >> #define ACPI_BITMASK_ARB_DISABLE 0x0001 >> >> +/* OST_EVENT */ >> +#define ACPI_NOTIFY_EJECT_REQUEST 0x03 >> +#define ACPI_OSPM_EJECT 0x103 >> + >> +/* OST_STATUS */ >> +#define ACPI_SUCCESS 0x0 >> +#define ACPI_FAILURE 0x1 >> +#define ACPI_UNRECOGNIZED_NOTIFY 0x2 >> +#define ACPI_EJECT_NOT_SUPPORTED 0x80 >> +#define ACPI_EJECT_DEVICE_IN_USE 0x81 >> +#define ACPI_EJECT_DEVICE_BUSY 0x82 >> +#define ACPI_EJECT_DEPENDENCY_BUSY 0x83 >> +#define ACPI_EJECT_IN_PROGRESS 0x84 >> + >> /* structs */ >> typedef struct ACPIPMTimer ACPIPMTimer; >> typedef struct ACPIPM1EVT ACPIPM1EVT; >> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h >> index fc6b868..fe41268 100644 >> --- a/include/hw/acpi/memory_hotplug.h >> +++ b/include/hw/acpi/memory_hotplug.h >> @@ -11,6 +11,7 @@ typedef struct MemStatus { >> DeviceState *dimm; >> bool is_enabled; >> bool is_inserting; >> + bool is_removing; >> uint32_t ost_event; >> uint32_t ost_status; >> } MemStatus; > . >
On Tue, 16 Sep 2014 18:12:02 +0800 Tang Chen <tangchen@cn.fujitsu.com> wrote: > Hi Igor, > > On 09/04/2014 10:20 PM, Igor Mammedov wrote: > > > ...... > > + > > + acpi_handle_ost_event(mdev); > > _OST is optional and OSPM doesn't have to call it at all, > > it was already discussed on list and using _OST for device removal > > was evaluated as not usable. > > We use _OST here as supplementary status information for management tools > > and no more /i.e. as LEDs on real hardware/. > > > > See "Figure 6-37 Device Ejection Flow Example Using _OST." in ACPI 5.1 spec > > and note below it. > > OK, I agree we should not make memory hotplug process depend on _OST. > > But we do need to handle one problem: When guest OS failed to remove > device, > we should not clear QEmu data of that device. > > As you and ACPI spec said, Platform (which is QEmu here) should reply _OST > rised by guest OS except things such as LEDs on real hardware. But _OST is > the only way I can find to get status from guest OS. > > So, let's do it in the following way: > 1. Make device hotplug process independent from _OST for the > compatibility reason. > > 2. For OSPMs that support _OST, QEmu use _OST to catch error from guest OS. > May be report an error message only, and stop QEmu from remove > related data. > (Please see/review patch-set: > https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html) > > 3. For OSPMs that do not support _OST, do not handle guest OS error for now. > > How do you think ? I'd keep _OST handling in ACPI write handler only for notification purposes only. This way management tools/end user will see guest OS error if supported , otherwise we do not care. Make '_OST unplug handler' reset removing bit in case of error, so that guest won't see DIMM stuck in continuous removing state when following hotplug events happen. > > Thanks. > > > > > > >> 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 */ > >> + mdev->is_enabled = false; > > device tear-down should be initiated here, when OSPM calls _EJ0 method > > and when we return from this function it should be destroyed. > > > >> } > >> + > >> + break; > >> + default: > >> break; > >> } > >> - > >> } > >> static const MemoryRegionOps acpi_memory_hotplug_ops = { > >> .read = acpi_memory_hotplug_read, > >> @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > >> void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > >> DeviceState *dev, Error **errp) > >> { > >> + MemStatus *mdev; > >> Error *local_err = NULL; > >> int slot = object_property_get_int(OBJECT(dev), "slot", &local_err); > >> > >> @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, > >> return; > >> } > >> > >> + mdev = &mem_st->devs[slot]; > >> + mdev->is_removing = true; > >> + > >> /* do ACPI magic */ > >> ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS; > >> acpi_update_sci(ar, irq); > >> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h > >> index 1f678b4..e105e45 100644 > >> --- a/include/hw/acpi/acpi.h > >> +++ b/include/hw/acpi/acpi.h > >> @@ -91,6 +91,20 @@ > >> /* PM2_CNT */ > >> #define ACPI_BITMASK_ARB_DISABLE 0x0001 > >> > >> +/* OST_EVENT */ > >> +#define ACPI_NOTIFY_EJECT_REQUEST 0x03 > >> +#define ACPI_OSPM_EJECT 0x103 > >> + > >> +/* OST_STATUS */ > >> +#define ACPI_SUCCESS 0x0 > >> +#define ACPI_FAILURE 0x1 > >> +#define ACPI_UNRECOGNIZED_NOTIFY 0x2 > >> +#define ACPI_EJECT_NOT_SUPPORTED 0x80 > >> +#define ACPI_EJECT_DEVICE_IN_USE 0x81 > >> +#define ACPI_EJECT_DEVICE_BUSY 0x82 > >> +#define ACPI_EJECT_DEPENDENCY_BUSY 0x83 > >> +#define ACPI_EJECT_IN_PROGRESS 0x84 > >> + > >> /* structs */ > >> typedef struct ACPIPMTimer ACPIPMTimer; > >> typedef struct ACPIPM1EVT ACPIPM1EVT; > >> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h > >> index fc6b868..fe41268 100644 > >> --- a/include/hw/acpi/memory_hotplug.h > >> +++ b/include/hw/acpi/memory_hotplug.h > >> @@ -11,6 +11,7 @@ typedef struct MemStatus { > >> DeviceState *dimm; > >> bool is_enabled; > >> bool is_inserting; > >> + bool is_removing; > >> uint32_t ost_event; > >> uint32_t ost_status; > >> } MemStatus; > > . > > >
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index c310b44..9074e7c 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -75,6 +75,7 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr, case 0x14: /* pack and return is_* fields */ val |= mdev->is_enabled ? 1 : 0; val |= mdev->is_inserting ? 2 : 0; + val |= mdev->is_removing ? 4 : 0; trace_mhp_acpi_read_flags(mem_st->selector, val); break; default: @@ -84,6 +85,51 @@ static uint64_t acpi_memory_hotplug_read(void *opaque, hwaddr addr, return val; } +static void acpi_handle_eject(MemStatus *mdev) +{ + switch (mdev->ost_status) { + case ACPI_SUCCESS: + object_unparent(OBJECT(mdev->dimm)); + mdev->is_removing = false; + mdev->dimm = NULL; + break; + case ACPI_EJECT_IN_PROGRESS: + /* For ejection triggered by hardware (device_del command), + * mdev->is_removing is set to true by acpi_memory_unplug_cb() + * before sending SCI. So we only handle ejection triggered by + * guest OS. + */ + if (mdev->ost_event == ACPI_OSPM_EJECT) { + mdev->is_enabled = false; + mdev->is_removing = true; + } + break; + case ACPI_FAILURE: + case ACPI_UNRECOGNIZED_NOTIFY: + case ACPI_EJECT_NOT_SUPPORTED: + case ACPI_EJECT_DEVICE_IN_USE: + case ACPI_EJECT_DEVICE_BUSY: + case ACPI_EJECT_DEPENDENCY_BUSY: + mdev->is_removing = false; + mdev->is_enabled = true; + break; + default: + break; + } +} + +static void acpi_handle_ost_event(MemStatus *mdev) +{ + switch (mdev->ost_event) { + case ACPI_NOTIFY_EJECT_REQUEST: /* Ejection triggered by hardware. */ + case ACPI_OSPM_EJECT: /* Ejection triggered by guest OS. */ + acpi_handle_eject(mdev); + break; + default: + break; + } +} + static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data, unsigned int size) { @@ -121,21 +167,27 @@ 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); + + acpi_handle_ost_event(mdev); 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 */ + mdev->is_enabled = false; } + + break; + default: break; } - } static const MemoryRegionOps acpi_memory_hotplug_ops = { .read = acpi_memory_hotplug_read, @@ -198,6 +250,7 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, DeviceState *dev, Error **errp) { + MemStatus *mdev; Error *local_err = NULL; int slot = object_property_get_int(OBJECT(dev), "slot", &local_err); @@ -215,6 +268,9 @@ void acpi_memory_unplug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st, return; } + mdev = &mem_st->devs[slot]; + mdev->is_removing = true; + /* do ACPI magic */ ar->gpe.sts[0] |= ACPI_MEMORY_HOTPLUG_STATUS; acpi_update_sci(ar, irq); diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h index 1f678b4..e105e45 100644 --- a/include/hw/acpi/acpi.h +++ b/include/hw/acpi/acpi.h @@ -91,6 +91,20 @@ /* PM2_CNT */ #define ACPI_BITMASK_ARB_DISABLE 0x0001 +/* OST_EVENT */ +#define ACPI_NOTIFY_EJECT_REQUEST 0x03 +#define ACPI_OSPM_EJECT 0x103 + +/* OST_STATUS */ +#define ACPI_SUCCESS 0x0 +#define ACPI_FAILURE 0x1 +#define ACPI_UNRECOGNIZED_NOTIFY 0x2 +#define ACPI_EJECT_NOT_SUPPORTED 0x80 +#define ACPI_EJECT_DEVICE_IN_USE 0x81 +#define ACPI_EJECT_DEVICE_BUSY 0x82 +#define ACPI_EJECT_DEPENDENCY_BUSY 0x83 +#define ACPI_EJECT_IN_PROGRESS 0x84 + /* structs */ typedef struct ACPIPMTimer ACPIPMTimer; typedef struct ACPIPM1EVT ACPIPM1EVT; diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h index fc6b868..fe41268 100644 --- a/include/hw/acpi/memory_hotplug.h +++ b/include/hw/acpi/memory_hotplug.h @@ -11,6 +11,7 @@ typedef struct MemStatus { DeviceState *dimm; bool is_enabled; bool is_inserting; + bool is_removing; uint32_t ost_event; uint32_t ost_status; } MemStatus;