[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v2 0/8] nvdimm: hotplug support |
Date: |
Mon, 10 Oct 2016 14:59:31 +0200 |
On Sat, 8 Oct 2016 16:34:06 +0800
Xiao Guangrong <address@hidden> wrote:
> On 10/03/2016 09:48 PM, Igor Mammedov wrote:
> > On Fri, 12 Aug 2016 14:54:02 +0800
> > Xiao Guangrong <address@hidden> 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!
>