From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support Date: Mon, 10 Oct 2016 14:59:31 +0200 Message-ID: <20161010145931.59c0c5bb@nial.brq.redhat.com> 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=US-ASCII Content-Transfer-Encoding: 7bit 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 To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:41526 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752218AbcJJM7f (ORCPT ); Mon, 10 Oct 2016 08:59:35 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Sat, 8 Oct 2016 16:34:06 +0800 Xiao Guangrong wrote: > 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. try to take into account this point so that FIT data aren't regenerated on every RFIT invocation. > 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 It's a bit complicated. How about returning func_ret_status = FAIL_FIT_CHANGED if current FIT data changed underfoot and restarting. > > > --- > > 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! >