| Message ID | 1445216059-88521-28-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 19, 2015 at 08:54:13AM +0800, Xiao Guangrong wrote: > Check if the input Arg3 is valid then store it into dsm_in if needed > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > hw/acpi/nvdimm.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 7e99889..b211b8b 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -624,10 +624,29 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, > > method = aml_method_serialized("NCAL", 4); > { > + Aml *ifctx; > + > aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); > aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); > aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); > > + /* Arg3 is passed as Package and it has one element? */ > + ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), > + aml_int(4)), > + aml_equal(aml_sizeof(aml_arg(3)), > + aml_int(1)))); > + { > + /* Local0 = Index(Arg3, 0) */ > + aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), > + aml_local(0))); > + /* Local3 = DeRefOf(Local0) */ > + aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), > + aml_local(3))); > + /* ARG3 = Local3 */ > + aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); > + } > + aml_append(method, ifctx); > + > aml_append(method, aml_store(aml_int(NOTIFY_VALUE), aml_name("NOTI"))); > > aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); I commented on this patch on v3. It doesn't look like this was addressed. > -- > 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/2015 01:16 AM, Michael S. Tsirkin wrote: > On Mon, Oct 19, 2015 at 08:54:13AM +0800, Xiao Guangrong wrote: >> Check if the input Arg3 is valid then store it into dsm_in if needed >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> hw/acpi/nvdimm.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 7e99889..b211b8b 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -624,10 +624,29 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, >> >> method = aml_method_serialized("NCAL", 4); >> { >> + Aml *ifctx; >> + >> aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); >> aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); >> aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); >> >> + /* Arg3 is passed as Package and it has one element? */ >> + ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), >> + aml_int(4)), >> + aml_equal(aml_sizeof(aml_arg(3)), >> + aml_int(1)))); >> + { >> + /* Local0 = Index(Arg3, 0) */ >> + aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), >> + aml_local(0))); >> + /* Local3 = DeRefOf(Local0) */ >> + aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), >> + aml_local(3))); >> + /* ARG3 = Local3 */ >> + aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); >> + } >> + aml_append(method, ifctx); >> + >> aml_append(method, aml_store(aml_int(NOTIFY_VALUE), aml_name("NOTI"))); >> >> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > > I commented on this patch on v3. > It doesn't look like this was addressed. > Ah... I see no one commented this patch ([PATCH v3 26/32] nvdimm: save arg3 for NVDIMM device _DSM method) on v3. Do you mean we need more and better comment to explain arg3? Or anything else? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 12:04:48PM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 01:16 AM, Michael S. Tsirkin wrote: > >On Mon, Oct 19, 2015 at 08:54:13AM +0800, Xiao Guangrong wrote: > >>Check if the input Arg3 is valid then store it into dsm_in if needed > >> > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >>--- > >> hw/acpi/nvdimm.c | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >>diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >>index 7e99889..b211b8b 100644 > >>--- a/hw/acpi/nvdimm.c > >>+++ b/hw/acpi/nvdimm.c > >>@@ -624,10 +624,29 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, > >> > >> method = aml_method_serialized("NCAL", 4); > >> { > >>+ Aml *ifctx; > >>+ > >> aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); > >> aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); > >> aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); > >> > >>+ /* Arg3 is passed as Package and it has one element? */ > >>+ ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), > >>+ aml_int(4)), > >>+ aml_equal(aml_sizeof(aml_arg(3)), > >>+ aml_int(1)))); > >>+ { > >>+ /* Local0 = Index(Arg3, 0) */ > >>+ aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), > >>+ aml_local(0))); > >>+ /* Local3 = DeRefOf(Local0) */ > >>+ aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), > >>+ aml_local(3))); > >>+ /* ARG3 = Local3 */ > >>+ aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); > >>+ } > >>+ aml_append(method, ifctx); > >>+ > >> aml_append(method, aml_store(aml_int(NOTIFY_VALUE), aml_name("NOTI"))); > >> > >> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > > > >I commented on this patch on v3. > >It doesn't look like this was addressed. > > > > Ah... I see no one commented this patch ([PATCH v3 26/32] nvdimm: save arg3 for NVDIMM > device _DSM method) on v3. > > Do you mean we need more and better comment to explain arg3? Or anything else? Interesting. I have it in my sent mail file, but it doesn't seem to be on list. I've just resent it, and a couple of other messages that seem to have disappeared into the ether. These are the messages I have for v3: 33587 F To Xiao Guangro Re: [PATCH v3 26/32] nvdimm: save arg3 for NVDIMM device _DSM method 33588 F To Xiao Guangro Re: [PATCH v3 22/32] nvdimm: init the address region used by NVDIMM ACPI 33589 F To Xiao Guangro Re: [PATCH v3 23/32] nvdimm: build ACPI NFIT table 33590 F To Xiao Guangro Re: [PATCH v3 00/32] implement vNVDIMM 33597 F To Xiao Guangro Re: [PATCH v3 23/32] nvdimm: build ACPI NFIT table 33598 F To Xiao Guangro Re: [PATCH v3 00/32] implement vNVDIMM 33599 F To Xiao Guangro Re: [PATCH v3 23/32] nvdimm: build ACPI NFIT table
On Mon, Oct 19, 2015 at 12:04:48PM +0800, Xiao Guangrong wrote: > > > On 10/19/2015 01:16 AM, Michael S. Tsirkin wrote: > >On Mon, Oct 19, 2015 at 08:54:13AM +0800, Xiao Guangrong wrote: > >>Check if the input Arg3 is valid then store it into dsm_in if needed > >> > >>Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >>--- > >> hw/acpi/nvdimm.c | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >>diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >>index 7e99889..b211b8b 100644 > >>--- a/hw/acpi/nvdimm.c > >>+++ b/hw/acpi/nvdimm.c > >>@@ -624,10 +624,29 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, > >> > >> method = aml_method_serialized("NCAL", 4); > >> { > >>+ Aml *ifctx; > >>+ > >> aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); > >> aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); > >> aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); > >> > >>+ /* Arg3 is passed as Package and it has one element? */ > >>+ ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), > >>+ aml_int(4)), > >>+ aml_equal(aml_sizeof(aml_arg(3)), > >>+ aml_int(1)))); > >>+ { > >>+ /* Local0 = Index(Arg3, 0) */ > >>+ aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), > >>+ aml_local(0))); > >>+ /* Local3 = DeRefOf(Local0) */ > >>+ aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), > >>+ aml_local(3))); > >>+ /* ARG3 = Local3 */ > >>+ aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); > >>+ } > >>+ aml_append(method, ifctx); > >>+ > >> aml_append(method, aml_store(aml_int(NOTIFY_VALUE), aml_name("NOTI"))); > >> > >> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > > > >I commented on this patch on v3. > >It doesn't look like this was addressed. > > > > Ah... I see no one commented this patch ([PATCH v3 26/32] nvdimm: save arg3 for NVDIMM > device _DSM method) on v3. > > Do you mean we need more and better comment to explain arg3? Or anything else? > I mean don't use ASL to comment C. It's not more readable. Describe why the code is the way it is. Use variables by preference, C does not have weird limitations like ASL so you don't need to call your variables "arg3". What does it hold? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 12:09 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Oct 19, 2015 at 12:04:48PM +0800, Xiao Guangrong wrote: > I mean don't use ASL to comment C. It's not more readable. > Describe why the code is the way it is. Use variables by preference, > C does not have weird limitations like ASL so you don't need to call > your variables "arg3". What does it hold? > What it holds is function number specific. It's similar to SYSCALL_DEFINEx where the ASL code is there to marshal arguments from the OS through ACPI to a BIOS routine. See the definition of the example _DSM functions here and the usages of "Arg3": http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 10:29:50AM -0700, Dan Williams wrote: > On Mon, Oct 19, 2015 at 12:09 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 19, 2015 at 12:04:48PM +0800, Xiao Guangrong wrote: > > I mean don't use ASL to comment C. It's not more readable. > > Describe why the code is the way it is. Use variables by preference, > > C does not have weird limitations like ASL so you don't need to call > > your variables "arg3". What does it hold? > > > > What it holds is function number specific. It's similar to > SYSCALL_DEFINEx where the ASL code is there to marshal arguments from > the OS through ACPI to a BIOS routine. See the definition of the > example _DSM functions here and the usages of "Arg3": > http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf So it seems to say "3.1.1.1.1 Input (Arg3)". Is that right? So Aml *input = aml_arg(3); /* Input (Arg3) */ or even Aml *input_arg3 = aml_arg(3); /* Input (Arg3) */ My point is we are not writing ASL. There is no reason to use cryptic names. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 2:19 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Oct 19, 2015 at 10:29:50AM -0700, Dan Williams wrote: >> On Mon, Oct 19, 2015 at 12:09 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> > On Mon, Oct 19, 2015 at 12:04:48PM +0800, Xiao Guangrong wrote: >> > I mean don't use ASL to comment C. It's not more readable. >> > Describe why the code is the way it is. Use variables by preference, >> > C does not have weird limitations like ASL so you don't need to call >> > your variables "arg3". What does it hold? >> > >> >> What it holds is function number specific. It's similar to >> SYSCALL_DEFINEx where the ASL code is there to marshal arguments from >> the OS through ACPI to a BIOS routine. See the definition of the >> example _DSM functions here and the usages of "Arg3": >> http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf > > So it seems to say "3.1.1.1.1 Input (Arg3)". > Is that right? So > > Aml *input = aml_arg(3); /* Input (Arg3) */ > or even > Aml *input_arg3 = aml_arg(3); /* Input (Arg3) */ > > My point is we are not writing ASL. There is no reason > to use cryptic names. > Ah, ok, sounds good to me. ASL is already hard to read and we shouldn't be using it as code commentary. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index 7e99889..b211b8b 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -624,10 +624,29 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, method = aml_method_serialized("NCAL", 4); { + Aml *ifctx; + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); + /* Arg3 is passed as Package and it has one element? */ + ifctx = aml_if(aml_and(aml_equal(aml_object_type(aml_arg(3)), + aml_int(4)), + aml_equal(aml_sizeof(aml_arg(3)), + aml_int(1)))); + { + /* Local0 = Index(Arg3, 0) */ + aml_append(ifctx, aml_store(aml_index(aml_arg(3), aml_int(0)), + aml_local(0))); + /* Local3 = DeRefOf(Local0) */ + aml_append(ifctx, aml_store(aml_derefof(aml_local(0)), + aml_local(3))); + /* ARG3 = Local3 */ + aml_append(ifctx, aml_store(aml_local(3), aml_name("ARG3"))); + } + aml_append(method, ifctx); + aml_append(method, aml_store(aml_int(NOTIFY_VALUE), aml_name("NOTI"))); aml_append(method, aml_store(aml_name("RLEN"), aml_local(6)));
Check if the input Arg3 is valid then store it into dsm_in if needed Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- hw/acpi/nvdimm.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)