On 10/03/2016 09:48 PM, Igor Mammedov wrote:
On Fri, 12 Aug 2016 14:54:02 +0800
Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> 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!
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
