From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34130) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br3bI-0007Qt-3v for qemu-devel@nongnu.org; Mon, 03 Oct 2016 09:48:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br3bC-0001gk-1s for qemu-devel@nongnu.org; Mon, 03 Oct 2016 09:48:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33560) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br3bB-0001gX-RM for qemu-devel@nongnu.org; Mon, 03 Oct 2016 09:48:05 -0400 Date: Mon, 3 Oct 2016 15:48:00 +0200 From: Igor Mammedov Message-ID: <20161003154800.17e3ba30@nial.brq.redhat.com> In-Reply-To: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: pbonzini@redhat.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On Fri, 12 Aug 2016 14:54:02 +0800 Xiao Guangrong wrote: General design issue in this series is regenerating _FIT data every time inside of _FIT read loop. The issue here is that if FIT data doesn't fit in one page RFIT would be called several times resulting in calling nvdimm_dsm_func_read_fit() N-times, and in between these N exits generated FIT data could change (for example if another NVDIMM has been hotpluged in between) resulting in corrupted concatenated FIT buffer returned by _FIT method. So one need either to block follow up hotplug or make sure that FIT data regenerated/updated only once per _FIT invocation and stay immutable until _FIT is completed (maybe RCU protected buffer). Regenerating/updating inside qemu could also be shared with NFIT table creation path so that both cold/hotplug would reuse the same data which are updated only when a NVDIMM is added/removed. --- I guess I'm done with v2 review at this point. PS: Not related to this series but still existing NVDIMM codebase issues: 1: OperationRegion (NRAM, SystemMemory, MEMA, 0x1000) ... Name (MEMA, 0x7FFFF000) is not valid ASL, and most certainly would make Windows BSOD. Check spec for RegionOffset := TermArg => Integer Named object is not a TermArg. I'd suggest to make that OperationRegion dynamic i.e put its definition into sole user NCAL() and use Store(MEMA, LocalX) OperationRegion (NRAM, SystemMemory, LocalX, 0x1000) 2: Field (NRAM, DWordAcc, NoLock, Preserve) { ... ARG3, 32672 } ... NCAL() ... Store (Local3, ARG3) /* \_SB_.NVDR.ARG3 */ Using ARG3 name is confusing at best and is wrong as ARG3 is reserved name and probably it won't compile back to valid AML. Suggest s/ARG3/FARG/ with comment at declaration point /* Package that contains function-specific arguments _DSM(..., Arg3) */ 3: Method (NCAL, 5, Serialized) { ... Concatenate (Buffer (Zero) {}, OBUF, Arg6) Return (Arg6) it's wrong to use Arg6 here as function has only 5 arguments, use LocalX instead. 4: if method creates/access named fields it should be serialized.