From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36721) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm3QF-0007tU-9i for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:31:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm3QB-0007uN-2J for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:31:35 -0400 Received: from mga09.intel.com ([134.134.136.24]:11426) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm3QA-0007t5-LZ for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:31:30 -0400 References: <1444535584-18220-1-git-send-email-guangrong.xiao@linux.intel.com> <1444535584-18220-26-git-send-email-guangrong.xiao@linux.intel.com> <20151013163929.493c0669@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <561D3E66.9000909@linux.intel.com> Date: Wed, 14 Oct 2015 01:24:54 +0800 MIME-Version: 1.0 In-Reply-To: <20151013163929.493c0669@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 25/32] nvdimm: build ACPI nvdimm devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 10/13/2015 10:39 PM, Igor Mammedov wrote: > On Sun, 11 Oct 2015 11:52:57 +0800 > Xiao Guangrong wrote: > >> NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices >> >> There is a root device under \_SB and specified NVDIMM devices are under the >> root device. Each NVDIMM device has _ADR which returns its handle used to >> associate MEMDEV structure in NFIT >> >> We reserve handle 0 for root device. In this patch, we save handle, arg0, >> arg1 and arg2. Arg3 is conditionally saved in later patch >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/mem/nvdimm/acpi.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 203 insertions(+) >> >> diff --git a/hw/mem/nvdimm/acpi.c b/hw/mem/nvdimm/acpi.c > I'd suggest to put ACPI parts to hw/acpi/nvdimm.c file so that ACPI > maintainers won't miss changes to this files. > Sounds reasonable to me. > >> index 1450a6a..d9fa0fd 100644 >> --- a/hw/mem/nvdimm/acpi.c >> +++ b/hw/mem/nvdimm/acpi.c >> @@ -308,15 +308,38 @@ static void build_nfit(void *fit, GSList *device_list, GArray *table_offsets, >> "NFIT", table_data->len - nfit_start, 1); >> } >> >> +#define NOTIFY_VALUE 0x99 >> + >> +struct dsm_in { >> + uint32_t handle; >> + uint8_t arg0[16]; >> + uint32_t arg1; >> + uint32_t arg2; >> + /* the remaining size in the page is used by arg3. */ >> + uint8_t arg3[0]; >> +} QEMU_PACKED; >> +typedef struct dsm_in dsm_in; >> + >> +struct dsm_out { >> + /* the size of buffer filled by QEMU. */ >> + uint16_t len; >> + uint8_t data[0]; >> +} QEMU_PACKED; >> +typedef struct dsm_out dsm_out; >> + >> static uint64_t dsm_read(void *opaque, hwaddr addr, >> unsigned size) >> { >> + fprintf(stderr, "BUG: we never read DSM notification MMIO.\n"); >> return 0; >> } >> >> static void dsm_write(void *opaque, hwaddr addr, >> uint64_t val, unsigned size) >> { >> + if (val != NOTIFY_VALUE) { >> + fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val); >> + } >> } >> >> static const MemoryRegionOps dsm_ops = { >> @@ -372,6 +395,183 @@ static MemoryRegion *build_dsm_memory(NVDIMMState *state) >> return dsm_fit_mr; >> } >> >> +#define BUILD_STA_METHOD(_dev_, _method_) \ >> + do { \ >> + _method_ = aml_method("_STA", 0); \ >> + aml_append(_method_, aml_return(aml_int(0x0f))); \ >> + aml_append(_dev_, _method_); \ >> + } while (0) >> + >> +#define SAVE_ARG012_HANDLE_LOCK(_method_, _handle_) \ >> + do { \ >> + aml_append(_method_, aml_acquire(aml_name("NLCK"), 0xFFFF)); \ > how about making method serialized, then you could drop explicit lock/unlock logic > for that you'd need to extend existing aml_method() to something like this: > > aml_method("FOO", 3/*count*/, AML_SERIALIZED, 0 /* sync_level */) I am not sure if multiple methods under different namespace objects can be serialized, for example: Device("__D0") { Method("FOO", 3, AML_SERIALIZED, 0) { BUF = Arg0 } } Device("__D1") { Method("FOO", 3, AML_SERIALIZED, 0) { BUF = Arg0 } } __D0.FOO and __D1.FOO can be serialized? Your suggestion definitely valuable to me, i will abstract the access of shared-memory into one method as your comment below. > >> + aml_append(_method_, aml_store(_handle_, aml_name("HDLE"))); \ >> + aml_append(_method_, aml_store(aml_arg(0), aml_name("ARG0"))); \ > Could you describe QEMU<->ASL interface in a separate spec > file (for example like: docs/specs/acpi_mem_hotplug.txt), > it will help to with review process as there will be something to compare > patches with. > Once that is finalized/agreed upon, it should be easy to review and probably > to write corresponding patches. Sure, i considered it too and was planing to make this kind of spec after this patchset is merged... I will document the interface in the next version. > > Also I'd try to minimize QEMU<->ASL interface and implement as much as possible > of ASL logic in AML instead of pushing it in hardware (QEMU). Okay, i agree. Since ACPI ASL/AML is new knowledge to me, i did it using the opposite way - move the control to QEMU side as possible ... :) > For example there isn't really any need to tell QEMU ARG0 (UUID), _DSM method > could just compare UUIDs itself and execute a corresponding branch. > Probably something else could be optimized as well but that we can find out > during discussion over QEMU<->ASL interface spec. Okay. > >> + aml_append(_method_, aml_store(aml_arg(1), aml_name("ARG1"))); \ >> + aml_append(_method_, aml_store(aml_arg(2), aml_name("ARG2"))); \ >> + } while (0) >> + >> +#define NOTIFY_AND_RETURN_UNLOCK(_method_) \ >> + do { \ >> + aml_append(_method_, aml_store(aml_int(NOTIFY_VALUE), \ >> + aml_name("NOTI"))); \ >> + aml_append(_method_, aml_store(aml_name("RLEN"), aml_local(6))); \ >> + aml_append(_method_, aml_store(aml_shiftleft(aml_local(6), \ >> + aml_int(3)), aml_local(6))); \ >> + aml_append(_method_, aml_create_field(aml_name("ODAT"), aml_int(0),\ >> + aml_local(6) , "OBUF")); \ >> + aml_append(_method_, aml_name_decl("ZBUF", aml_buffer(0, NULL))); \ >> + aml_append(_method_, aml_concatenate(aml_name("ZBUF"), \ >> + aml_name("OBUF"), aml_arg(6))); \ >> + aml_append(_method_, aml_release(aml_name("NLCK"))); \ >> + aml_append(_method_, aml_return(aml_arg(6))); \ >> + } while (0) >> + >> +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ >> + aml_append(_field_, aml_named_field(_name_, \ >> + sizeof(typeof_field(_s_, _f_)) * BITS_PER_BYTE)) >> + >> +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ >> + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) >> + >> +static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list, >> + Aml *root_dev) >> +{ >> + for (; device_list; device_list = device_list->next) { >> + NVDIMMDevice *nvdimm = device_list->data; >> + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, >> + NULL); >> + uint32_t handle = nvdimm_slot_to_handle(slot); >> + Aml *dev, *method; >> + >> + dev = aml_device("NV%02X", slot); >> + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); >> + >> + BUILD_STA_METHOD(dev, method); >> + >> + method = aml_method("_DSM", 4); > That will create the same method per each device which increases > ACPI table size unnecessarily. > I'd suggest to make per nvdimm device method a wrapper over common > NVDR._DSM method and make the later handle all the logic. Good to me. Really appropriate for your review, Igor!