Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 10/03/2016 09:48 PM, Igor Mammedov wrote:
> 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.

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!