From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44777) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTpig-0005l0-Rp for qemu-devel@nongnu.org; Tue, 16 Sep 2014 06:10:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XTpib-0006bi-F9 for qemu-devel@nongnu.org; Tue, 16 Sep 2014 06:10:46 -0400 Received: from [59.151.112.132] (port=43024 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XTpia-0006Yd-8H for qemu-devel@nongnu.org; Tue, 16 Sep 2014 06:10:41 -0400 Message-ID: <54180CF2.3080405@cn.fujitsu.com> Date: Tue, 16 Sep 2014 18:12:02 +0800 From: Tang Chen MIME-Version: 1.0 References: <1409126919-22233-1-git-send-email-tangchen@cn.fujitsu.com> <1409126919-22233-7-git-send-email-tangchen@cn.fujitsu.com> <20140904162026.06aeb332@nial.usersys.redhat.com> In-Reply-To: <20140904162026.06aeb332@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v3 6/8] acpi: Add hardware implementation for memory hot unplug. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: zhugh.fnst@cn.fujitsu.com, mst@redhat.com, hutao@cn.fujitsu.com, qemu-devel@nongnu.org, isimatu.yasuaki@jp.fujitsu.com, pbonzini@redhat.com 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; > . >