From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53814) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo4X0-0006re-7B for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:06:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zo4Ww-0006ZD-UF for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:06:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42294) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo4Ww-0006YE-Kl for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:06:50 -0400 Date: Mon, 19 Oct 2015 10:06:45 +0300 From: "Michael S. Tsirkin" Message-ID: <20151019100041-mutt-send-email-mst@redhat.com> References: <1445216059-88521-1-git-send-email-guangrong.xiao@linux.intel.com> <1445216059-88521-29-git-send-email-guangrong.xiao@linux.intel.com> <20151018205030-mutt-send-email-mst@redhat.com> <562473FC.1000309@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <562473FC.1000309@linux.intel.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong 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 On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote: >=20 >=20 > On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote: > >On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: > >>__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method) > >> > >>Function 0 is a query function. We do not support any function on roo= t > >>device and only 3 functions are support for NVDIMM device, > >>DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DAT= A and > >>DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only al= low to > >>access device's Label Namespace > >> > >>Signed-off-by: Xiao Guangrong > >>--- > >> hw/acpi/nvdimm.c | 184 ++++++++++++++++++++++++++++++++++++++++++++= ++++++++++- > >> 1 file changed, 182 insertions(+), 2 deletions(-) > >> > >>diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >>index b211b8b..37fea1c 100644 > >>--- a/hw/acpi/nvdimm.c > >>+++ b/hw/acpi/nvdimm.c > >>@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slo= t) > >> return nvdimm_slot_to_spa_index(slot) + 1; > >> } > >> > >>+static NVDIMMDevice > >>+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle) > >>+{ > >>+ for (; list; list =3D list->next) { > >>+ NVDIMMDevice *nvdimm =3D list->data; > >>+ int slot =3D object_property_get_int(OBJECT(nvdimm), DIMM_SL= OT_PROP, > >>+ NULL); > >>+ > >>+ if (nvdimm_slot_to_handle(slot) =3D=3D handle) { > >>+ return nvdimm; > >>+ } > >>+ } > >>+ > >>+ return NULL; > >>+} > >>+ > >> /* > >> * Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range > >> * Structure > >>@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures= , GArray *table_offsets, > >> /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt = */ > >> #define NOTIFY_VALUE 0x99 > > > >Again, please prefix everything consistently. >=20 > Okay, will do. Sorry for i missed it. >=20 > > > >> > >>+enum { > >>+ DSM_FUN_IMPLEMENTED =3D 0, > >>+ > >>+ /* NVDIMM Root Device Functions */ > >>+ DSM_ROOT_DEV_FUN_ARS_CAP =3D 1, > >>+ DSM_ROOT_DEV_FUN_ARS_START =3D 2, > >>+ DSM_ROOT_DEV_FUN_ARS_QUERY =3D 3, > >>+ > >>+ /* NVDIMM Device (non-root) Functions */ > >>+ DSM_DEV_FUN_SMART =3D 1, > >>+ DSM_DEV_FUN_SMART_THRESHOLD =3D 2, > >>+ DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS =3D 3, > >>+ DSM_DEV_FUN_NAMESPACE_LABEL_SIZE =3D 4, > >>+ DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA =3D 5, > >>+ DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA =3D 6, > >>+ DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE =3D 7, > >>+ DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG =3D 8, > >>+ DSM_DEV_FUN_VENDOR_SPECIFIC =3D 9, > >>+}; > > > >Does FUN stand for "function"? FUNC or FN is probably better. > > >=20 > Yes. >=20 > >Please list exact names as they appear in spec so > >they can be searched for. >=20 > The spec reference was at where this _FUN_ is used, eg: >=20 > /* > * Please refer to DSM specification 4.4.1 Get Namespace Label Size > * (Function Index 4). > * > * It gets the size of Namespace Label data area and the max data size > * that Get/Set Namespace Label Data functions can transfer. > */ > static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *ou= t) >=20 > I will follow your =E2=80=98single use=E2=80=99 comments below, these d= efinitions will > be dropped, the code will be like this: >=20 > switch (function) { > case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label S= ize. */: If it's the same spec, you don't have to repeat it: /* Encode function according to DSM Spec rev 1.0 */ > switch (function) { > case 4 /* 4.4.1 Get Namespace Label Size Get Namespace Label Size. = */: same for chapter etc. > nvdimm_dsm_func_label_size(); > case ... > ... > }; >=20 > > > > > > > >>+ > >>+enum { > >>+ /* Common return status codes. */ > >>+ DSM_STATUS_SUCCESS =3D 0, /* Success */ > >>+ DSM_STATUS_NOT_SUPPORTED =3D 1, /* Not Supported */ > >>+ > >>+ /* NVDIMM Root Device _DSM function return status codes*/ > >>+ DSM_ROOT_DEV_STATUS_INVALID_PARAS =3D 2, /* Invalid Input Par= ameters */ > >>+ DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR =3D 3, /* Function-S= pecific > >>+ Error */ > >>+ > >>+ /* NVDIMM Device (non-root) _DSM function return status codes*/ > >>+ DSM_DEV_STATUS_NON_EXISTING_MEM_DEV =3D 2, /* Non-Existing Memo= ry Device */ > >>+ DSM_DEV_STATUS_INVALID_PARAS =3D 3, /* Invalid Input Par= ameters */ > >>+ DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR =3D 4, /* Vendor Specific E= rror */ > >>+}; > >>+ > >>+/* Current revision supported by DSM specification is 1. */ > >>+#define DSM_REVISION (1) > >>+ > >>+/* > >>+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): R= eturn > >>+ * Value Information: > > > >Drop "please refer to". >=20 > Okay. >=20 > > > >>+ * if set to zero, no functions are supported (other than function= zero) > >>+ * for the specified UUID and Revision ID. If set to one, at least= one > >>+ * additional function is supported. > >>+ */ > >>+ > >>+/* do not support any function on root. */ > >>+#define ROOT_SUPPORT_FUN (0ULL) > > > >Needs a name that implies the comment somehow. > > > >>+#define DIMM_SUPPORT_FUN ((1 << DSM_FUN_IMPLEMENTED) = \ > >>+ | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE)= \ > >>+ | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_D= ATA) \ > >>+ | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_D= ATA)) > >>+ > > > >I think it's best to just drop these macros. > >There's a single point of use - just add a comment there > >explaining what does it mean. >=20 > Okay. Good to me. >=20 > >You will be able to drop all _FUN_ macros too. >=20 > Yes, it's good for code reduction. >=20 > > > > > >> struct dsm_in { > >> uint32_t handle; > >> uint32_t revision; > >>@@ -420,6 +490,11 @@ struct dsm_in { > >> } QEMU_PACKED; > >> typedef struct dsm_in dsm_in; > >> > >>+struct cmd_out_implemented { > >>+ uint64_t cmd_list; > >>+}; > >>+typedef struct cmd_out_implemented cmd_out_implemented; > >>+ > >> struct dsm_out { > >> /* the size of buffer filled by QEMU. */ > >> uint32_t len; > >>@@ -434,12 +509,115 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, uns= igned size) > >> return 0; > >> } > >> > >>+static void nvdimm_dsm_write_status(GArray *out, uint32_t status) > >>+{ > >>+ /* status locates in the first 4 bytes in the dsm memory. */ > > > >located? >=20 > Yes... > > > >>+ assert(!out->len); > > > > > >But dsm itself can be part of a bigger table. > >So don't do it. >=20 > Okay, will drop it. >=20 > > > >>+ > >>+ status =3D cpu_to_le32(status); > >>+ g_array_append_vals(out, &status, sizeof(status)); > > > >I think this should just use the (unfortunately named) > >build_append_int_noprefix. Same applied everywhere > >where you add single values. >=20 > Okay, will use it instead. >=20 > > > >>+} > >>+ > >>+static void nvdimm_dsm_write_root(dsm_in *in, GArray *out) > >>+{ > >>+ uint32_t status =3D DSM_STATUS_NOT_SUPPORTED; > >>+ > >>+ /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method= ) */ > >>+ if (in->function =3D=3D DSM_FUN_IMPLEMENTED) { > >>+ uint64_t cmd_list =3D cpu_to_le64(ROOT_SUPPORT_FUN); > > > >see about about single use values. > > >=20 > Yes, it is good to me, will follow it. >=20 > > > >>+ > >>+ g_array_append_vals(out, &cmd_list, sizeof(cmd_list)); > >>+ return; > >>+ } > >>+ > >>+ nvdimm_debug("Return status %#x.\n", status); > >>+ nvdimm_dsm_write_status(out, status); > >>+} > >>+ > >>+static void nvdimm_dsm_write_nvdimm(dsm_in *in, GArray *out) > >>+{ > >>+ GSList *list =3D nvdimm_get_plugged_device_list(); > >>+ NVDIMMDevice *nvdimm =3D nvdimm_get_device_by_handle(list, in->h= andle); > >>+ uint32_t status =3D DSM_DEV_STATUS_NON_EXISTING_MEM_DEV; > >>+ uint64_t cmd_list; > >>+ > >>+ if (!nvdimm) { > >>+ goto set_status_free; > >>+ } > >>+ > >>+ switch (in->function) { > >>+ /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method= ) */ > >>+ case DSM_FUN_IMPLEMENTED: > >>+ cmd_list =3D cpu_to_le64(DIMM_SUPPORT_FUN); > >>+ g_array_append_vals(out, &cmd_list, sizeof(cmd_list)); > >>+ goto free; > >>+ default: > >>+ status =3D DSM_STATUS_NOT_SUPPORTED; > >>+ }; > >>+ > >>+set_status_free: > >>+ nvdimm_debug("Return status %#x.\n", status); > >>+ nvdimm_dsm_write_status(out, status); > >>+free: > >>+ g_slist_free(list); > >>+} > >>+ > >> static void > >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned = size) > >> { > >>+ NVDIMMState *state =3D opaque; > >>+ MemoryRegion *dsm_ram_mr; > >>+ dsm_in *in; > >>+ GArray *out; > >>+ void *dsm_ram_addr; > > > > > >Why don't you give this the correct type? Will avoid need for casts. >=20 > If it's defined as "dsm_out *", that will make "copy(in, dsm_ram_addr..= )" > little strange. >=20 > I will do it as your suggestion, and make a comment for the copy operat= ion: > /* > * The DSM memory is used for both OSPM saves its input parameter and Q= EMU > * saves its output result. > */ Fix up the english here pls: > * The DSM memory has two uses: OSPM saves its input parameter there, Q= EMU > * uses it to save its output result. > > > >>+ > >> if (val !=3D NOTIFY_VALUE) { > >> fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64,= val); > >> } > >>+ > >>+ dsm_ram_mr =3D memory_region_find(&state->mr, getpagesize(), > >>+ getpagesize()).mr; > >>+ dsm_ram_addr =3D memory_region_get_ram_ptr(dsm_ram_mr); > > > > > >This needs a validity check for size. >=20 > Okay, will add this: >=20 > assert(memory_region_size(dsm_ram_mr) =3D=3D getpagesize()); No, the point is to make sure getpagesize is big enough to hold the structure. > > > >>+ > >>+ /* > >>+ * copy all input data to our local memory to avoid potential is= sue > >>+ * as the dsm memory is visible to guest. > > > >this comment doesn't help. > >pls replace "potential issue" with an explanation. >=20 > Okay, will change the comment to: > /* As DSM memory is mapped to guest address space so that evil guest ca= n change s/As/The/ s/that evil/an evil/ > * its content while we are doing DSM emulation. Avoid it by copying DS= M memory s/Avoid it/Avoid this/ > * to QEMU local memory > */ >=20 > > > >>+ */ > >>+ in =3D g_malloc(memory_region_size(dsm_ram_mr)); > >>+ memcpy(in, dsm_ram_addr, memory_region_size(dsm_ram_mr)); > >>+ > >>+ le32_to_cpus(&in->revision); > >>+ le32_to_cpus(&in->function); > >>+ le32_to_cpus(&in->handle); > >>+ > >>+ nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->rev= ision, > >>+ in->handle, in->function); > >>+ > >>+ out =3D g_array_new(false, true /* clear */, 1); > >>+ > >>+ if (in->revision !=3D DSM_REVISION) { > >>+ nvdimm_debug("Revision %#x is not supported, expect %#x.\n", > >>+ in->revision, DSM_REVISION); > >>+ nvdimm_dsm_write_status(out, DSM_STATUS_NOT_SUPPORTED); > >>+ goto exit; > >>+ } > >>+ > >>+ /* Handle 0 is reserved for NVDIMM Root Device. */ > >>+ if (!in->handle) { > >>+ nvdimm_dsm_write_root(in, out); > >>+ goto exit; > >>+ } > >>+ > >>+ nvdimm_dsm_write_nvdimm(in, out); > >>+ > >>+exit: > >>+ /* Write our output result to dsm memory. */ > >>+ ((dsm_out *)dsm_ram_addr)->len =3D out->len; > >>+ memcpy(((dsm_out *)dsm_ram_addr)->data, out->data, out->len); > > > >This breaks migration as memory is not dirtied. > > > >address_space_write is generally preferable to change memory. >=20 > Good point, will fix.