[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device
From: |
Robert Hoo |
Subject: |
Re: [PATCH 1/2] acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device |
Date: |
Fri, 29 Apr 2022 17:01:47 +0800 |
On Wed, 2022-04-27 at 16:34 +0200, Igor Mammedov wrote:
> On Tue, 12 Apr 2022 14:57:52 +0800
> Robert Hoo <robert.hu@linux.intel.com> wrote:
>
> > Since ACPI 6.2, previous NVDIMM/_DSM funcions "Get Namespace Label
> > Data
> > Size (function index 4)", "Get Namespace Label Data (function index
> > 5)",
> > "Set Namespace Label Data (function index 6)" has been deprecated
> > by ACPI
>
> where it's said that old way was deprecated, should be mentioned here
> including
> pointer to spec where it came into effect.
OK. https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
3.10 Deprecated Functions.
I put it in cover letter. Will also mention it here.
>
...
> >
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 0d43da19ea..7cc419401b 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -848,10 +848,10 @@ nvdimm_dsm_write(void *opaque, hwaddr addr,
> > uint64_t val, unsigned size)
> >
> > nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n",
> > in->revision,
> > in->handle, in->function);
> > -
> > - if (in->revision != 0x1 /* Currently we only support DSM Spec
> > Rev1. */) {
> > - nvdimm_debug("Revision 0x%x is not supported, expect
> > 0x%x.\n",
> > - in->revision, 0x1);
> > + /* Currently we only support DSM Spec Rev1 and Rev2. */
>
> where does revision 2 come from? It would be better to add a pointer
> to relevant spec.
https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
Section 3 "_DSM Interface for the NVDIMM Device", table 3-A and 3-B.
I'll add this in comments in next version.
>
> > + if (in->revision != 0x1 && in->revision != 0x2) {
> > + nvdimm_debug("Revision 0x%x is not supported, expect 0x1
> > or 0x2.\n",
> > + in->revision);
>
> since you are touching nvdimm_debug(), please replace it with
> tracing,
> see docs/devel/tracing.rst and any commit that adds tracing calls
> (functions starting with 'trace_').
OK I'll have a try.
>
> > nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > dsm_mem_addr);
> > goto exit;
> > }
>
>
> this whole hunk should be a separate patch, properly documented
>
OK
>
> also I wonder if DSM
It's not in SDM, but above-mentioned _DSM Interface spec by Intel.
>
> > @@ -1247,6 +1247,11 @@ static void nvdimm_build_fit(Aml *dev)
> > static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t
> > ram_slots)
> > {
> > uint32_t slot;
> > + Aml *method, *pkg, *buff;
> > +
> > + /* Build common shared buffer for params pass in/out */
> > + buff = aml_buffer(4096, NULL);
> > + aml_append(root_dev, aml_name_decl("BUFF", buff));
>
> is there a reason to use global variable instead of LocalX?
Local in root_dev but global to its sub devices? I think it is doable.
But given your below comments on return param _LS{I,R,W}, I now think,
in v2, I'm not going to reuse existing "NCAL" method, but implement
_LS{I,R,W} their own, stringently follow interface spec. Then, no buff
required at all. How do you like this?
>
> >
> > for (slot = 0; slot < ram_slots; slot++) {
> > uint32_t handle = nvdimm_slot_to_handle(slot);
> > @@ -1264,6 +1269,49 @@ static void nvdimm_build_nvdimm_devices(Aml
> > *root_dev, uint32_t ram_slots)
> > */
> > aml_append(nvdimm_dev, aml_name_decl("_ADR",
> > aml_int(handle)));
> >
> > + /* Build _LSI, _LSR, _LSW */
>
> should be 1 comment per method with spec/ver and chapter where it's
> defined
OK
>
> > + method = aml_method("_LSI", 0, AML_NOTSERIALIZED);
> > + aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> > + aml_touuid("4309AC30-0D11-11E4-9191-
> > 0800200C9A66"),
> > + aml_int(2), aml_int(4), aml_int(0),
> > + aml_int(handle))));
> > + aml_append(nvdimm_dev, method);
>
> _LSI should return Package
Right. See above.
>
> > + method = aml_method("_LSR", 2, AML_SERIALIZED);
> > + aml_append(method,
> > + aml_create_dword_field(aml_name("BUFF"), aml_int(0),
> > "DWD0"));
> > + aml_append(method,
> > + aml_create_dword_field(aml_name("BUFF"), aml_int(4),
> > "DWD1"));
>
> theoretically aml_create_dword_field() takes TermArg as source
> buffer,
> so it doesn't have to be a global named buffer.
> Try keep buffer in LocalX variable and check if it works in earliest
> Windows version that supports NVDIMMs. If it does then drop BUFF and
> use
> Local variable, if not then that fact should be mentioned in commit
> message/patch
Thanks Igor. I'm new to asl grammar, I'll take your advice.
>
> > + pkg = aml_package(1);
> > + aml_append(pkg, aml_name("BUFF"));
> > + aml_append(method, aml_name_decl("PKG1", pkg));
> > + aml_append(method, aml_store(aml_arg(0),
> > aml_name("DWD0")));
> > + aml_append(method, aml_store(aml_arg(1),
> > aml_name("DWD1")));
>
> perhaps use less magical names for fields, something like:
> DOFF
> TLEN
> add appropriate comments
No problem.
>
> Also I'd prepare/fill in buffer first and only then declare
> initialize
> Package + don't use named object for Package if it can be done with
> help
> of Local variables.
>
> > + aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> > + aml_touuid("4309AC30-0D11-11E4-9191-
> > 0800200C9A66"),
> > + aml_int(2), aml_int(5),
> > aml_name("PKG1"),
> > + aml_int(handle))));
>
> this shall return Package not a Buffer
Right, Going to re-implement these methods rather than wrapper NCAL.
>
> > + aml_append(nvdimm_dev, method);
> > +
> > + method = aml_method("_LSW", 3, AML_SERIALIZED);
> > + aml_append(method,
> > + aml_create_dword_field(aml_name("BUFF"), aml_int(0),
> > "DWD0"));
> > + aml_append(method,
> > + aml_create_dword_field(aml_name("BUFF"), aml_int(4),
> > "DWD1"));
> > + aml_append(method,
> > + aml_create_field(aml_name("BUFF"), aml_int(64),
> > aml_int(32672), "FILD"));
> > + pkg = aml_package(1);
> > + aml_append(pkg, aml_name("BUFF"));
> > + aml_append(method, aml_name_decl("PKG1", pkg));
> > + aml_append(method, aml_store(aml_arg(0),
> > aml_name("DWD0")));
> > + aml_append(method, aml_store(aml_arg(1),
> > aml_name("DWD1")));
> > + aml_append(method, aml_store(aml_arg(2),
> > aml_name("FILD")));
> > + aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> > + aml_touuid("4309AC30-0D11-11E4-9191-
> > 0800200C9A66"),
> > + aml_int(2), aml_int(6),
> > aml_name("PKG1"),
> > + aml_int(handle))));
>
> should return Integer not Buffer, it looks like implicit conversion
> will take care of it,
> but it would be better to explicitly convert it to Integer even if
> it's only for the sake
> of documenting expected return value (or add a comment)
I observed guest kernel ACPI component complaining this; just warning,
no harm. I'll re-implement it.
>
> Also returned value in case of error NVDIMM_DSM_RET_STATUS_INVALID,
> in NVDIMM and ACPI spec differ. So fix the spec or remap returned
> value.
>
>
> > + aml_append(nvdimm_dev, method);
> > +
> > nvdimm_build_device_dsm(nvdimm_dev, handle);
> > aml_append(root_dev, nvdimm_dev);
> > }
>
>
- [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods, Robert Hoo, 2022/04/12
- [PATCH 1/2] acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device, Robert Hoo, 2022/04/12
- [PATCH 2/2] acpi/nvdimm: Fix aml_or() and aml_and() in if clause, Robert Hoo, 2022/04/12
- Re: [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods, Robert Hoo, 2022/04/20
- Re: [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods, Igor Mammedov, 2022/04/27