From mboxrd@z Thu Jan  1 00:00:00 1970
From: Igor Mammedov <imammedo@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support
Date: Mon, 3 Oct 2016 15:48:00 +0200
Message-ID: <20161003154800.17e3ba30@nial.brq.redhat.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
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: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:53672 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752367AbcJCNsF (ORCPT <rfc822;kvm@vger.kernel.org>);
        Mon, 3 Oct 2016 09:48:05 -0400
In-Reply-To: <1470984850-66891-1-git-send-email-guangrong.xiao@linux.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Fri, 12 Aug 2016 14:54:02 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> 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.