On Tue, 16 Sep 2014 18:12:02 +0800 Tang Chen <tangc...@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; > > . > > >