| Message ID | 1447671063-92665-5-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 16, 2015 at 06:51:02PM +0800, Xiao Guangrong wrote: > NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices > > There is a root device under \_SB and specified NVDIMM devices are under the > root device. Each NVDIMM device has _ADR which returns its handle used to > associate MEMDEV structure in NFIT > > Currently, we do not support any function on _DSM, that means, NVDIMM > label data has not been supported yet > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > hw/acpi/nvdimm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 98c004d..abe0daa 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -367,6 +367,90 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, > g_array_free(structures, true); > } > > +static void nvdimm_build_common_dsm(Aml *root_dev) > +{ > + Aml *method, *ifctx, *function; > + uint8_t byte_list[1]; > + > + method = aml_method("NCAL", 4); This "NCAL" needs a define as it's used in multiple places. It's really just a DSM implementation, right? Reflect this in the macro name. > + { What's this doing? > + function = aml_arg(2); > + > + /* > + * function 0 is called to inquire what functions are supported by > + * OSPM > + */ > + ifctx = aml_if(aml_equal(function, aml_int(0))); > + byte_list[0] = 0 /* No function Supported */; > + aml_append(ifctx, aml_return(aml_buffer(1, byte_list))); > + aml_append(method, ifctx); > + > + /* No function is supported yet. */ > + byte_list[0] = 1 /* Not Supported */; > + aml_append(method, aml_return(aml_buffer(1, byte_list))); > + } > + aml_append(root_dev, method); > +} > + > +static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev) > +{ > + for (; device_list; device_list = device_list->next) { > + DeviceState *dev = device_list->data; > + int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, > + NULL); > + uint32_t handle = nvdimm_slot_to_handle(slot); > + Aml *nvdimm_dev, *method; > + > + nvdimm_dev = aml_device("NV%02X", slot); > + aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle))); > + > + method = aml_method("_DSM", 4); > + { > + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), > + aml_arg(1), aml_arg(2), aml_arg(3)))); > + } > + aml_append(nvdimm_dev, method); > + > + aml_append(root_dev, nvdimm_dev); > + } > +} > + > +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, > + GArray *table_data, GArray *linker) > +{ > + Aml *ssdt, *sb_scope, *dev, *method; > + > + acpi_add_table(table_offsets, table_data); > + > + ssdt = init_aml_allocator(); > + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > + > + sb_scope = aml_scope("\\_SB"); > + > + dev = aml_device("NVDR"); > + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); Pls add a comment explaining that ACPI0012 is NVDIMM root device. Also - this will now appear for all users, e.g. windows guests will prompt users for a driver. Not nice if user didn't actually ask for nvdimm. A simple solution is to default this functionality to off by default. > + > + nvdimm_build_common_dsm(dev); > + method = aml_method("_DSM", 4); > + { > + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), > + aml_arg(1), aml_arg(2), aml_arg(3)))); > + } Some duplication here, move above to a sub-function please. > + aml_append(dev, method); > + > + nvdimm_build_nvdimm_devices(device_list, dev); > + > + aml_append(sb_scope, dev); > + > + aml_append(ssdt, sb_scope); > + /* copy AML table into ACPI tables blob and patch header there */ > + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len); > + build_header(linker, table_data, > + (void *)(table_data->data + table_data->len - ssdt->buf->len), > + "SSDT", ssdt->buf->len, 1, "NVDIMM"); > + free_aml_allocator(); > +} > + > void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > GArray *linker) > { > @@ -378,5 +462,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > return; > } > nvdimm_build_nfit(device_list, table_offsets, table_data, linker); > + nvdimm_build_ssdt(device_list, table_offsets, table_data, linker); > g_slist_free(device_list); > } > -- > 1.8.3.1 > > -- > 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 -- 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
On Mon, Nov 16, 2015 at 06:51:02PM +0800, Xiao Guangrong wrote: > NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices Forgot to mention: Pls put spec info in code comments near relevant functions, not just the log. > > There is a root device under \_SB and specified NVDIMM devices are under the > root device. Each NVDIMM device has _ADR which returns its handle used to > associate MEMDEV structure in NFIT > > Currently, we do not support any function on _DSM, that means, NVDIMM > label data has not been supported yet > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > hw/acpi/nvdimm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index 98c004d..abe0daa 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -367,6 +367,90 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, > g_array_free(structures, true); > } > > +static void nvdimm_build_common_dsm(Aml *root_dev) > +{ > + Aml *method, *ifctx, *function; > + uint8_t byte_list[1]; > + > + method = aml_method("NCAL", 4); > + { > + function = aml_arg(2); > + > + /* > + * function 0 is called to inquire what functions are supported by > + * OSPM > + */ > + ifctx = aml_if(aml_equal(function, aml_int(0))); > + byte_list[0] = 0 /* No function Supported */; > + aml_append(ifctx, aml_return(aml_buffer(1, byte_list))); > + aml_append(method, ifctx); > + > + /* No function is supported yet. */ > + byte_list[0] = 1 /* Not Supported */; > + aml_append(method, aml_return(aml_buffer(1, byte_list))); > + } > + aml_append(root_dev, method); > +} > + > +static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev) > +{ > + for (; device_list; device_list = device_list->next) { > + DeviceState *dev = device_list->data; > + int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, > + NULL); > + uint32_t handle = nvdimm_slot_to_handle(slot); > + Aml *nvdimm_dev, *method; > + > + nvdimm_dev = aml_device("NV%02X", slot); > + aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle))); > + > + method = aml_method("_DSM", 4); > + { > + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), > + aml_arg(1), aml_arg(2), aml_arg(3)))); > + } > + aml_append(nvdimm_dev, method); > + > + aml_append(root_dev, nvdimm_dev); > + } > +} > + > +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, > + GArray *table_data, GArray *linker) > +{ > + Aml *ssdt, *sb_scope, *dev, *method; So why don't we skip this completely if device list is empty? > + > + acpi_add_table(table_offsets, table_data); > + > + ssdt = init_aml_allocator(); > + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > + > + sb_scope = aml_scope("\\_SB"); > + > + dev = aml_device("NVDR"); > + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); > + > + nvdimm_build_common_dsm(dev); > + method = aml_method("_DSM", 4); > + { > + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), > + aml_arg(1), aml_arg(2), aml_arg(3)))); > + } > + aml_append(dev, method); > + > + nvdimm_build_nvdimm_devices(device_list, dev); > + > + aml_append(sb_scope, dev); > + > + aml_append(ssdt, sb_scope); > + /* copy AML table into ACPI tables blob and patch header there */ > + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len); > + build_header(linker, table_data, > + (void *)(table_data->data + table_data->len - ssdt->buf->len), > + "SSDT", ssdt->buf->len, 1, "NVDIMM"); > + free_aml_allocator(); > +} > + > void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > GArray *linker) > { > @@ -378,5 +462,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > return; > } > nvdimm_build_nfit(device_list, table_offsets, table_data, linker); > + nvdimm_build_ssdt(device_list, table_offsets, table_data, linker); > g_slist_free(device_list); > } > -- > 1.8.3.1 > > -- > 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 -- 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
On 11/30/2015 06:30 PM, Michael S. Tsirkin wrote: > On Mon, Nov 16, 2015 at 06:51:02PM +0800, Xiao Guangrong wrote: >> NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices >> >> There is a root device under \_SB and specified NVDIMM devices are under the >> root device. Each NVDIMM device has _ADR which returns its handle used to >> associate MEMDEV structure in NFIT >> >> Currently, we do not support any function on _DSM, that means, NVDIMM >> label data has not been supported yet >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> hw/acpi/nvdimm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 85 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 98c004d..abe0daa 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -367,6 +367,90 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, >> g_array_free(structures, true); >> } >> >> +static void nvdimm_build_common_dsm(Aml *root_dev) >> +{ >> + Aml *method, *ifctx, *function; >> + uint8_t byte_list[1]; >> + >> + method = aml_method("NCAL", 4); > > This "NCAL" needs a define as it's used > in multiple places. It's really just a DSM > implementation, right? Reflect this in the macro > name. > Yes, it is a common DSM method used by both root device and nvdimm devices. I will do it like this: #define NVDIMM_COMMON_DSM "NCAL" >> + { > > What's this doing? > It just a reminder that the code containing in this braces is a DSM body like a C function. However, i do not have strong opinion on it, will drop this style if you dislike it. >> + function = aml_arg(2); >> + >> + /* >> + * function 0 is called to inquire what functions are supported by >> + * OSPM >> + */ >> + ifctx = aml_if(aml_equal(function, aml_int(0))); >> + byte_list[0] = 0 /* No function Supported */; >> + aml_append(ifctx, aml_return(aml_buffer(1, byte_list))); >> + aml_append(method, ifctx); >> + >> + /* No function is supported yet. */ >> + byte_list[0] = 1 /* Not Supported */; >> + aml_append(method, aml_return(aml_buffer(1, byte_list))); >> + } >> + aml_append(root_dev, method); >> +} >> + >> +static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev) >> +{ >> + for (; device_list; device_list = device_list->next) { >> + DeviceState *dev = device_list->data; >> + int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, >> + NULL); >> + uint32_t handle = nvdimm_slot_to_handle(slot); >> + Aml *nvdimm_dev, *method; >> + >> + nvdimm_dev = aml_device("NV%02X", slot); >> + aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle))); >> + >> + method = aml_method("_DSM", 4); >> + { >> + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), >> + aml_arg(1), aml_arg(2), aml_arg(3)))); >> + } >> + aml_append(nvdimm_dev, method); >> + >> + aml_append(root_dev, nvdimm_dev); >> + } >> +} >> + >> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, >> + GArray *table_data, GArray *linker) >> +{ >> + Aml *ssdt, *sb_scope, *dev, *method; >> + >> + acpi_add_table(table_offsets, table_data); >> + >> + ssdt = init_aml_allocator(); >> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); >> + >> + sb_scope = aml_scope("\\_SB"); >> + >> + dev = aml_device("NVDR"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); > > Pls add a comment explaining that ACPI0012 is NVDIMM root device. Okay, will add these comment: /* * NVDIMM is introduced in ACPI 6.0 9.20 NVDIMM Devices which defines an NVDIMM * root device under _SB scope with a _HID of “ACPI0012”. For each NVDIMM present * or intended to be supported by platform, platform firmware also exposes an ACPI * Namespace Device under the root device. */ > > Also - this will now appear for all users, e.g. > windows guests will prompt users for a driver. > Not nice if user didn't actually ask for nvdimm. > > A simple solution is to default this functionality > to off by default. > Okay, will disable nvdimm on default in the next version. >> + >> + nvdimm_build_common_dsm(dev); >> + method = aml_method("_DSM", 4); >> + { >> + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), >> + aml_arg(1), aml_arg(2), aml_arg(3)))); >> + } > > Some duplication here, move above to a sub-function please. Okay, will add a function named nvdimm_build_device_dsm() to do these things. Thanks! -- 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
On 11/30/2015 06:32 PM, Michael S. Tsirkin wrote: > On Mon, Nov 16, 2015 at 06:51:02PM +0800, Xiao Guangrong wrote: >> NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices > > Forgot to mention: > > Pls put spec info in code comments near > relevant functions, not just the log. > Sure, good to me. >> + >> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, >> + GArray *table_data, GArray *linker) >> +{ >> + Aml *ssdt, *sb_scope, *dev, *method; > > So why don't we skip this completely if device list is empty? Yes, it is exactly what we did: void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, GArray *linker) { GSList *device_list; /* no NVDIMM device is plugged. */ device_list = nvdimm_get_plugged_device_list(); if (!device_list) { return; } nvdimm_build_nfit(device_list, table_offsets, table_data, linker); + nvdimm_build_ssdt(device_list, table_offsets, table_data, linker); g_slist_free(device_list); } -- 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 98c004d..abe0daa 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -367,6 +367,90 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, g_array_free(structures, true); } +static void nvdimm_build_common_dsm(Aml *root_dev) +{ + Aml *method, *ifctx, *function; + uint8_t byte_list[1]; + + method = aml_method("NCAL", 4); + { + function = aml_arg(2); + + /* + * function 0 is called to inquire what functions are supported by + * OSPM + */ + ifctx = aml_if(aml_equal(function, aml_int(0))); + byte_list[0] = 0 /* No function Supported */; + aml_append(ifctx, aml_return(aml_buffer(1, byte_list))); + aml_append(method, ifctx); + + /* No function is supported yet. */ + byte_list[0] = 1 /* Not Supported */; + aml_append(method, aml_return(aml_buffer(1, byte_list))); + } + aml_append(root_dev, method); +} + +static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev) +{ + for (; device_list; device_list = device_list->next) { + DeviceState *dev = device_list->data; + int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, + NULL); + uint32_t handle = nvdimm_slot_to_handle(slot); + Aml *nvdimm_dev, *method; + + nvdimm_dev = aml_device("NV%02X", slot); + aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle))); + + method = aml_method("_DSM", 4); + { + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), + aml_arg(1), aml_arg(2), aml_arg(3)))); + } + aml_append(nvdimm_dev, method); + + aml_append(root_dev, nvdimm_dev); + } +} + +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, + GArray *table_data, GArray *linker) +{ + Aml *ssdt, *sb_scope, *dev, *method; + + acpi_add_table(table_offsets, table_data); + + ssdt = init_aml_allocator(); + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); + + sb_scope = aml_scope("\\_SB"); + + dev = aml_device("NVDR"); + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); + + nvdimm_build_common_dsm(dev); + method = aml_method("_DSM", 4); + { + aml_append(method, aml_return(aml_call4("NCAL", aml_arg(0), + aml_arg(1), aml_arg(2), aml_arg(3)))); + } + aml_append(dev, method); + + nvdimm_build_nvdimm_devices(device_list, dev); + + aml_append(sb_scope, dev); + + aml_append(ssdt, sb_scope); + /* copy AML table into ACPI tables blob and patch header there */ + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len); + build_header(linker, table_data, + (void *)(table_data->data + table_data->len - ssdt->buf->len), + "SSDT", ssdt->buf->len, 1, "NVDIMM"); + free_aml_allocator(); +} + void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, GArray *linker) { @@ -378,5 +462,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, return; } nvdimm_build_nfit(device_list, table_offsets, table_data, linker); + nvdimm_build_ssdt(device_list, table_offsets, table_data, linker); g_slist_free(device_list); }
NVDIMM devices is defined in ACPI 6.0 9.20 NVDIMM Devices There is a root device under \_SB and specified NVDIMM devices are under the root device. Each NVDIMM device has _ADR which returns its handle used to associate MEMDEV structure in NFIT Currently, we do not support any function on _DSM, that means, NVDIMM label data has not been supported yet Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- hw/acpi/nvdimm.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)