| Message ID | 1445216059-88521-29-git-send-email-guangrong.xiao@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
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 <guangrong.xiao@linux.intel.com> > --- > 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. > > +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. Please list exact names as they appear in spec so they can be searched for. > + > +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". > + * 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. You will be able to drop all _FUN_ macros too. > 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? > + assert(!out->len); But dsm itself can be part of a bigger table. So don't do 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. > +} > + > +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. > + > + 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 (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. > + > + /* > + * 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. > + */ > + in = 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->revision, > + in->handle, in->function); > + > + out = g_array_new(false, true /* clear */, 1); > + > + if (in->revision != 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 = 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. > + > + g_free(in); > + g_array_free(out, true); > + memory_region_unref(dsm_ram_mr); > } > > static const MemoryRegionOps nvdimm_dsm_ops = { > @@ -547,7 +725,8 @@ static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list, > */ > BUILD_DSM_METHOD(dev, method, > handle /* NVDIMM Device Handle */, > - 3 /* Invalid Input Parameters */, > + DSM_DEV_STATUS_INVALID_PARAS, /* error code if UUID > + is not matched. */ > "4309AC30-0D11-11E4-9191-0800200C9A66" > /* UUID for NVDIMM Devices. */); > > @@ -669,7 +848,8 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, > */ > BUILD_DSM_METHOD(dev, method, > 0 /* 0 is reserved for NVDIMM Root Device*/, > - 2 /* Invalid Input Parameters */, > + DSM_ROOT_DEV_STATUS_INVALID_PARAS, /* error code if > + UUID is not matched. */ > "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" > /* UUID for NVDIMM Root Devices. */); > > -- > 1.8.3.1
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 <guangrong.xiao@linux.intel.com> >> --- >> 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. */: 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. */ > >> + >> 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()); > >> + >> + /* >> + * 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 * its content while we are doing DSM emulation. Avoid it by copying DSM memory * to QEMU local memory */ > >> + */ >> + in = 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->revision, >> + in->handle, in->function); >> + >> + out = g_array_new(false, true /* clear */, 1); >> + >> + if (in->revision != 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 = 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.
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 <guangrong.xiao@linux.intel.com> > >>--- > >> 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. > 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. > > > >>+ > >> 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. > > > >>+ > >>+ /* > >>+ * 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 > */ > > > > >>+ */ > >>+ in = 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->revision, > >>+ in->handle, in->function); > >>+ > >>+ out = g_array_new(false, true /* clear */, 1); > >>+ > >>+ if (in->revision != 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 = 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.
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 <guangrong.xiao@linux.intel.com> >>>> --- >>>> 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.
On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: > +exit: > + /* Write our output result to dsm memory. */ > + ((dsm_out *)dsm_ram_addr)->len = out->len; Missing byteswap? I thought you were going to remove this field because it wasn't needed by the guest.
On Tue, Oct 20, 2015 at 04:51:49PM +0100, Stefan Hajnoczi wrote: > On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: > > +exit: > > + /* Write our output result to dsm memory. */ > > + ((dsm_out *)dsm_ram_addr)->len = out->len; > > Missing byteswap? That's why I'd prefer no structures, using build_append_int_noprefix, unless the structure is used outside ACPI as well. > I thought you were going to remove this field because it wasn't needed > by the guest.
On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote: > On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: >> +exit: >> + /* Write our output result to dsm memory. */ >> + ((dsm_out *)dsm_ram_addr)->len = out->len; > > Missing byteswap? > > I thought you were going to remove this field because it wasn't needed > by the guest. > The @len is the size of _DSM result buffer, for example, for the function of DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code how much size of memory we need to return to the _DSM caller. In _DSM code, it's handled like this: "RLEN" is @len, “OBUF” is the left memory in DSM page. /* get @len*/ aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); /* @len << 3 to get bits. */ aml_append(method, aml_store(aml_shiftleft(aml_local(6), aml_int(3)), aml_local(6))); /* get @len << 3 bits from OBUF, and return it to the caller. */ aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), aml_local(6) , "OBUF")); Since @len is our internally used, it's not return to guest, so i did not do byteswap here.
On 10/21/2015 12:26 AM, Xiao Guangrong wrote: > > > On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote: >> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: >>> +exit: >>> + /* Write our output result to dsm memory. */ >>> + ((dsm_out *)dsm_ram_addr)->len = out->len; >> >> Missing byteswap? >> >> I thought you were going to remove this field because it wasn't needed >> by the guest. >> > > The @len is the size of _DSM result buffer, for example, for the function of > DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for > DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code Sorry, s/DSM_DEV_FUN_NAMESPACE_LABEL_SIZE/DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA > how much size of memory we need to return to the _DSM caller. > > In _DSM code, it's handled like this: > > "RLEN" is @len, “OBUF” is the left memory in DSM page. > > /* get @len*/ > aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > /* @len << 3 to get bits. */ > aml_append(method, aml_store(aml_shiftleft(aml_local(6), > aml_int(3)), aml_local(6))); > > /* get @len << 3 bits from OBUF, and return it to the caller. */ > aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > aml_local(6) , "OBUF")); > > Since @len is our internally used, it's not return to guest, so i did not do > byteswap here.
On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote: > > > On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote: > >On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: > >>+exit: > >>+ /* Write our output result to dsm memory. */ > >>+ ((dsm_out *)dsm_ram_addr)->len = out->len; > > > >Missing byteswap? > > > >I thought you were going to remove this field because it wasn't needed > >by the guest. > > > > The @len is the size of _DSM result buffer, for example, for the function of > DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for > DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code > how much size of memory we need to return to the _DSM caller. > > In _DSM code, it's handled like this: > > "RLEN" is @len, “OBUF” is the left memory in DSM page. > > /* get @len*/ > aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > /* @len << 3 to get bits. */ > aml_append(method, aml_store(aml_shiftleft(aml_local(6), > aml_int(3)), aml_local(6))); > > /* get @len << 3 bits from OBUF, and return it to the caller. */ > aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > aml_local(6) , "OBUF")); > > Since @len is our internally used, it's not return to guest, so i did not do > byteswap here. I am not familiar with the ACPI details, but I think this emits bytecode that will be run by the guest's ACPI interpreter? You still need to define the endianness of fields since QEMU and the guest could have different endianness. In other words, will the following work if a big-endian ppc host is running a little-endian x86 guest? ((dsm_out *)dsm_ram_addr)->len = out->len; Stefan
On 10/21/2015 06:49 PM, Stefan Hajnoczi wrote: > On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote: >> >> >> On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote: >>> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: >>>> +exit: >>>> + /* Write our output result to dsm memory. */ >>>> + ((dsm_out *)dsm_ram_addr)->len = out->len; >>> >>> Missing byteswap? >>> >>> I thought you were going to remove this field because it wasn't needed >>> by the guest. >>> >> >> The @len is the size of _DSM result buffer, for example, for the function of >> DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for >> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code >> how much size of memory we need to return to the _DSM caller. >> >> In _DSM code, it's handled like this: >> >> "RLEN" is @len, “OBUF” is the left memory in DSM page. >> >> /* get @len*/ >> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); >> /* @len << 3 to get bits. */ >> aml_append(method, aml_store(aml_shiftleft(aml_local(6), >> aml_int(3)), aml_local(6))); >> >> /* get @len << 3 bits from OBUF, and return it to the caller. */ >> aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), >> aml_local(6) , "OBUF")); >> >> Since @len is our internally used, it's not return to guest, so i did not do >> byteswap here. > > I am not familiar with the ACPI details, but I think this emits bytecode > that will be run by the guest's ACPI interpreter? > > You still need to define the endianness of fields since QEMU and the > guest could have different endianness. > > In other words, will the following work if a big-endian ppc host is > running a little-endian x86 guest? > > ((dsm_out *)dsm_ram_addr)->len = out->len; > Er... If we do byteswap in QEMU then it is also needed in ASL code, however, ASL lacks this kind of instruction. I guess ACPI interpreter is smart enough to change value to Littel-Endian for all 2 bytes / 4 bytes / 8 bytes accesses I will do the change in next version, thanks for you pointing it out, Stefan!
On Wed, 21 Oct 2015 21:32:38 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > On 10/21/2015 06:49 PM, Stefan Hajnoczi wrote: > > On Wed, Oct 21, 2015 at 12:26:35AM +0800, Xiao Guangrong wrote: > >> > >> > >> On 10/20/2015 11:51 PM, Stefan Hajnoczi wrote: > >>> On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote: > >>>> +exit: > >>>> + /* Write our output result to dsm memory. */ > >>>> + ((dsm_out *)dsm_ram_addr)->len = out->len; > >>> > >>> Missing byteswap? > >>> > >>> I thought you were going to remove this field because it wasn't needed > >>> by the guest. > >>> > >> > >> The @len is the size of _DSM result buffer, for example, for the function of > >> DSM_FUN_IMPLEMENTED the result buffer is 8 bytes, and for > >> DSM_DEV_FUN_NAMESPACE_LABEL_SIZE the buffer size is 4 bytes. It tells ASL code > >> how much size of memory we need to return to the _DSM caller. > >> > >> In _DSM code, it's handled like this: > >> > >> "RLEN" is @len, “OBUF” is the left memory in DSM page. > >> > >> /* get @len*/ > >> aml_append(method, aml_store(aml_name("RLEN"), aml_local(6))); > >> /* @len << 3 to get bits. */ > >> aml_append(method, aml_store(aml_shiftleft(aml_local(6), > >> aml_int(3)), aml_local(6))); > >> > >> /* get @len << 3 bits from OBUF, and return it to the caller. */ > >> aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > >> aml_local(6) , "OBUF")); > >> > >> Since @len is our internally used, it's not return to guest, so i did not do > >> byteswap here. > > > > I am not familiar with the ACPI details, but I think this emits bytecode > > that will be run by the guest's ACPI interpreter? > > > > You still need to define the endianness of fields since QEMU and the > > guest could have different endianness. > > > > In other words, will the following work if a big-endian ppc host is > > running a little-endian x86 guest? > > > > ((dsm_out *)dsm_ram_addr)->len = out->len; > > > > Er... If we do byteswap in QEMU then it is also needed in ASL code, however, > ASL lacks this kind of instruction. I guess ACPI interpreter is smart enough > to change value to Littel-Endian for all 2 bytes / 4 bytes / 8 bytes accesses > > I will do the change in next version, thanks for you pointing it out, Stefan! According to ACPI spec integers encoded as little endian, so QEMU needs to convert fields accessible by OSPM to it (i.e. do cpu_to_le()) > > -- > 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 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 +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, +}; + +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: + * 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) +#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)) + 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. */ + assert(!out->len); + + status = cpu_to_le32(status); + g_array_append_vals(out, &status, sizeof(status)); +} + +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); + + 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; + 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); + + /* + * copy all input data to our local memory to avoid potential issue + * as the dsm memory is visible to guest. + */ + in = 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->revision, + in->handle, in->function); + + out = g_array_new(false, true /* clear */, 1); + + if (in->revision != 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 = out->len; + memcpy(((dsm_out *)dsm_ram_addr)->data, out->data, out->len); + + g_free(in); + g_array_free(out, true); + memory_region_unref(dsm_ram_mr); } static const MemoryRegionOps nvdimm_dsm_ops = { @@ -547,7 +725,8 @@ static void build_nvdimm_devices(NVDIMMState *state, GSList *device_list, */ BUILD_DSM_METHOD(dev, method, handle /* NVDIMM Device Handle */, - 3 /* Invalid Input Parameters */, + DSM_DEV_STATUS_INVALID_PARAS, /* error code if UUID + is not matched. */ "4309AC30-0D11-11E4-9191-0800200C9A66" /* UUID for NVDIMM Devices. */); @@ -669,7 +848,8 @@ static void nvdimm_build_acpi_devices(NVDIMMState *state, GSList *device_list, */ BUILD_DSM_METHOD(dev, method, 0 /* 0 is reserved for NVDIMM Root Device*/, - 2 /* Invalid Input Parameters */, + DSM_ROOT_DEV_STATUS_INVALID_PARAS, /* error code if + UUID is not matched. */ "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" /* UUID for NVDIMM Root Devices. */);
__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 <guangrong.xiao@linux.intel.com> --- hw/acpi/nvdimm.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 182 insertions(+), 2 deletions(-)