From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support Date: Sat, 8 Oct 2016 16:34:06 +0800 Message-ID: References: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com> <20161003154800.17e3ba30@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, 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 To: Igor Mammedov Return-path: Received: from mga11.intel.com ([192.55.52.93]:43144 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757928AbcJHIkj (ORCPT ); Sat, 8 Oct 2016 04:40:39 -0400 In-Reply-To: <20161003154800.17e3ba30@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 10/03/2016 09:48 PM, Igor Mammedov wrote: > 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. Explicitly sync up QEMU and OSPM is hard as OSPM is running in a VM which is vulnerable and can not be expected always triggering correct sequence. So how about introducing generation-number which is updated for each hotplug event and a new DSM function reserved to get this number by OSPM. Then the ACPI code is like this: restart: old-number = DSM(Read_NUM); FIT = DSM(FIT) new-number = DSM(Read_NUM); if (old-number != new-number) goto restart; return FIT > --- > I guess I'm done with v2 review at this point. It is a hard work to review ACPI changes. Thank you, Igor! > > 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. Er, i tried windows guests which do not trigger the issue. > > 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) > You are right, will fix it. > > 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) */ > Good to me, will fix. > 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. > Will fix it too. > > 4: > if method creates/access named fields it should be serialized. > Yup, will pay more attention on it. Thanks!