From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v2 3/8] nvdimm acpi: introduce _FIT Date: Sat, 8 Oct 2016 15:17:14 +0800 Message-ID: <88933673-0afc-7389-f792-43bd7f6a1bcc@linux.intel.com> References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-4-git-send-email-guangrong.xiao@linux.intel.com> <20160930151422.6327c7d1@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org To: Igor Mammedov Return-path: Received: from mga04.intel.com ([192.55.52.120]:29847 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750742AbcJHHXq (ORCPT ); Sat, 8 Oct 2016 03:23:46 -0400 In-Reply-To: <20160930151422.6327c7d1@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 09/30/2016 09:14 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:05 +0800 > Xiao Guangrong wrote: > >> _FIT is required for hotplug support, guest will inquire the updated >> device info from it if a hotplug event is received >> >> As FIT buffer is not completely mapped into guest address space, so a >> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved >> by QEMU to read the piece of FIT buffer. The buffer is concatenated >> before _FIT return > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > 0xFFFFFFFF for some purposes. > So spec should be amended first or custom generated UUID should be used. Okay. I will change the changelog to reflect this fact and move the spec update to this patch. > >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > and amend docs to reflect that. Already done in the spec, i will merge the spec changes into this patch as you suggested later. > >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 82 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 0e2b9f0..4bbd1e7 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) >> aml_append(dev, method); >> } >> >> +static void nvdimm_build_fit(Aml *dev) >> +{ >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; >> + Aml *whilectx, *ifcond, *ifctx, *fit; >> + >> + buf = aml_local(0); >> + buf_size = aml_local(1); >> + fit = aml_local(2); >> + >> + /* build helper function, RFIT. */ >> + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); > since you create named fields (global variable) in method scope, > you should make method serialized. Same goes for _FIT method. Indeed, will fix. > > >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), "OFST")); >> + >> + /* prepare input package. */ >> + pkg = aml_package(1); >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); >> + aml_append(pkg, aml_name("OFST")); >> + >> + /* call Read_FIT function. */ >> + call_result = aml_call5(NVDIMM_COMMON_DSM, >> + aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> + /* UUID for NVDIMM Root Device */), >> + aml_int(1) /* Revision 1 */, >> + aml_int(0xFFFFFFFF) /* Read FIT. */, >> + pkg, aml_int(0) /* for root device. */); >> + aml_append(method, aml_store(call_result, buf)); >> + >> + /* handle _DSM result. */ >> + aml_append(method, aml_create_dword_field(buf, >> + aml_int(0) /* offset at byte 0 */, "STAU")); >> + >> + /* if something is wrong during _DSM. */ >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); >> + ifctx = aml_if(aml_lnot(ifcond)); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); >> + aml_append(method, aml_subtract(buf_size, >> + aml_int(4) /* the size of "STAU" */, >> + buf_size)); > > Since you handle error case the same as EOF case you could replace > it with EOF case here and on qemu side of interface as well. That should > simplify code a bit as you won't need to strip out func_ret_status. > You mean returning NULL buffer if errors happen? However, the buffer is generated by NVDIMM_COMMON_DSM function which is also used by _DSM based on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway no matter is successful or failed. Or i missed your idea? From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57784) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bslz8-0003d0-Q9 for qemu-devel@nongnu.org; Sat, 08 Oct 2016 03:23:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bslz2-0002iG-Nw for qemu-devel@nongnu.org; Sat, 08 Oct 2016 03:23:53 -0400 Received: from mga05.intel.com ([192.55.52.43]:16333) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bslz2-0002gc-DU for qemu-devel@nongnu.org; Sat, 08 Oct 2016 03:23:48 -0400 References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <1470984850-66891-4-git-send-email-guangrong.xiao@linux.intel.com> <20160930151422.6327c7d1@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <88933673-0afc-7389-f792-43bd7f6a1bcc@linux.intel.com> Date: Sat, 8 Oct 2016 15:17:14 +0800 MIME-Version: 1.0 In-Reply-To: <20160930151422.6327c7d1@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/8] nvdimm acpi: introduce _FIT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org On 09/30/2016 09:14 PM, Igor Mammedov wrote: > On Fri, 12 Aug 2016 14:54:05 +0800 > Xiao Guangrong wrote: > >> _FIT is required for hotplug support, guest will inquire the updated >> device info from it if a hotplug event is received >> >> As FIT buffer is not completely mapped into guest address space, so a >> new function, Read FIT whose function index is 0xFFFFFFFF, is reserved >> by QEMU to read the piece of FIT buffer. The buffer is concatenated >> before _FIT return > Only issuer of UUID 2F10E7A4-9E91-11E4-89D3-123B93F75CBA can reserve > 0xFFFFFFFF for some purposes. > So spec should be amended first or custom generated UUID should be used. Okay. I will change the changelog to reflect this fact and move the spec update to this patch. > >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > and amend docs to reflect that. Already done in the spec, i will merge the spec changes into this patch as you suggested later. > >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/acpi/nvdimm.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 82 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 0e2b9f0..4bbd1e7 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -886,6 +886,87 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) >> aml_append(dev, method); >> } >> >> +static void nvdimm_build_fit(Aml *dev) >> +{ >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; >> + Aml *whilectx, *ifcond, *ifctx, *fit; >> + >> + buf = aml_local(0); >> + buf_size = aml_local(1); >> + fit = aml_local(2); >> + >> + /* build helper function, RFIT. */ >> + method = aml_method("RFIT", 1, AML_NOTSERIALIZED); > since you create named fields (global variable) in method scope, > you should make method serialized. Same goes for _FIT method. Indeed, will fix. > > >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), "OFST")); >> + >> + /* prepare input package. */ >> + pkg = aml_package(1); >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); >> + aml_append(pkg, aml_name("OFST")); >> + >> + /* call Read_FIT function. */ >> + call_result = aml_call5(NVDIMM_COMMON_DSM, >> + aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> + /* UUID for NVDIMM Root Device */), >> + aml_int(1) /* Revision 1 */, >> + aml_int(0xFFFFFFFF) /* Read FIT. */, >> + pkg, aml_int(0) /* for root device. */); >> + aml_append(method, aml_store(call_result, buf)); >> + >> + /* handle _DSM result. */ >> + aml_append(method, aml_create_dword_field(buf, >> + aml_int(0) /* offset at byte 0 */, "STAU")); >> + >> + /* if something is wrong during _DSM. */ >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); >> + ifctx = aml_if(aml_lnot(ifcond)); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); >> + aml_append(method, aml_subtract(buf_size, >> + aml_int(4) /* the size of "STAU" */, >> + buf_size)); > > Since you handle error case the same as EOF case you could replace > it with EOF case here and on qemu side of interface as well. That should > simplify code a bit as you won't need to strip out func_ret_status. > You mean returning NULL buffer if errors happen? However, the buffer is generated by NVDIMM_COMMON_DSM function which is also used by _DSM based on NVDIMM DSN specification, i.e, the 'func_ret_status' is needed anyway no matter is successful or failed. Or i missed your idea?