From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34720) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo5A2-0003kx-KC for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:47:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zo59y-0007sa-Ft for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:47:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57011) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo59y-0007sS-7q for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:47:10 -0400 Date: Mon, 19 Oct 2015 10:47:05 +0300 From: "Michael S. Tsirkin" Message-ID: <20151019104112-mutt-send-email-mst@redhat.com> References: <1444535584-18220-1-git-send-email-guangrong.xiao@linux.intel.com> <1444535584-18220-27-git-send-email-guangrong.xiao@linux.intel.com> <20151019095026-mutt-send-email-mst@redhat.com> <56249854.2090007@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56249854.2090007@linux.intel.com> Subject: Re: [Qemu-devel] [PATCH v3 26/32] nvdimm: save arg3 for NVDIMM device _DSM method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On Mon, Oct 19, 2015 at 03:14:28PM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 02:50 PM, Michael S. Tsirkin wrote: > >On Sun, Oct 11, 2015 at 11:52:58AM +0800, Xiao Guangrong wrote: > >>Check if the input Arg3 is valid then store it into dsm_in if needed > >> > >>We only do the save on NVDIMM device since we are not going to support any > >>function on root device > >> > >>Signed-off-by: Xiao Guangrong > >>--- > >> hw/mem/nvdimm/acpi.c | 21 ++++++++++++++++++++- > >> 1 file changed, 20 insertions(+), 1 deletion(-) > >> > >>diff --git a/hw/mem/nvdimm/acpi.c b/hw/mem/nvdimm/acpi.c > >>index d9fa0fd..3b9399c 100644 > >>--- a/hw/mem/nvdimm/acpi.c > >>+++ b/hw/mem/nvdimm/acpi.c > >>@@ -442,7 +442,7 @@ static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list, > >> int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, > >> NULL); > >> uint32_t handle = nvdimm_slot_to_handle(slot); > >>- Aml *dev, *method; > >>+ Aml *dev, *method, *ifctx; > >> > >> dev = aml_device("NV%02X", slot); > >> aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); > >>@@ -452,6 +452,24 @@ static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list, > >> method = aml_method("_DSM", 4); > >> { > >> SAVE_ARG012_HANDLE_LOCK(method, aml_int(handle)); > >>+ > >>+ /* Arg3 is passed as Package and it has one element? */ > >>+ ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), > >>+ aml_int(4)), > >>+ aml_equal(aml_sizeof(aml_arg(3)), > > > >aml_arg(3) is used many times below. > >Pls give it a name that makes sense (not arg3! what is it for?) > > > > Er. aml_arg(3) is just the fourth parameter of _DSM method. Will add some > comments: > > /* > * The fourth parameter (Arg3) of _DMS is a package which contains a buffer, the > * layout of the buffer is specified by UUID (Arg0), Revision ID (Arg1) and > * Function Index (Arg2) which are documented in the DSM specification. > */ > > >>+ aml_int(1)))); > > > >Pls document AML constants used. > >Like this: > > > > ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), > > aml_int(4 /* 4 - Package */) ), > > aml_equal(aml_sizeof(aml_arg(3)), > > aml_int(1)))); > > > >>+ { > >>+ /* Local0 = Index(Arg3, 0) */ > >>+ aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), > >>+ aml_local(0))); > >>+ /* Local3 = DeRefOf(Local0) */ > >>+ aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), > >>+ aml_local(3))); > >>+ /* ARG3 = Local3 */ > >>+ aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); > > > >This isn't a good way to comment things: you are > >just adding ASL before the equivalent C. > >Pls document what's going on. > > > > Okay... i just thought C is little readable than AML. Will change the comment > to: > > /* fetch buffer from the package (Arg3) and store it to DSM memory. */ > > Thanks. You can use variables to make the logic clear. E.g.: Aml *pckg = aml_arg(3); Aml *pckg_idx = aml_local(0); Aml *pckg_buf = aml_local(3); aml_append(ifctx, aml_store(aml_index(pckg, aml_int(0)), pckg_idx); aml_append(ifctx, aml_store(aml_derefof(pckg_idx), pckg_buf)); This is also better than repeating aml_arg(3) many times. -- MST