| Message ID | 20220412065753.3216538-2-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi/nvdimm: support NVDIMM _LS{I,R,W} methods | expand |
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. > standard method _LSI, _LSR, _LSW respectively. Functions semantics are > almost identical, so my implementation is to reuse existing _DSMs, just > create _LS{I,R,W} interfaces and constructs parameters and call _DSMs. > > Only child NVDIMM devices has these methods, rather Root device. > > By this patch, new NVDIMM sub device in ACPI namespace will be like this: > > Device (NV00) > { > Name (_ADR, One) // _ADR: Address > Method (_LSI, 0, NotSerialized) // _LSI: Label Storage Information > { > Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x04, Zero, One)) > } > > Method (_LSR, 2, Serialized) // _LSR: Label Storage Read > { > CreateDWordField (BUFF, Zero, DWD0) > CreateDWordField (BUFF, 0x04, DWD1) > Name (PKG1, Package (0x01) > { > BUFF > }) > DWD0 = Arg0 > DWD1 = Arg1 > Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x05, PKG1, One)) > } > > Method (_LSW, 3, Serialized) // _LSW: Label Storage Write > { > CreateDWordField (BUFF, Zero, DWD0) > CreateDWordField (BUFF, 0x04, DWD1) > CreateField (BUFF, 0x40, 0x7FA0, FILD) > Name (PKG1, Package (0x01) > { > BUFF > }) > DWD0 = Arg0 > DWD1 = Arg1 > FILD = Arg2 > Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x06, PKG1, One)) > } > > Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method > { > Return (NCAL (Arg0, Arg1, Arg2, Arg3, One)) > } > } > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > Reviewed-by: Jingqi Liu<jingqi.liu@intel.com> > --- > hw/acpi/nvdimm.c | 56 ++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 4 deletions(-) > > 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. > + 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_'). > nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr); > goto exit; > } this whole hunk should be a separate patch, properly documented also I wonder if DSM > @@ -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? > > 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 > + 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 > + 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 > + 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 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 > + 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) 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); > }
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); > > } > >
On Fri, 29 Apr 2022 17:01:47 +0800 Robert Hoo <robert.hu@linux.intel.com> wrote: > 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. just make conversion a separate patch > > > > > 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. wrapper is fine, you just need to repackage content of the Buffer into a Package > > > > > + 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. try to test it with Windows guest (it usually less tolerable to errors than Linux + it's own quirks that you need to carter to) Also it would e nice if you test and put results in cover letter not only for Linux but for Windows as well. > > > > 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); > > > } > > > > >
On Tue, 2022-05-03 at 10:27 +0200, Igor Mammedov wrote: > On Fri, 29 Apr 2022 17:01:47 +0800 > Robert Hoo <robert.hu@linux.intel.com> wrote: > > > 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. > > just make conversion a separate patch Yeah, I supposed so too. > > > > > > > > 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. > > wrapper is fine, you just need to repackage content of the Buffer > into a Package > I now prefer re-implementation, i.e. make _LS{I,R,W} their own functions, less NACL's burden and don't make it more complex, it's already not neat; and more point, I think by this we can save the 4K Buff at all. Does this sound all right to you? > > > > > > > + 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. > > try to test it with Windows guest (it usually less tolerable to > errors > than Linux + it's own quirks that you need to carter to) > Also it would e nice if you test and put results in cover letter > not only for Linux but for Windows as well. > I'll try to, but have no Windows resource at hand, I'll ask around if any test resource to cover that. > > > > > > 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); > > > > } > > > > > > > >
On Thu, 05 May 2022 11:07:53 +0800 Robert Hoo <robert.hu@linux.intel.com> wrote: > On Tue, 2022-05-03 at 10:27 +0200, Igor Mammedov wrote: > > On Fri, 29 Apr 2022 17:01:47 +0800 > > Robert Hoo <robert.hu@linux.intel.com> wrote: > > > > > 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. > > > > just make conversion a separate patch > > Yeah, I supposed so too. > > > > > > > > > > > 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. > > > > wrapper is fine, you just need to repackage content of the Buffer > > into a Package > > > I now prefer re-implementation, i.e. make _LS{I,R,W} their own > functions, less NACL's burden and don't make it more complex, it's > already not neat; and more point, I think by this we can save the 4K > Buff at all. > Does this sound all right to you? On one hand what you propose will be bit simpler (but not mach) than repacking (and only on AML guest side), however it will add to host side an extra interface/ABI that we will have to maintain, also it won't save space as buffer will still be there for legacy interface. So unless we have to add new host/guest ABI, I'd prefer reusing existing one and complicate only new _LS{I,R,W} AML without touching NACL or host side. > > > > > > > > > > + 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. > > > > try to test it with Windows guest (it usually less tolerable to > > errors > > than Linux + it's own quirks that you need to carter to) > > Also it would e nice if you test and put results in cover letter > > not only for Linux but for Windows as well. > > > I'll try to, but have no Windows resource at hand, I'll ask around if > any test resource to cover that. > > > > > > > > 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); > > > > > } > > > > > > > > > > > > >
On Thu, 2022-05-05 at 10:50 +0200, Igor Mammedov wrote: ... > > > > > > @@ -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? > > > > > ... > > > > > > > > > > > + 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. > > > > > > wrapper is fine, you just need to repackage content of the Buffer > > > into a Package > > > > > > > I now prefer re-implementation, i.e. make _LS{I,R,W} their own > > functions, less NACL's burden and don't make it more complex, it's > > already not neat; and more point, I think by this we can save the > > 4K > > Buff at all. > > Does this sound all right to you? > > On one hand what you propose will be bit simpler (but not mach) than > repacking (and only on AML guest side), however it will add to host > side an extra interface/ABI that we will have to maintain, also it > won't save space as buffer will still be there for legacy interface. No, sorry, I didn't explain it clear. No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-device methods. Of course, I'm going to extract 'SystemIO' and 'SystemMemory' operation regions out of NACL to be globally available. The buffer (BUFF in above patch) will be gone. It is added by my this patch, its mere use is to covert param of _LS{I,R,W} into those of NACL. If I implemented each _LS{I,R,W} on their own, rather than wrap the multi-purpose NACL, no buffer needed, at least I now assume so. And, why declare the 4K buffer global to sub-nvdimm? I now recall that it is because if not each sub-nvdimm device would contain a 4K buff, which will make this SSDT enormously large. > > So unless we have to add new host/guest ABI, I'd prefer reusing > existing one and complicate only new _LS{I,R,W} AML without > touching NACL or host side. As mentioned above, I assume no new host/guest ABI, just extract 'SystemIO' and 'SystemMemory' operation regions to a higher level scope. > > > > > > > > > > > > > > + 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. > > > > > > try to test it with Windows guest (it usually less tolerable to > > > errors > > > than Linux + it's own quirks that you need to carter to) > > > Also it would e nice if you test and put results in cover letter > > > not only for Linux but for Windows as well. > > > > > > > I'll try to, but have no Windows resource at hand, I'll ask around > > if > > any test resource to cover that. > > > > > > > > > > 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); > > > > > > }
On Thu, 05 May 2022 21:26:59 +0800 Robert Hoo <robert.hu@linux.intel.com> wrote: > On Thu, 2022-05-05 at 10:50 +0200, Igor Mammedov wrote: > ... > > > > > > > @@ -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? > > > > > > > ... > > > > > > > > > > > > > + 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. > > > > > > > > wrapper is fine, you just need to repackage content of the Buffer > > > > into a Package > > > > > > > > > > I now prefer re-implementation, i.e. make _LS{I,R,W} their own > > > functions, less NACL's burden and don't make it more complex, it's > > > already not neat; and more point, I think by this we can save the > > > 4K > > > Buff at all. > > > Does this sound all right to you? > > > > On one hand what you propose will be bit simpler (but not mach) than > > repacking (and only on AML guest side), however it will add to host > > side an extra interface/ABI that we will have to maintain, also it > > won't save space as buffer will still be there for legacy interface. > > No, sorry, I didn't explain it clear. > No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-device > methods. Of course, I'm going to extract 'SystemIO' and 'SystemMemory' > operation regions out of NACL to be globally available. > > The buffer (BUFF in above patch) will be gone. It is added by my this > patch, its mere use is to covert param of _LS{I,R,W} into those of > NACL. If I implemented each _LS{I,R,W} on their own, rather than wrap > the multi-purpose NACL, no buffer needed, at least I now assume so. > And, why declare the 4K buffer global to sub-nvdimm? I now recall that > it is because if not each sub-nvdimm device would contain a 4K buff, > which will make this SSDT enormously large. ok, lets see how it will look like when you are done. > > > > So unless we have to add new host/guest ABI, I'd prefer reusing > > existing one and complicate only new _LS{I,R,W} AML without > > touching NACL or host side. > > As mentioned above, I assume no new host/guest ABI, just extract > 'SystemIO' and 'SystemMemory' operation regions to a higher level > scope. > > > > > > > > > > > > > > > > > > + 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. > > > > > > > > try to test it with Windows guest (it usually less tolerable to > > > > errors > > > > than Linux + it's own quirks that you need to carter to) > > > > Also it would e nice if you test and put results in cover letter > > > > not only for Linux but for Windows as well. > > > > > > > > > > I'll try to, but have no Windows resource at hand, I'll ask around > > > if > > > any test resource to cover that. > > > > > > > > > > > > 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); > > > > > > > } >
On Fri, 2022-05-06 at 11:23 +0200, Igor Mammedov wrote: > > > > > No, sorry, I didn't explain it clear. > > No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub- > > device > > methods. Of course, I'm going to extract 'SystemIO' and > > 'SystemMemory' > > operation regions out of NACL to be globally available. > > > > The buffer (BUFF in above patch) will be gone. It is added by my > > this > > patch, its mere use is to covert param of _LS{I,R,W} into those of > > NACL. If I implemented each _LS{I,R,W} on their own, rather than > > wrap > > the multi-purpose NACL, no buffer needed, at least I now assume so. > > And, why declare the 4K buffer global to sub-nvdimm? I now recall > > that > > it is because if not each sub-nvdimm device would contain a 4K > > buff, > > which will make this SSDT enormously large. > > ok, lets see how it will look like when you are done. In ASL, can we define package with Arg in? e.g. Name (PKG1, Package () { Arg0, Arg1, Arg2 }) But it cannot pass compilation. Any approach to achieve this? if so, we can still use simpler wrap scheme like v1 and save the 4K buffer. > > > > > > > So unless we have to add new host/guest ABI, I'd prefer reusing > > > existing one and complicate only new _LS{I,R,W} AML without > > > touching NACL or host side. > > > > As mentioned above, I assume no new host/guest ABI, just extract > > 'SystemIO' and 'SystemMemory' operation regions to a higher level > > scope. > > >
On Wed, 18 May 2022 08:20:56 +0800 Robert Hoo <robert.hu@linux.intel.com> wrote: > On Fri, 2022-05-06 at 11:23 +0200, Igor Mammedov wrote: > > > > > > > > No, sorry, I didn't explain it clear. > > > No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub- > > > device > > > methods. Of course, I'm going to extract 'SystemIO' and > > > 'SystemMemory' > > > operation regions out of NACL to be globally available. > > > > > > The buffer (BUFF in above patch) will be gone. It is added by my > > > this > > > patch, its mere use is to covert param of _LS{I,R,W} into those of > > > NACL. If I implemented each _LS{I,R,W} on their own, rather than > > > wrap > > > the multi-purpose NACL, no buffer needed, at least I now assume so. > > > And, why declare the 4K buffer global to sub-nvdimm? I now recall > > > that > > > it is because if not each sub-nvdimm device would contain a 4K > > > buff, > > > which will make this SSDT enormously large. > > > > ok, lets see how it will look like when you are done. > > In ASL, can we define package with Arg in? e.g. > > Name (PKG1, Package () > { > Arg0, > Arg1, > Arg2 > }) Looking at the spec it doesn't seem to be a valid construct. see "DefPackage :=" and "PackageElement :=" definitions. However you can try to play with RefOf to turn ArgX into reference (mind 'read' rules fro ArgTerm). > But it cannot pass compilation. Any approach to achieve this? if so, we > can still use simpler wrap scheme like v1 and save the 4K buffer. > > > > > > > > > > So unless we have to add new host/guest ABI, I'd prefer reusing > > > > existing one and complicate only new _LS{I,R,W} AML without > > > > touching NACL or host side. > > > > > > As mentioned above, I assume no new host/guest ABI, just extract > > > 'SystemIO' and 'SystemMemory' operation regions to a higher level > > > scope. > > > > >
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. */ + if (in->revision != 0x1 && in->revision != 0x2) { + nvdimm_debug("Revision 0x%x is not supported, expect 0x1 or 0x2.\n", + in->revision); nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr); goto exit; } @@ -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)); 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 */ + 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); + + 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")); + 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_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)))); + 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)))); + aml_append(nvdimm_dev, method); + nvdimm_build_device_dsm(nvdimm_dev, handle); aml_append(root_dev, nvdimm_dev); }
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 standard method _LSI, _LSR, _LSW respectively. Functions semantics are almost identical, so my implementation is to reuse existing _DSMs, just create _LS{I,R,W} interfaces and constructs parameters and call _DSMs. Only child NVDIMM devices has these methods, rather Root device. By this patch, new NVDIMM sub device in ACPI namespace will be like this: Device (NV00) { Name (_ADR, One) // _ADR: Address Method (_LSI, 0, NotSerialized) // _LSI: Label Storage Information { Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x04, Zero, One)) } Method (_LSR, 2, Serialized) // _LSR: Label Storage Read { CreateDWordField (BUFF, Zero, DWD0) CreateDWordField (BUFF, 0x04, DWD1) Name (PKG1, Package (0x01) { BUFF }) DWD0 = Arg0 DWD1 = Arg1 Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x05, PKG1, One)) } Method (_LSW, 3, Serialized) // _LSW: Label Storage Write { CreateDWordField (BUFF, Zero, DWD0) CreateDWordField (BUFF, 0x04, DWD1) CreateField (BUFF, 0x40, 0x7FA0, FILD) Name (PKG1, Package (0x01) { BUFF }) DWD0 = Arg0 DWD1 = Arg1 FILD = Arg2 Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x06, PKG1, One)) } Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method { Return (NCAL (Arg0, Arg1, Arg2, Arg3, One)) } } Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> Reviewed-by: Jingqi Liu<jingqi.liu@intel.com> --- hw/acpi/nvdimm.c | 56 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 4 deletions(-)