From: "Michael S. Tsirkin" <mst@redhat.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: pbonzini@redhat.com, imammedo@redhat.com, gleb@kernel.org,
mtosatti@redhat.com, stefanha@redhat.com, rth@twiddle.net,
ehabkost@redhat.com, dan.j.williams@intel.com,
kvm@vger.kernel.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v3 00/32] implement vNVDIMM
Date: Mon, 19 Oct 2015 09:57:08 +0300 [thread overview]
Message-ID: <20151019095708-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <561C96CC.40702@linux.intel.com>
On Tue, Oct 13, 2015 at 01:29:48PM +0800, Xiao Guangrong wrote:
>
>
> On 10/12/2015 07:55 PM, Michael S. Tsirkin wrote:
> >On Sun, Oct 11, 2015 at 11:52:32AM +0800, Xiao Guangrong wrote:
> >>Changelog in v3:
> >>There is huge change in this version, thank Igor, Stefan, Paolo, Eduardo,
> >>Michael for their valuable comments, the patchset finally gets better shape.
> >
> >Thanks!
> >This needs some changes in coding style, and more comments, to
> >make it easier to maintain going forward.
>
> Thanks for your review, Michael. I have learned lots of thing from
> your comments.
>
> >
> >High level comments - I didn't point out all instances,
> >please go over code and locate them yourself.
> >I focused on acpi code in this review.
>
> Okay, will do.
>
> >
> > - fix coding style violations, prefix eveything with nvdimm_ etc
>
> Actually i did not pay attention on naming the stuff which is only internally
> used. Thank you for pointing it out and will fix it in next version.
>
> > - in apci code, avoid manual memory management/complex pointer math
>
> I am not very good at ACPI ASL/AML, could you please more detail?
It's about C.
For example:
Foo *foo = acpi_data_push(table, sizeof *foo);
Bar *foo = acpi_data_push(table, sizeof *bar);
is pretty obviously safe, and it doesn't require you to do any
calculations.
char *buf = acpi_data_push(table, sizeof *foo + sizeof *bar);
is worse, now you need:
Bar *bar = (Bar *)(buf + sizeof *foo);
which will corrupt memory if you get the size wrong in push.
> > - comments are needed to document apis & explain what's going on
> > - constants need comments too, refer to text that
> > can be looked up in acpi spec verbatim
>
> Indeed, will document carefully.
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>
Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org,
mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com,
imammedo@redhat.com, pbonzini@redhat.com,
dan.j.williams@intel.com, rth@twiddle.net
Subject: Re: [Qemu-devel] [PATCH v3 00/32] implement vNVDIMM
Date: Mon, 19 Oct 2015 09:57:08 +0300 [thread overview]
Message-ID: <20151019095708-mutt-send-email-mst@redhat.com> (raw)
In-Reply-To: <561C96CC.40702@linux.intel.com>
On Tue, Oct 13, 2015 at 01:29:48PM +0800, Xiao Guangrong wrote:
>
>
> On 10/12/2015 07:55 PM, Michael S. Tsirkin wrote:
> >On Sun, Oct 11, 2015 at 11:52:32AM +0800, Xiao Guangrong wrote:
> >>Changelog in v3:
> >>There is huge change in this version, thank Igor, Stefan, Paolo, Eduardo,
> >>Michael for their valuable comments, the patchset finally gets better shape.
> >
> >Thanks!
> >This needs some changes in coding style, and more comments, to
> >make it easier to maintain going forward.
>
> Thanks for your review, Michael. I have learned lots of thing from
> your comments.
>
> >
> >High level comments - I didn't point out all instances,
> >please go over code and locate them yourself.
> >I focused on acpi code in this review.
>
> Okay, will do.
>
> >
> > - fix coding style violations, prefix eveything with nvdimm_ etc
>
> Actually i did not pay attention on naming the stuff which is only internally
> used. Thank you for pointing it out and will fix it in next version.
>
> > - in apci code, avoid manual memory management/complex pointer math
>
> I am not very good at ACPI ASL/AML, could you please more detail?
It's about C.
For example:
Foo *foo = acpi_data_push(table, sizeof *foo);
Bar *foo = acpi_data_push(table, sizeof *bar);
is pretty obviously safe, and it doesn't require you to do any
calculations.
char *buf = acpi_data_push(table, sizeof *foo + sizeof *bar);
is worse, now you need:
Bar *bar = (Bar *)(buf + sizeof *foo);
which will corrupt memory if you get the size wrong in push.
> > - comments are needed to document apis & explain what's going on
> > - constants need comments too, refer to text that
> > can be looked up in acpi spec verbatim
>
> Indeed, will document carefully.
next prev parent reply other threads:[~2015-10-19 6:57 UTC|newest]
Thread overview: 200+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-11 3:52 [PATCH v3 00/32] implement vNVDIMM Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-10 21:17 ` Dan Williams
2015-10-10 21:17 ` [Qemu-devel] " Dan Williams
2015-10-12 4:33 ` Xiao Guangrong
2015-10-12 4:33 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 16:36 ` Dan Williams
2015-10-12 16:36 ` [Qemu-devel] " Dan Williams
2015-10-13 3:14 ` Xiao Guangrong
2015-10-13 3:14 ` Xiao Guangrong
2015-10-13 3:38 ` Dan Williams
2015-10-13 3:38 ` Dan Williams
2015-10-13 5:49 ` Xiao Guangrong
2015-10-13 5:49 ` Xiao Guangrong
2015-10-13 6:36 ` Dan Williams
2015-10-13 6:36 ` Dan Williams
2015-10-14 4:03 ` Xiao Guangrong
2015-10-14 4:03 ` Xiao Guangrong
2015-10-14 19:20 ` Dan Williams
2015-10-14 19:20 ` Dan Williams
2015-10-11 3:52 ` [PATCH v3 01/32] acpi: add aml_derefof Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 12:30 ` Igor Mammedov
2015-10-13 12:30 ` [Qemu-devel] " Igor Mammedov
2015-10-11 3:52 ` [PATCH v3 02/32] acpi: add aml_sizeof Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 12:33 ` Igor Mammedov
2015-10-13 12:33 ` [Qemu-devel] " Igor Mammedov
2015-10-11 3:52 ` [PATCH v3 03/32] acpi: add aml_create_field Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 12:38 ` Igor Mammedov
2015-10-13 12:38 ` [Qemu-devel] " Igor Mammedov
2015-10-13 16:36 ` Xiao Guangrong
2015-10-13 16:36 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 04/32] acpi: add aml_mutex, aml_acquire, aml_release Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 13:34 ` Igor Mammedov
2015-10-13 13:34 ` [Qemu-devel] " Igor Mammedov
2015-10-13 16:44 ` Xiao Guangrong
2015-10-13 16:44 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 05/32] acpi: add aml_concatenate Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 06/32] acpi: add aml_object_type Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 07/32] util: introduce qemu_file_get_page_size() Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 08/32] exec: allow memory to be allocated from any kind of path Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 10:08 ` Michael S. Tsirkin
2015-10-12 10:08 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-13 3:31 ` Xiao Guangrong
2015-10-13 3:31 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 09/32] exec: allow file_ram_alloc to work on file Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 10/32] hostmem-file: clean up memory allocation Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 11/32] hostmem-file: use whole file size if possible Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 11:50 ` Vladimir Sementsov-Ogievskiy
2015-10-13 11:50 ` Vladimir Sementsov-Ogievskiy
2015-10-13 16:53 ` Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 12/32] pc-dimm: remove DEFAULT_PC_DIMMSIZE Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 13/32] pc-dimm: make pc_existing_dimms_capacity static and rename it Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 14/32] pc-dimm: drop the prefix of pc-dimm Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 16:43 ` Eric Blake
2015-10-13 3:32 ` Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 15/32] stubs: rename qmp_pc_dimm_device_list.c Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 16/32] pc-dimm: rename pc-dimm.c and pc-dimm.h Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 17/32] dimm: abstract dimm device from pc-dimm Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 18/32] dimm: get mapped memory region from DIMMDeviceClass->get_memory_region Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 19/32] dimm: keep the state of the whole backend memory Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 20/32] dimm: introduce realize callback Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 21/32] nvdimm: implement NVDIMM device abstract Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 22/32] nvdimm: init the address region used by NVDIMM ACPI Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 6:56 ` Michael S. Tsirkin
2015-10-19 6:56 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 7:27 ` Xiao Guangrong
2015-10-19 7:27 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 7:39 ` Michael S. Tsirkin
2015-10-19 7:39 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 7:44 ` Xiao Guangrong
2015-10-19 7:44 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 9:17 ` Michael S. Tsirkin
2015-10-19 9:17 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 9:46 ` Igor Mammedov
2015-10-19 9:46 ` [Qemu-devel] " Igor Mammedov
2015-10-19 10:01 ` Xiao Guangrong
2015-10-19 10:34 ` Michael S. Tsirkin
2015-10-19 10:34 ` Michael S. Tsirkin
2015-10-19 10:42 ` Igor Mammedov
2015-10-19 10:42 ` Igor Mammedov
2015-10-19 17:56 ` Xiao Guangrong
2015-10-19 17:56 ` Xiao Guangrong
2015-10-20 2:27 ` Xiao Guangrong
2015-10-20 2:27 ` Xiao Guangrong
2015-10-19 9:18 ` Igor Mammedov
2015-10-19 9:18 ` [Qemu-devel] " Igor Mammedov
2015-10-19 10:25 ` Michael S. Tsirkin
2015-10-19 10:25 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 17:54 ` Xiao Guangrong
2015-10-19 17:54 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 21:20 ` Michael S. Tsirkin
2015-10-19 21:20 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-11 3:52 ` [PATCH v3 23/32] nvdimm: build ACPI NFIT table Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 11:27 ` Michael S. Tsirkin
2015-10-12 11:27 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-13 5:13 ` Xiao Guangrong
2015-10-13 5:13 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 5:42 ` Michael S. Tsirkin
2015-10-13 5:42 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-13 6:06 ` Xiao Guangrong
2015-10-13 6:06 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 16:40 ` Dan Williams
2015-10-12 16:40 ` [Qemu-devel] " Dan Williams
2015-10-13 5:17 ` Xiao Guangrong
2015-10-13 5:17 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 6:07 ` Michael S. Tsirkin
2015-10-13 6:07 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-11 3:52 ` [PATCH v3 24/32] nvdimm: init the address region used by DSM method Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 25/32] nvdimm: build ACPI nvdimm devices Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 14:39 ` Igor Mammedov
2015-10-13 14:39 ` Igor Mammedov
2015-10-13 17:24 ` Xiao Guangrong
2015-10-13 17:24 ` Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 26/32] nvdimm: save arg3 for NVDIMM device _DSM method Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 6:50 ` Michael S. Tsirkin
2015-10-19 6:50 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 7:14 ` Xiao Guangrong
2015-10-19 7:14 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 7:47 ` Michael S. Tsirkin
2015-10-19 7:47 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-19 7:51 ` Xiao Guangrong
2015-10-19 7:51 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:52 ` [PATCH v3 27/32] nvdimm: support DSM_CMD_IMPLEMENTED function Xiao Guangrong
2015-10-11 3:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-14 9:40 ` Stefan Hajnoczi
2015-10-14 9:40 ` [Qemu-devel] " Stefan Hajnoczi
2015-10-14 14:50 ` Xiao Guangrong
2015-10-14 14:50 ` [Qemu-devel] " Xiao Guangrong
2015-10-14 17:06 ` Eduardo Habkost
2015-10-14 17:06 ` [Qemu-devel] " Eduardo Habkost
2015-10-15 1:43 ` Xiao Guangrong
2015-10-15 1:43 ` [Qemu-devel] " Xiao Guangrong
2015-10-15 15:07 ` Stefan Hajnoczi
2015-10-15 15:07 ` [Qemu-devel] " Stefan Hajnoczi
2015-10-16 2:30 ` Xiao Guangrong
2015-10-16 2:30 ` [Qemu-devel] " Xiao Guangrong
2015-10-14 9:41 ` Stefan Hajnoczi
2015-10-14 9:41 ` [Qemu-devel] " Stefan Hajnoczi
2015-10-14 14:52 ` Xiao Guangrong
2015-10-14 14:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-15 15:01 ` Stefan Hajnoczi
2015-10-15 15:01 ` [Qemu-devel] " Stefan Hajnoczi
2015-10-16 2:32 ` Xiao Guangrong
2015-10-16 2:32 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:53 ` [PATCH v3 28/32] nvdimm: support DSM_CMD_NAMESPACE_LABEL_SIZE function Xiao Guangrong
2015-10-11 3:53 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:53 ` [PATCH v3 29/32] nvdimm: support DSM_CMD_GET_NAMESPACE_LABEL_DATA Xiao Guangrong
2015-10-11 3:53 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:53 ` [PATCH v3 30/32] nvdimm: support DSM_CMD_SET_NAMESPACE_LABEL_DATA Xiao Guangrong
2015-10-11 3:53 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:53 ` [PATCH v3 31/32] nvdimm: allow using whole backend memory as pmem Xiao Guangrong
2015-10-11 3:53 ` [Qemu-devel] " Xiao Guangrong
2015-10-11 3:53 ` [PATCH v3 32/32] nvdimm: add maintain info Xiao Guangrong
2015-10-11 3:53 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 2:59 ` [PATCH v3 00/32] implement vNVDIMM Bharata B Rao
2015-10-12 2:59 ` [Qemu-devel] " Bharata B Rao
2015-10-12 3:06 ` Xiao Guangrong
2015-10-12 3:06 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 8:20 ` Igor Mammedov
2015-10-12 8:20 ` [Qemu-devel] " Igor Mammedov
2015-10-12 8:21 ` Xiao Guangrong
2015-10-12 8:21 ` [Qemu-devel] " Xiao Guangrong
2015-10-12 11:55 ` Michael S. Tsirkin
2015-10-12 11:55 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-13 5:29 ` Xiao Guangrong
2015-10-13 5:29 ` [Qemu-devel] " Xiao Guangrong
2015-10-13 5:57 ` Michael S. Tsirkin
2015-10-13 5:57 ` [Qemu-devel] " Michael S. Tsirkin
2015-10-13 5:52 ` Xiao Guangrong
2015-10-13 5:52 ` [Qemu-devel] " Xiao Guangrong
2015-10-19 6:57 ` Michael S. Tsirkin [this message]
2015-10-19 6:57 ` Michael S. Tsirkin
2015-10-19 6:56 ` Michael S. Tsirkin
2015-10-19 6:56 ` [Qemu-devel] " Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151019095708-mutt-send-email-mst@redhat.com \
--to=mst@redhat.com \
--cc=dan.j.williams@intel.com \
--cc=ehabkost@redhat.com \
--cc=gleb@kernel.org \
--cc=guangrong.xiao@linux.intel.com \
--cc=imammedo@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=mtosatti@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.