From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH v4 28/33] nvdimm acpi: support DSM_FUN_IMPLEMENTED function Date: Mon, 19 Oct 2015 12:39:24 +0800 Message-ID: <562473FC.1000309@linux.intel.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE 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 To: "Michael S. Tsirkin" Return-path: Received: from mga03.intel.com ([134.134.136.65]:34860 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750868AbbJSEpq (ORCPT ); Mon, 19 Oct 2015 00:45:46 -0400 In-Reply-To: <20151018205030-mutt-send-email-mst@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 ro= ot >> device and only 3 functions are support for NVDIMM device, >> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DA= TA and >> DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only a= llow 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 sl= ot) >> 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_S= LOT_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 Rang= e >> * Structure >> @@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structure= s, GArray *table_offsets, >> /* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt= */ >> #define NOTIFY_VALUE 0x99 > > Again, please prefix everything consistently. Okay, will do. Sorry for i missed it. > >> >> +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. > Yes. > Please list exact names as they appear in spec so > they can be searched for. The spec reference was at where this _FUN_ is used, eg: /* * 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) 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: switch (function) { case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label S= ize. */: nvdimm_dsm_func_label_size(); case ... =2E.. }; > > > >> + >> +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 Pa= rameters */ >> + DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR =3D 3, /* Function-= Specific >> + Error */ >> + >> + /* NVDIMM Device (non-root) _DSM function return status codes*/ >> + DSM_DEV_STATUS_NON_EXISTING_MEM_DEV =3D 2, /* Non-Existing Mem= ory Device */ >> + DSM_DEV_STATUS_INVALID_PARAS =3D 3, /* Invalid Input Pa= rameters */ >> + DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR =3D 4, /* Vendor Specific = Error */ >> +}; >> + >> +/* 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): = Return >> + * Value Information: > > Drop "please refer to". Okay. > >> + * if set to zero, no functions are supported (other than functio= n zero) >> + * for the specified UUID and Revision ID. If set to one, at leas= t 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_= DATA) \ >> + | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_= DATA)) >> + > > 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. Okay. Good to me. > You will be able to drop all _FUN_ macros too. Yes, it's good for code reduction. > > >> 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, un= signed 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? Yes... > >> + assert(!out->len); > > > But dsm itself can be part of a bigger table. > So don't do it. Okay, will drop it. > >> + >> + 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. Okay, will use it instead. > >> +} >> + >> +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 Metho= d) */ >> + 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. > Yes, it is good to me, will follow it. > >> + >> + 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->= handle); >> + 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 Metho= d) */ >> + 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. If it's defined as "dsm_out *", that will make "copy(in, dsm_ram_addr..= )" little strange. 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 = QEMU * saves 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. Okay, will add this: assert(memory_region_size(dsm_ram_mr) =3D=3D getpagesize()); > >> + >> + /* >> + * copy all input data to our local memory to avoid potential i= ssue >> + * as the dsm memory is visible to guest. > > this comment doesn't help. > pls replace "potential issue" with an explanation. Okay, will change the comment to: /* As DSM memory is mapped to guest address space so that evil guest ca= n change * its content while we are doing DSM emulation. Avoid it by copying D= SM memory * to QEMU local memory */ > >> + */ >> + 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->re= vision, >> + 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. Good point, will fix.