From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51174) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bta39-0007Ee-AG for qemu-devel@nongnu.org; Mon, 10 Oct 2016 08:51:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bta33-0005ye-Nc for qemu-devel@nongnu.org; Mon, 10 Oct 2016 08:51:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38062) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bta33-0005y8-Dd for qemu-devel@nongnu.org; Mon, 10 Oct 2016 08:51:17 -0400 Date: Mon, 10 Oct 2016 14:51:13 +0200 From: Igor Mammedov Message-ID: <20161010145113.71229994@nial.brq.redhat.com> In-Reply-To: <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> <88933673-0afc-7389-f792-43bd7f6a1bcc@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Xiao Guangrong 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 Sat, 8 Oct 2016 15:17:14 +0800 Xiao Guangrong wrote: > 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. under spec, I've meant ACPI spec where this UUID is declared > > > > >> > >> 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? ok, leave it as is. > > > >