From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo58p-0002dd-AX for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:46:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zo58l-0007Vz-8V for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:45:59 -0400 Received: from mga03.intel.com ([134.134.136.65]:49121) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zo58k-0007Vv-Rp for qemu-devel@nongnu.org; Mon, 19 Oct 2015 03:45:55 -0400 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> <20151019100041-mutt-send-email-mst@redhat.com> From: Xiao Guangrong Message-ID: <56249E29.9010000@linux.intel.com> Date: Mon, 19 Oct 2015 15:39:21 +0800 MIME-Version: 1.0 In-Reply-To: <20151019100041-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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: "Michael S. Tsirkin" 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 10/19/2015 03:06 PM, Michael S. Tsirkin wrote: > On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote: >> >> >> 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 root >>>> device and only 3 functions are support for NVDIMM device, >>>> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and >>>> DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow 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 slot) >>>> return nvdimm_slot_to_spa_index(slot) + 1; >>>> } >>>> >>>> +static NVDIMMDevice >>>> +*nvdimm_get_device_by_handle(GSList *list, uint32_t handle) >>>> +{ >>>> + for (; list; list = list->next) { >>>> + NVDIMMDevice *nvdimm = list->data; >>>> + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, >>>> + NULL); >>>> + >>>> + if (nvdimm_slot_to_handle(slot) == 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. >> >> Okay, will do. Sorry for i missed it. >> >>> >>>> >>>> +enum { >>>> + DSM_FUN_IMPLEMENTED = 0, >>>> + >>>> + /* NVDIMM Root Device Functions */ >>>> + DSM_ROOT_DEV_FUN_ARS_CAP = 1, >>>> + DSM_ROOT_DEV_FUN_ARS_START = 2, >>>> + DSM_ROOT_DEV_FUN_ARS_QUERY = 3, >>>> + >>>> + /* NVDIMM Device (non-root) Functions */ >>>> + DSM_DEV_FUN_SMART = 1, >>>> + DSM_DEV_FUN_SMART_THRESHOLD = 2, >>>> + DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3, >>>> + DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4, >>>> + DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5, >>>> + DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6, >>>> + DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7, >>>> + DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8, >>>> + DSM_DEV_FUN_VENDOR_SPECIFIC = 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 *out) >> >> I will follow your ‘single use’ comments below, these definitions 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 Size. */: > > 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. Okay. > >> nvdimm_dsm_func_label_size(); >> case ... >> ... >> }; >> >>> >>> >>> >>>> + >>>> +enum { >>>> + /* Common return status codes. */ >>>> + DSM_STATUS_SUCCESS = 0, /* Success */ >>>> + DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */ >>>> + >>>> + /* NVDIMM Root Device _DSM function return status codes*/ >>>> + DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2, /* Invalid Input Parameters */ >>>> + DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific >>>> + Error */ >>>> + >>>> + /* NVDIMM Device (non-root) _DSM function return status codes*/ >>>> + DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2, /* Non-Existing Memory Device */ >>>> + DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */ >>>> + DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 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 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_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, unsigned 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 = 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 = DSM_STATUS_NOT_SUPPORTED; >>>> + >>>> + /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */ >>>> + if (in->function == DSM_FUN_IMPLEMENTED) { >>>> + uint64_t cmd_list = 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 = nvdimm_get_plugged_device_list(); >>>> + NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(list, in->handle); >>>> + uint32_t status = 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 = cpu_to_le64(DIMM_SUPPORT_FUN); >>>> + g_array_append_vals(out, &cmd_list, sizeof(cmd_list)); >>>> + goto free; >>>> + default: >>>> + status = 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 = 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 operation: >> /* >> * The DSM memory is used for both OSPM saves its input parameter and QEMU >> * saves its output result. >> */ > > Fix up the english here pls: >> * The DSM memory has two uses: OSPM saves its input parameter there, QEMU >> * uses it to save its output result. > Sorry for my English, will fix. > >>> >>>> + >>>> if (val != NOTIFY_VALUE) { >>>> fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val); >>>> } >>>> + >>>> + dsm_ram_mr = memory_region_find(&state->mr, getpagesize(), >>>> + getpagesize()).mr; >>>> + dsm_ram_addr = 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) == getpagesize()); > > No, the point is to make sure getpagesize is big enough to hold > the structure. Got it, will change it to: /* * The DSM memory should be big enough to contain Input parameters and * output result. */ assert(memory_region_size(dsm_ram_mr) >= sizeof(dsm_in) && memory_region_size(dsm_ram_mr) >= sizeof(dsm_out)); > >>> >>>> + >>>> + /* >>>> + * copy all input data to our local memory to avoid potential issue >>>> + * 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 can change > > s/As/The/ > s/that evil/an evil/ > >> * its content while we are doing DSM emulation. Avoid it by copying DSM memory > > s/Avoid it/Avoid this/ > >> * to QEMU local memory >> */ Thanks for your fix.