| Message ID | 1446455617-129562-28-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2 Nov 2015 17:13:29 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 > > We reserve handle 0 for root device. In this patch, we save handle, handle, > arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch > > Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > --- > hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 184 insertions(+) > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index dd84e5f..53ed675 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, > g_array_free(structures, true); > } > > +struct NvdimmDsmIn { > + uint32_t handle; > + uint32_t revision; > + uint32_t function; > + /* the remaining size in the page is used by arg3. */ > + uint8_t arg3[0]; > +} QEMU_PACKED; > +typedef struct NvdimmDsmIn NvdimmDsmIn; > + > static uint64_t > nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > { > @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > static void > nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > { > + fprintf(stderr, "BUG: we never write DSM notification IO Port.\n"); it doesn't seem like this hunk belongs here > } > > static const MemoryRegionOps nvdimm_dsm_ops = { > @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, MemoryRegion *io, > memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr); > } > > +#define BUILD_STA_METHOD(_dev_, _method_) \ > + do { \ > + _method_ = aml_method("_STA", 0); \ > + aml_append(_method_, aml_return(aml_int(0x0f))); \ > + aml_append(_dev_, _method_); \ > + } while (0) _STA doesn't have any logic here so drop macro and just replace its call sites with: aml_append(foo_dev, aml_name_decl("_STA", aml_int(0xf)); > + > +#define BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_) \ > + do { \ > + Aml *ifctx, *uuid; \ > + _method_ = aml_method("_DSM", 4); \ > + /* check UUID if it is we expect, return the errorcode if not.*/ \ > + uuid = aml_touuid(_uuid_); \ > + ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid))); \ > + aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */))); \ > + aml_append(method, ifctx); \ > + aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \ > + aml_arg(1), aml_arg(2), aml_arg(3)))); \ > + aml_append(_dev_, _method_); \ > + } while (0) > + > +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ > + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) > + > +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ > + BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_) > + > +static void build_nvdimm_devices(GSList *device_list, Aml *root_dev) > +{ > + for (; device_list; device_list = device_list->next) { > + NVDIMMDevice *nvdimm = device_list->data; > + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, > + NULL); > + uint32_t handle = nvdimm_slot_to_handle(slot); > + Aml *dev, *method; > + > + dev = aml_device("NV%02X", slot); > + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); > + > + BUILD_STA_METHOD(dev, method); > + > + /* > + * Chapter 4: _DSM Interface for NVDIMM Device (non-root) - Example > + * in DSM Spec Rev1. > + */ > + BUILD_DSM_METHOD(dev, method, > + handle /* NVDIMM Device Handle */, > + "4309AC30-0D11-11E4-9191-0800200C9A66" > + /* UUID for NVDIMM Devices. */); this will add N-bytes * #NVDIMMS in worst case. Please drop macro and just consolidate this method into _DSM method of parent scope and then call it from here like this: Method(_DSM, 4) Return(^_DSM(Arg[0-3])) > + > + aml_append(root_dev, dev); > + } > +} > + > +static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) > +{ > + Aml *dev, *method, *field; > + uint64_t page_size = TARGET_PAGE_SIZE; > + > + dev = aml_device("NVDR"); > + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); > + > + /* map DSM memory and IO into ACPI namespace. */ > + aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO, > + NVDIMM_ACPI_IO_BASE, NVDIMM_ACPI_IO_LEN)); > + aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY, > + NVDIMM_ACPI_MEM_BASE, page_size)); > + > + /* > + * DSM notifier: > + * @NOTI: Read it will notify QEMU that _DSM method is being > + * called and the parameters can be found in NvdimmDsmIn. > + * The value read from it is the buffer size of DSM output > + * filled by QEMU. > + */ > + field = aml_field("NPIO", AML_DWORD_ACC, AML_PRESERVE); > + BUILD_FIELD_UNIT_SIZE(field, sizeof(uint32_t), "NOTI"); > + aml_append(dev, field); > + > + /* > + * DSM input: > + * @HDLE: store device's handle, it's zero if the _DSM call happens > + * on NVDIMM Root Device. > + * @REVS: store the Arg1 of _DSM call. > + * @FUNC: store the Arg2 of _DSM call. > + * @ARG3: store the Arg3 of _DSM call. > + * > + * They are RAM mapping on host so that these accesses never cause > + * VM-EXIT. > + */ > + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); > + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, handle, "HDLE"); > + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, revision, "REVS"); > + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, function, "FUNC"); > + BUILD_FIELD_UNIT_SIZE(field, page_size - offsetof(NvdimmDsmIn, arg3), > + "ARG3"); These macros don't make code any better and one has to jump to their definition every time one sees it to figure out what it's doing. Please don't hide code behind macros and just replace them with aml_foo() here and at other places in this patch. > + aml_append(dev, field); > + > + /* > + * DSM output: > + * @ODAT: the buffer QEMU uses to store the result, the actual size > + * filled by QEMU is the value read from NOT1. > + * > + * Since the page is reused by both input and out, the input data > + * will be lost after storing new result into @ODAT. > + */ > + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); > + BUILD_FIELD_UNIT_SIZE(field, page_size, "ODAT"); > + aml_append(dev, field); > + > + method = aml_method_serialized("NCAL", 4); > + { > + Aml *buffer_size = aml_local(0); > + > + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); > + aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); > + aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); > + > + /* > + * transfer control to QEMU and the buffer size filled by > + * QEMU is returned. > + */ > + aml_append(method, aml_store(aml_name("NOTI"), buffer_size)); > + > + aml_append(method, aml_store(aml_shiftleft(buffer_size, > + aml_int(3)), buffer_size)); > + > + aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > + buffer_size , "OBUF")); > + aml_append(method, aml_concatenate(aml_buffer(0, NULL), > + aml_name("OBUF"), aml_local(1))); > + aml_append(method, aml_return(aml_local(1))); > + } > + aml_append(dev, method); > + > + BUILD_STA_METHOD(dev, method); > + > + /* > + * Chapter 3: _DSM Interface for NVDIMM Root Device - Example in DSM > + * Spec Rev1. > + */ > + BUILD_DSM_METHOD(dev, method, > + 0 /* 0 is reserved for NVDIMM Root Device*/, > + "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" > + /* UUID for NVDIMM Root Devices. */); > + > + build_nvdimm_devices(device_list, dev); > + > + aml_append(sb_scope, dev); > +} > + > +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, > + GArray *table_data, GArray *linker) > +{ > + Aml *ssdt, *sb_scope; > + > + acpi_add_table(table_offsets, table_data); > + > + ssdt = init_aml_allocator(); > + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > + > + sb_scope = aml_scope("\\_SB"); > + nvdimm_build_acpi_devices(device_list, sb_scope); is there need for dedicated nvdimm_build_acpi_devices()? Is it reused somewhere else? If it's not then just inline it here. > + > + 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); It's not ok to have several SSDT tables with exact same signature. how about extending build_header(..., oem_table_id)? You can set it to NULL to get original behavior but provide NVDIMM specific id for this table. for example "NVDIMM" > + free_aml_allocator(); > +} > + > void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > GArray *linker) > { > @@ -414,5 +597,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > } > > 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
On 11/03/2015 09:13 PM, Igor Mammedov wrote: > On Mon, 2 Nov 2015 17:13:29 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 >> >> We reserve handle 0 for root device. In this patch, we save handle, handle, >> arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch >> >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >> --- >> hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 184 insertions(+) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index dd84e5f..53ed675 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, >> g_array_free(structures, true); >> } >> >> +struct NvdimmDsmIn { >> + uint32_t handle; >> + uint32_t revision; >> + uint32_t function; >> + /* the remaining size in the page is used by arg3. */ >> + uint8_t arg3[0]; >> +} QEMU_PACKED; >> +typedef struct NvdimmDsmIn NvdimmDsmIn; >> + >> static uint64_t >> nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >> { >> @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >> static void >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> { >> + fprintf(stderr, "BUG: we never write DSM notification IO Port.\n"); > it doesn't seem like this hunk belongs here Er, we have changed the logic: - others: 1) the buffer length is directly got from IO read rather than got from dsm memory [ This has documented in v5's changelog. ] So, the IO write is replaced by IO read, nvdimm_dsm_write() should not be triggered. > >> } >> >> static const MemoryRegionOps nvdimm_dsm_ops = { >> @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, MemoryRegion *io, >> memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr); >> } >> >> +#define BUILD_STA_METHOD(_dev_, _method_) \ >> + do { \ >> + _method_ = aml_method("_STA", 0); \ >> + aml_append(_method_, aml_return(aml_int(0x0f))); \ >> + aml_append(_dev_, _method_); \ >> + } while (0) > _STA doesn't have any logic here so drop macro and just > replace its call sites with: Okay, I was just wanting to save some code lines. I will drop this macro. > > aml_append(foo_dev, aml_name_decl("_STA", aml_int(0xf)); _STA is required as a method with zero argument but this statement just define a object. It is okay? > > >> + >> +#define BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_) \ >> + do { \ >> + Aml *ifctx, *uuid; \ >> + _method_ = aml_method("_DSM", 4); \ >> + /* check UUID if it is we expect, return the errorcode if not.*/ \ >> + uuid = aml_touuid(_uuid_); \ >> + ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid))); \ >> + aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */))); \ >> + aml_append(method, ifctx); \ >> + aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \ >> + aml_arg(1), aml_arg(2), aml_arg(3)))); \ >> + aml_append(_dev_, _method_); \ >> + } while (0) >> + >> +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ >> + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) >> + >> +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ >> + BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_) >> + >> +static void build_nvdimm_devices(GSList *device_list, Aml *root_dev) >> +{ >> + for (; device_list; device_list = device_list->next) { >> + NVDIMMDevice *nvdimm = device_list->data; >> + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, >> + NULL); >> + uint32_t handle = nvdimm_slot_to_handle(slot); >> + Aml *dev, *method; >> + >> + dev = aml_device("NV%02X", slot); >> + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); >> + >> + BUILD_STA_METHOD(dev, method); >> + >> + /* >> + * Chapter 4: _DSM Interface for NVDIMM Device (non-root) - Example >> + * in DSM Spec Rev1. >> + */ >> + BUILD_DSM_METHOD(dev, method, >> + handle /* NVDIMM Device Handle */, >> + "4309AC30-0D11-11E4-9191-0800200C9A66" >> + /* UUID for NVDIMM Devices. */); > this will add N-bytes * #NVDIMMS in worst case. > Please drop macro and just consolidate this method into _DSM method of parent scope > and then call it from here like this: > Method(_DSM, 4) > Return(^_DSM(Arg[0-3])) Parent _DSM can not be directly called as _DSM in parent requires different UUID. UUID is not saved in dsm memory so that UUID verification should be done in AML. This macro, BUILD_DSM_METHOD(), build its _DSM call and check if UUID is valid, if not, it returns error code 1 (Not Supoorted), otherwise it call the common method NCAL which saves input parameters into dsm memory and trigger IO exit. It seems no byte is wasted. No? > >> + >> + aml_append(root_dev, dev); >> + } >> +} >> + >> +static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) >> +{ >> + Aml *dev, *method, *field; >> + uint64_t page_size = TARGET_PAGE_SIZE; >> + >> + dev = aml_device("NVDR"); >> + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); >> + >> + /* map DSM memory and IO into ACPI namespace. */ >> + aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO, >> + NVDIMM_ACPI_IO_BASE, NVDIMM_ACPI_IO_LEN)); >> + aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY, >> + NVDIMM_ACPI_MEM_BASE, page_size)); >> + >> + /* >> + * DSM notifier: >> + * @NOTI: Read it will notify QEMU that _DSM method is being >> + * called and the parameters can be found in NvdimmDsmIn. >> + * The value read from it is the buffer size of DSM output >> + * filled by QEMU. >> + */ >> + field = aml_field("NPIO", AML_DWORD_ACC, AML_PRESERVE); >> + BUILD_FIELD_UNIT_SIZE(field, sizeof(uint32_t), "NOTI"); >> + aml_append(dev, field); >> + >> + /* >> + * DSM input: >> + * @HDLE: store device's handle, it's zero if the _DSM call happens >> + * on NVDIMM Root Device. >> + * @REVS: store the Arg1 of _DSM call. >> + * @FUNC: store the Arg2 of _DSM call. >> + * @ARG3: store the Arg3 of _DSM call. >> + * >> + * They are RAM mapping on host so that these accesses never cause >> + * VM-EXIT. >> + */ >> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, handle, "HDLE"); >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, revision, "REVS"); >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, function, "FUNC"); >> + BUILD_FIELD_UNIT_SIZE(field, page_size - offsetof(NvdimmDsmIn, arg3), >> + "ARG3"); > These macros don't make code any better and one has to jump to their > definition every time one sees it to figure out what it's doing. > Please don't hide code behind macros and just replace them with aml_foo() > here and at other places in this patch. > Okay, will follow your way. :) >> + aml_append(dev, field); >> + >> + /* >> + * DSM output: >> + * @ODAT: the buffer QEMU uses to store the result, the actual size >> + * filled by QEMU is the value read from NOT1. >> + * >> + * Since the page is reused by both input and out, the input data >> + * will be lost after storing new result into @ODAT. >> + */ >> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); >> + BUILD_FIELD_UNIT_SIZE(field, page_size, "ODAT"); >> + aml_append(dev, field); >> + >> + method = aml_method_serialized("NCAL", 4); >> + { >> + Aml *buffer_size = aml_local(0); >> + >> + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); >> + aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); >> + aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); >> + >> + /* >> + * transfer control to QEMU and the buffer size filled by >> + * QEMU is returned. >> + */ >> + aml_append(method, aml_store(aml_name("NOTI"), buffer_size)); >> + >> + aml_append(method, aml_store(aml_shiftleft(buffer_size, >> + aml_int(3)), buffer_size)); >> + >> + aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), >> + buffer_size , "OBUF")); >> + aml_append(method, aml_concatenate(aml_buffer(0, NULL), >> + aml_name("OBUF"), aml_local(1))); >> + aml_append(method, aml_return(aml_local(1))); >> + } >> + aml_append(dev, method); >> + >> + BUILD_STA_METHOD(dev, method); >> + >> + /* >> + * Chapter 3: _DSM Interface for NVDIMM Root Device - Example in DSM >> + * Spec Rev1. >> + */ >> + BUILD_DSM_METHOD(dev, method, >> + 0 /* 0 is reserved for NVDIMM Root Device*/, >> + "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> + /* UUID for NVDIMM Root Devices. */); >> + >> + build_nvdimm_devices(device_list, dev); >> + >> + aml_append(sb_scope, dev); >> +} >> + >> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, >> + GArray *table_data, GArray *linker) >> +{ >> + Aml *ssdt, *sb_scope; >> + >> + acpi_add_table(table_offsets, table_data); >> + >> + ssdt = init_aml_allocator(); >> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); >> + >> + sb_scope = aml_scope("\\_SB"); >> + nvdimm_build_acpi_devices(device_list, sb_scope); > is there need for dedicated nvdimm_build_acpi_devices()? > Is it reused somewhere else? > If it's not then just inline it here. Since building NVDIMM devices is a complex work so i designed to let nvdimm_build_acpi_devices() build NVDIMM root device then it calls build_nvdimm_devices() to build children devices. You can see nvdimm_build_acpi_devices is a big function. That proposal just wants to make the code clear. If you really hate this, i will drop nvdimm_build_acpi_devices, no problem. :) > >> + >> + 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); > It's not ok to have several SSDT tables with exact same signature. > how about extending build_header(..., oem_table_id)? > You can set it to NULL to get original behavior but provide NVDIMM > specific id for this table. for example "NVDIMM" > Ah, i just noticed the ACPI spec says: | each secondary system description table listed in the RSDT/XSDT with a unique OEM Table ID is | loaded. You are right. Okay, i will extend this function, thanks for your suggestion. -- 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 Tue, 3 Nov 2015 22:22:40 +0800 Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > > > On 11/03/2015 09:13 PM, Igor Mammedov wrote: > > On Mon, 2 Nov 2015 17:13:29 +0800 > > Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 > >> > >> We reserve handle 0 for root device. In this patch, we save handle, handle, > >> arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch > >> > >> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> > >> --- > >> hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 184 insertions(+) > >> > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > >> index dd84e5f..53ed675 100644 > >> --- a/hw/acpi/nvdimm.c > >> +++ b/hw/acpi/nvdimm.c > >> @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, > >> g_array_free(structures, true); > >> } > >> > >> +struct NvdimmDsmIn { > >> + uint32_t handle; > >> + uint32_t revision; > >> + uint32_t function; > >> + /* the remaining size in the page is used by arg3. */ > >> + uint8_t arg3[0]; > >> +} QEMU_PACKED; > >> +typedef struct NvdimmDsmIn NvdimmDsmIn; > >> + > >> static uint64_t > >> nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > >> { > >> @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) > >> static void > >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > >> { > >> + fprintf(stderr, "BUG: we never write DSM notification IO Port.\n"); > > it doesn't seem like this hunk belongs here > > Er, we have changed the logic: > - others: > 1) the buffer length is directly got from IO read rather than got > from dsm memory > [ This has documented in v5's changelog. ] > > So, the IO write is replaced by IO read, nvdimm_dsm_write() should not be > triggered. > > > > >> } > >> > >> static const MemoryRegionOps nvdimm_dsm_ops = { > >> @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, MemoryRegion *io, > >> memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr); > >> } > >> > >> +#define BUILD_STA_METHOD(_dev_, _method_) \ > >> + do { \ > >> + _method_ = aml_method("_STA", 0); \ > >> + aml_append(_method_, aml_return(aml_int(0x0f))); \ > >> + aml_append(_dev_, _method_); \ > >> + } while (0) > > _STA doesn't have any logic here so drop macro and just > > replace its call sites with: > > Okay, I was just wanting to save some code lines. I will drop this macro. > > > > > aml_append(foo_dev, aml_name_decl("_STA", aml_int(0xf)); > > _STA is required as a method with zero argument but this statement just > define a object. It is okay? Spec doesn't say that it must be method, it says that it will evaluate _STA object and result must be a combination of defined flags. AML wise calling a method with 0 arguments and referencing named variable is the same thing, both end up being just a namestring. Also note that _STA here return 0xF, and spec says that if _STA is missing OSPM shall assume its implicit value being 0xF, so you can just drop _STA object here altogether. > > > > > > >> + > >> +#define BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_) \ > >> + do { \ > >> + Aml *ifctx, *uuid; \ > >> + _method_ = aml_method("_DSM", 4); \ > >> + /* check UUID if it is we expect, return the errorcode if not.*/ \ > >> + uuid = aml_touuid(_uuid_); \ > >> + ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid))); \ > >> + aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */))); \ > >> + aml_append(method, ifctx); \ > >> + aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \ > >> + aml_arg(1), aml_arg(2), aml_arg(3)))); \ > >> + aml_append(_dev_, _method_); \ > >> + } while (0) > >> + > >> +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ > >> + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) > >> + > >> +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ > >> + BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_) > >> + > >> +static void build_nvdimm_devices(GSList *device_list, Aml *root_dev) > >> +{ > >> + for (; device_list; device_list = device_list->next) { > >> + NVDIMMDevice *nvdimm = device_list->data; > >> + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, > >> + NULL); > >> + uint32_t handle = nvdimm_slot_to_handle(slot); > >> + Aml *dev, *method; > >> + > >> + dev = aml_device("NV%02X", slot); > >> + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); > >> + > >> + BUILD_STA_METHOD(dev, method); > >> + > >> + /* > >> + * Chapter 4: _DSM Interface for NVDIMM Device (non-root) - Example > >> + * in DSM Spec Rev1. > >> + */ > >> + BUILD_DSM_METHOD(dev, method, > >> + handle /* NVDIMM Device Handle */, > >> + "4309AC30-0D11-11E4-9191-0800200C9A66" > >> + /* UUID for NVDIMM Devices. */); > > this will add N-bytes * #NVDIMMS in worst case. > > Please drop macro and just consolidate this method into _DSM method of parent scope > > and then call it from here like this: > > Method(_DSM, 4) > > Return(^_DSM(Arg[0-3])) > > Parent _DSM can not be directly called as _DSM in parent requires different UUID. > UUID is not saved in dsm memory so that UUID verification should be done in AML. > > This macro, BUILD_DSM_METHOD(), build its _DSM call and check if UUID is valid, if > not, it returns error code 1 (Not Supoorted), otherwise it call the common method > NCAL which saves input parameters into dsm memory and trigger IO exit. It seems no > byte is wasted. No? add an extra arg to NCAL lets say IS_ROOT NCAL will check for root UUID if IS_ROOT true and check for nvdimm device UUID if it's false. That roughly will save ~150bytes per nvdimm or more than 2Kb in case of 256 NVDIMMs. > > > > >> + > >> + aml_append(root_dev, dev); > >> + } > >> +} > >> + > >> +static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) > >> +{ > >> + Aml *dev, *method, *field; > >> + uint64_t page_size = TARGET_PAGE_SIZE; > >> + > >> + dev = aml_device("NVDR"); > >> + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); > >> + > >> + /* map DSM memory and IO into ACPI namespace. */ > >> + aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO, > >> + NVDIMM_ACPI_IO_BASE, NVDIMM_ACPI_IO_LEN)); > >> + aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY, > >> + NVDIMM_ACPI_MEM_BASE, page_size)); > >> + > >> + /* > >> + * DSM notifier: > >> + * @NOTI: Read it will notify QEMU that _DSM method is being > >> + * called and the parameters can be found in NvdimmDsmIn. > >> + * The value read from it is the buffer size of DSM output > >> + * filled by QEMU. > >> + */ > >> + field = aml_field("NPIO", AML_DWORD_ACC, AML_PRESERVE); > >> + BUILD_FIELD_UNIT_SIZE(field, sizeof(uint32_t), "NOTI"); > >> + aml_append(dev, field); > >> + > >> + /* > >> + * DSM input: > >> + * @HDLE: store device's handle, it's zero if the _DSM call happens > >> + * on NVDIMM Root Device. > >> + * @REVS: store the Arg1 of _DSM call. > >> + * @FUNC: store the Arg2 of _DSM call. > >> + * @ARG3: store the Arg3 of _DSM call. > >> + * > >> + * They are RAM mapping on host so that these accesses never cause > >> + * VM-EXIT. > >> + */ > >> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); > >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, handle, "HDLE"); > >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, revision, "REVS"); > >> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, function, "FUNC"); > >> + BUILD_FIELD_UNIT_SIZE(field, page_size - offsetof(NvdimmDsmIn, arg3), > >> + "ARG3"); > > These macros don't make code any better and one has to jump to their > > definition every time one sees it to figure out what it's doing. > > Please don't hide code behind macros and just replace them with aml_foo() > > here and at other places in this patch. > > > > Okay, will follow your way. :) > > >> + aml_append(dev, field); > >> + > >> + /* > >> + * DSM output: > >> + * @ODAT: the buffer QEMU uses to store the result, the actual size > >> + * filled by QEMU is the value read from NOT1. > >> + * > >> + * Since the page is reused by both input and out, the input data > >> + * will be lost after storing new result into @ODAT. > >> + */ > >> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); > >> + BUILD_FIELD_UNIT_SIZE(field, page_size, "ODAT"); > >> + aml_append(dev, field); > >> + > >> + method = aml_method_serialized("NCAL", 4); Why method is called with 4 arguments but only arg[0-2] are used? > >> + { > >> + Aml *buffer_size = aml_local(0); > >> + > >> + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); > >> + aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); > >> + aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); > >> + > >> + /* > >> + * transfer control to QEMU and the buffer size filled by > >> + * QEMU is returned. > >> + */ > >> + aml_append(method, aml_store(aml_name("NOTI"), buffer_size)); > >> + > >> + aml_append(method, aml_store(aml_shiftleft(buffer_size, > >> + aml_int(3)), buffer_size)); > >> + > >> + aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), > >> + buffer_size , "OBUF")); > >> + aml_append(method, aml_concatenate(aml_buffer(0, NULL), > >> + aml_name("OBUF"), aml_local(1))); > >> + aml_append(method, aml_return(aml_local(1))); > >> + } > >> + aml_append(dev, method); > >> + > >> + BUILD_STA_METHOD(dev, method); > >> + > >> + /* > >> + * Chapter 3: _DSM Interface for NVDIMM Root Device - Example in DSM > >> + * Spec Rev1. > >> + */ > >> + BUILD_DSM_METHOD(dev, method, > >> + 0 /* 0 is reserved for NVDIMM Root Device*/, Does 'handle' equal to slot number? > >> + "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" > >> + /* UUID for NVDIMM Root Devices. */); > >> + > >> + build_nvdimm_devices(device_list, dev); > >> + > >> + aml_append(sb_scope, dev); > >> +} > >> + > >> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, > >> + GArray *table_data, GArray *linker) > >> +{ > >> + Aml *ssdt, *sb_scope; > >> + > >> + acpi_add_table(table_offsets, table_data); > >> + > >> + ssdt = init_aml_allocator(); > >> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > >> + > >> + sb_scope = aml_scope("\\_SB"); > >> + nvdimm_build_acpi_devices(device_list, sb_scope); > > is there need for dedicated nvdimm_build_acpi_devices()? > > Is it reused somewhere else? > > If it's not then just inline it here. > > Since building NVDIMM devices is a complex work so i designed to > let nvdimm_build_acpi_devices() build NVDIMM root device then > it calls build_nvdimm_devices() to build children devices. You > can see nvdimm_build_acpi_devices is a big function. > > That proposal just wants to make the code clear. If you really > hate this, i will drop nvdimm_build_acpi_devices, no problem. :) seeing how much this function will add to nvdimm_build_acpi_devices, I don't see point in having a separate nvdimm_build_acpi_devices(). > > > > >> + > >> + 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); > > It's not ok to have several SSDT tables with exact same signature. > > how about extending build_header(..., oem_table_id)? > > You can set it to NULL to get original behavior but provide NVDIMM > > specific id for this table. for example "NVDIMM" > > > > Ah, i just noticed the ACPI spec says: > | each secondary system description table listed in the RSDT/XSDT with a unique OEM Table ID is > | loaded. > You are right. > > Okay, i will extend this function, thanks for your suggestion. > > -- > 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/04/2015 04:56 PM, Igor Mammedov wrote: > On Tue, 3 Nov 2015 22:22:40 +0800 > Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote: > >> >> >> On 11/03/2015 09:13 PM, Igor Mammedov wrote: >>> On Mon, 2 Nov 2015 17:13:29 +0800 >>> Xiao Guangrong <guangrong.xiao@linux.intel.com> 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 >>>> >>>> We reserve handle 0 for root device. In this patch, we save handle, handle, >>>> arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch >>>> >>>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> >>>> --- >>>> hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 184 insertions(+) >>>> >>>> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >>>> index dd84e5f..53ed675 100644 >>>> --- a/hw/acpi/nvdimm.c >>>> +++ b/hw/acpi/nvdimm.c >>>> @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, >>>> g_array_free(structures, true); >>>> } >>>> >>>> +struct NvdimmDsmIn { >>>> + uint32_t handle; >>>> + uint32_t revision; >>>> + uint32_t function; >>>> + /* the remaining size in the page is used by arg3. */ >>>> + uint8_t arg3[0]; >>>> +} QEMU_PACKED; >>>> +typedef struct NvdimmDsmIn NvdimmDsmIn; >>>> + >>>> static uint64_t >>>> nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >>>> { >>>> @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >>>> static void >>>> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >>>> { >>>> + fprintf(stderr, "BUG: we never write DSM notification IO Port.\n"); >>> it doesn't seem like this hunk belongs here >> >> Er, we have changed the logic: >> - others: >> 1) the buffer length is directly got from IO read rather than got >> from dsm memory >> [ This has documented in v5's changelog. ] >> >> So, the IO write is replaced by IO read, nvdimm_dsm_write() should not be >> triggered. >> >>> >>>> } >>>> >>>> static const MemoryRegionOps nvdimm_dsm_ops = { >>>> @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, MemoryRegion *io, >>>> memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr); >>>> } >>>> >>>> +#define BUILD_STA_METHOD(_dev_, _method_) \ >>>> + do { \ >>>> + _method_ = aml_method("_STA", 0); \ >>>> + aml_append(_method_, aml_return(aml_int(0x0f))); \ >>>> + aml_append(_dev_, _method_); \ >>>> + } while (0) >>> _STA doesn't have any logic here so drop macro and just >>> replace its call sites with: >> >> Okay, I was just wanting to save some code lines. I will drop this macro. >> >>> >>> aml_append(foo_dev, aml_name_decl("_STA", aml_int(0xf)); >> >> _STA is required as a method with zero argument but this statement just >> define a object. It is okay? > Spec doesn't say that it must be method, it says that it will evaluate _STA object > and result must be a combination of defined flags. > AML wise calling a method with 0 arguments and referencing named variable > is the same thing, both end up being just a namestring. I just tested it, it works. > > Also note that _STA here return 0xF, and spec says that if _STA is missing > OSPM shall assume its implicit value being 0xF, so you can just drop _STA > object here altogether. Actually, it will be needed for NVDIMM hotplug, but it is okay to me to drop it at present. Let's introduce it when we implement hotplug. > >> >>> >>> >>>> + >>>> +#define BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_) \ >>>> + do { \ >>>> + Aml *ifctx, *uuid; \ >>>> + _method_ = aml_method("_DSM", 4); \ >>>> + /* check UUID if it is we expect, return the errorcode if not.*/ \ >>>> + uuid = aml_touuid(_uuid_); \ >>>> + ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid))); \ >>>> + aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */))); \ >>>> + aml_append(method, ifctx); \ >>>> + aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \ >>>> + aml_arg(1), aml_arg(2), aml_arg(3)))); \ >>>> + aml_append(_dev_, _method_); \ >>>> + } while (0) >>>> + >>>> +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ >>>> + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) >>>> + >>>> +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ >>>> + BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_) >>>> + >>>> +static void build_nvdimm_devices(GSList *device_list, Aml *root_dev) >>>> +{ >>>> + for (; device_list; device_list = device_list->next) { >>>> + NVDIMMDevice *nvdimm = device_list->data; >>>> + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, >>>> + NULL); >>>> + uint32_t handle = nvdimm_slot_to_handle(slot); >>>> + Aml *dev, *method; >>>> + >>>> + dev = aml_device("NV%02X", slot); >>>> + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); >>>> + >>>> + BUILD_STA_METHOD(dev, method); >>>> + >>>> + /* >>>> + * Chapter 4: _DSM Interface for NVDIMM Device (non-root) - Example >>>> + * in DSM Spec Rev1. >>>> + */ >>>> + BUILD_DSM_METHOD(dev, method, >>>> + handle /* NVDIMM Device Handle */, >>>> + "4309AC30-0D11-11E4-9191-0800200C9A66" >>>> + /* UUID for NVDIMM Devices. */); >>> this will add N-bytes * #NVDIMMS in worst case. >>> Please drop macro and just consolidate this method into _DSM method of parent scope >>> and then call it from here like this: >>> Method(_DSM, 4) >>> Return(^_DSM(Arg[0-3])) >> >> Parent _DSM can not be directly called as _DSM in parent requires different UUID. >> UUID is not saved in dsm memory so that UUID verification should be done in AML. >> >> This macro, BUILD_DSM_METHOD(), build its _DSM call and check if UUID is valid, if >> not, it returns error code 1 (Not Supoorted), otherwise it call the common method >> NCAL which saves input parameters into dsm memory and trigger IO exit. It seems no >> byte is wasted. No? > add an extra arg to NCAL lets say IS_ROOT > NCAL will check for root UUID if IS_ROOT true and > check for nvdimm device UUID if it's false. > That roughly will save ~150bytes per nvdimm or more than 2Kb in case of 256 NVDIMMs. Okay, good to me. > >> >>> >>>> + >>>> + aml_append(root_dev, dev); >>>> + } >>>> +} >>>> + >>>> +static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) >>>> +{ >>>> + Aml *dev, *method, *field; >>>> + uint64_t page_size = TARGET_PAGE_SIZE; >>>> + >>>> + dev = aml_device("NVDR"); >>>> + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); >>>> + >>>> + /* map DSM memory and IO into ACPI namespace. */ >>>> + aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO, >>>> + NVDIMM_ACPI_IO_BASE, NVDIMM_ACPI_IO_LEN)); >>>> + aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY, >>>> + NVDIMM_ACPI_MEM_BASE, page_size)); >>>> + >>>> + /* >>>> + * DSM notifier: >>>> + * @NOTI: Read it will notify QEMU that _DSM method is being >>>> + * called and the parameters can be found in NvdimmDsmIn. >>>> + * The value read from it is the buffer size of DSM output >>>> + * filled by QEMU. >>>> + */ >>>> + field = aml_field("NPIO", AML_DWORD_ACC, AML_PRESERVE); >>>> + BUILD_FIELD_UNIT_SIZE(field, sizeof(uint32_t), "NOTI"); >>>> + aml_append(dev, field); >>>> + >>>> + /* >>>> + * DSM input: >>>> + * @HDLE: store device's handle, it's zero if the _DSM call happens >>>> + * on NVDIMM Root Device. >>>> + * @REVS: store the Arg1 of _DSM call. >>>> + * @FUNC: store the Arg2 of _DSM call. >>>> + * @ARG3: store the Arg3 of _DSM call. >>>> + * >>>> + * They are RAM mapping on host so that these accesses never cause >>>> + * VM-EXIT. >>>> + */ >>>> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); >>>> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, handle, "HDLE"); >>>> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, revision, "REVS"); >>>> + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, function, "FUNC"); >>>> + BUILD_FIELD_UNIT_SIZE(field, page_size - offsetof(NvdimmDsmIn, arg3), >>>> + "ARG3"); >>> These macros don't make code any better and one has to jump to their >>> definition every time one sees it to figure out what it's doing. >>> Please don't hide code behind macros and just replace them with aml_foo() >>> here and at other places in this patch. >>> >> >> Okay, will follow your way. :) >> >>>> + aml_append(dev, field); >>>> + >>>> + /* >>>> + * DSM output: >>>> + * @ODAT: the buffer QEMU uses to store the result, the actual size >>>> + * filled by QEMU is the value read from NOT1. >>>> + * >>>> + * Since the page is reused by both input and out, the input data >>>> + * will be lost after storing new result into @ODAT. >>>> + */ >>>> + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); >>>> + BUILD_FIELD_UNIT_SIZE(field, page_size, "ODAT"); >>>> + aml_append(dev, field); >>>> + >>>> + method = aml_method_serialized("NCAL", 4); > Why method is called with 4 arguments but only arg[0-2] are used? The arg3 is used in the later patch: [PATCH v7 28/35] nvdimm acpi: save arg3 for NVDIMM device _DSM method > >>>> + { >>>> + Aml *buffer_size = aml_local(0); >>>> + >>>> + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); >>>> + aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); >>>> + aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); >>>> + >>>> + /* >>>> + * transfer control to QEMU and the buffer size filled by >>>> + * QEMU is returned. >>>> + */ >>>> + aml_append(method, aml_store(aml_name("NOTI"), buffer_size)); >>>> + >>>> + aml_append(method, aml_store(aml_shiftleft(buffer_size, >>>> + aml_int(3)), buffer_size)); >>>> + >>>> + aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), >>>> + buffer_size , "OBUF")); >>>> + aml_append(method, aml_concatenate(aml_buffer(0, NULL), >>>> + aml_name("OBUF"), aml_local(1))); >>>> + aml_append(method, aml_return(aml_local(1))); >>>> + } >>>> + aml_append(dev, method); >>>> + >>>> + BUILD_STA_METHOD(dev, method); >>>> + >>>> + /* >>>> + * Chapter 3: _DSM Interface for NVDIMM Root Device - Example in DSM >>>> + * Spec Rev1. >>>> + */ >>>> + BUILD_DSM_METHOD(dev, method, >>>> + 0 /* 0 is reserved for NVDIMM Root Device*/, > Does 'handle' equal to slot number? handle = slot + 1, to reserve 0 for NVDIMM root device: /* * handle is used to uniquely associate nfit_memdev structure with NVDIMM * ACPI device - nfit_memdev.nfit_handle matches with the value returned * by ACPI device _ADR method. * * We generate the handle with the slot id of NVDIMM device and reserve * 0 for NVDIMM root device. */ static uint32_t nvdimm_slot_to_handle(int slot) { return slot + 1; } > > >>>> + "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >>>> + /* UUID for NVDIMM Root Devices. */); >>>> + >>>> + build_nvdimm_devices(device_list, dev); >>>> + >>>> + aml_append(sb_scope, dev); >>>> +} >>>> + >>>> +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, >>>> + GArray *table_data, GArray *linker) >>>> +{ >>>> + Aml *ssdt, *sb_scope; >>>> + >>>> + acpi_add_table(table_offsets, table_data); >>>> + >>>> + ssdt = init_aml_allocator(); >>>> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); >>>> + >>>> + sb_scope = aml_scope("\\_SB"); >>>> + nvdimm_build_acpi_devices(device_list, sb_scope); >>> is there need for dedicated nvdimm_build_acpi_devices()? >>> Is it reused somewhere else? >>> If it's not then just inline it here. >> >> Since building NVDIMM devices is a complex work so i designed to >> let nvdimm_build_acpi_devices() build NVDIMM root device then >> it calls build_nvdimm_devices() to build children devices. You >> can see nvdimm_build_acpi_devices is a big function. >> >> That proposal just wants to make the code clear. If you really >> hate this, i will drop nvdimm_build_acpi_devices, no problem. :) > seeing how much this function will add to nvdimm_build_acpi_devices, > I don't see point in having a separate nvdimm_build_acpi_devices(). > Well, let's happily drop it. -- 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 dd84e5f..53ed675 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -368,6 +368,15 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, g_array_free(structures, true); } +struct NvdimmDsmIn { + uint32_t handle; + uint32_t revision; + uint32_t function; + /* the remaining size in the page is used by arg3. */ + uint8_t arg3[0]; +} QEMU_PACKED; +typedef struct NvdimmDsmIn NvdimmDsmIn; + static uint64_t nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) { @@ -377,6 +386,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) static void nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { + fprintf(stderr, "BUG: we never write DSM notification IO Port.\n"); } static const MemoryRegionOps nvdimm_dsm_ops = { @@ -402,6 +412,179 @@ void nvdimm_init_acpi_state(MemoryRegion *memory, MemoryRegion *io, memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, &state->io_mr); } +#define BUILD_STA_METHOD(_dev_, _method_) \ + do { \ + _method_ = aml_method("_STA", 0); \ + aml_append(_method_, aml_return(aml_int(0x0f))); \ + aml_append(_dev_, _method_); \ + } while (0) + +#define BUILD_DSM_METHOD(_dev_, _method_, _handle_, _uuid_) \ + do { \ + Aml *ifctx, *uuid; \ + _method_ = aml_method("_DSM", 4); \ + /* check UUID if it is we expect, return the errorcode if not.*/ \ + uuid = aml_touuid(_uuid_); \ + ifctx = aml_if(aml_lnot(aml_equal(aml_arg(0), uuid))); \ + aml_append(ifctx, aml_return(aml_int(1 /* Not Supported */))); \ + aml_append(method, ifctx); \ + aml_append(method, aml_return(aml_call4("NCAL", aml_int(_handle_), \ + aml_arg(1), aml_arg(2), aml_arg(3)))); \ + aml_append(_dev_, _method_); \ + } while (0) + +#define BUILD_FIELD_UNIT_SIZE(_field_, _byte_, _name_) \ + aml_append(_field_, aml_named_field(_name_, (_byte_) * BITS_PER_BYTE)) + +#define BUILD_FIELD_UNIT_STRUCT(_field_, _s_, _f_, _name_) \ + BUILD_FIELD_UNIT_SIZE(_field_, sizeof(typeof_field(_s_, _f_)), _name_) + +static void build_nvdimm_devices(GSList *device_list, Aml *root_dev) +{ + for (; device_list; device_list = device_list->next) { + NVDIMMDevice *nvdimm = device_list->data; + int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP, + NULL); + uint32_t handle = nvdimm_slot_to_handle(slot); + Aml *dev, *method; + + dev = aml_device("NV%02X", slot); + aml_append(dev, aml_name_decl("_ADR", aml_int(handle))); + + BUILD_STA_METHOD(dev, method); + + /* + * Chapter 4: _DSM Interface for NVDIMM Device (non-root) - Example + * in DSM Spec Rev1. + */ + BUILD_DSM_METHOD(dev, method, + handle /* NVDIMM Device Handle */, + "4309AC30-0D11-11E4-9191-0800200C9A66" + /* UUID for NVDIMM Devices. */); + + aml_append(root_dev, dev); + } +} + +static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) +{ + Aml *dev, *method, *field; + uint64_t page_size = TARGET_PAGE_SIZE; + + dev = aml_device("NVDR"); + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012"))); + + /* map DSM memory and IO into ACPI namespace. */ + aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO, + NVDIMM_ACPI_IO_BASE, NVDIMM_ACPI_IO_LEN)); + aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY, + NVDIMM_ACPI_MEM_BASE, page_size)); + + /* + * DSM notifier: + * @NOTI: Read it will notify QEMU that _DSM method is being + * called and the parameters can be found in NvdimmDsmIn. + * The value read from it is the buffer size of DSM output + * filled by QEMU. + */ + field = aml_field("NPIO", AML_DWORD_ACC, AML_PRESERVE); + BUILD_FIELD_UNIT_SIZE(field, sizeof(uint32_t), "NOTI"); + aml_append(dev, field); + + /* + * DSM input: + * @HDLE: store device's handle, it's zero if the _DSM call happens + * on NVDIMM Root Device. + * @REVS: store the Arg1 of _DSM call. + * @FUNC: store the Arg2 of _DSM call. + * @ARG3: store the Arg3 of _DSM call. + * + * They are RAM mapping on host so that these accesses never cause + * VM-EXIT. + */ + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, handle, "HDLE"); + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, revision, "REVS"); + BUILD_FIELD_UNIT_STRUCT(field, NvdimmDsmIn, function, "FUNC"); + BUILD_FIELD_UNIT_SIZE(field, page_size - offsetof(NvdimmDsmIn, arg3), + "ARG3"); + aml_append(dev, field); + + /* + * DSM output: + * @ODAT: the buffer QEMU uses to store the result, the actual size + * filled by QEMU is the value read from NOT1. + * + * Since the page is reused by both input and out, the input data + * will be lost after storing new result into @ODAT. + */ + field = aml_field("NRAM", AML_DWORD_ACC, AML_PRESERVE); + BUILD_FIELD_UNIT_SIZE(field, page_size, "ODAT"); + aml_append(dev, field); + + method = aml_method_serialized("NCAL", 4); + { + Aml *buffer_size = aml_local(0); + + aml_append(method, aml_store(aml_arg(0), aml_name("HDLE"))); + aml_append(method, aml_store(aml_arg(1), aml_name("REVS"))); + aml_append(method, aml_store(aml_arg(2), aml_name("FUNC"))); + + /* + * transfer control to QEMU and the buffer size filled by + * QEMU is returned. + */ + aml_append(method, aml_store(aml_name("NOTI"), buffer_size)); + + aml_append(method, aml_store(aml_shiftleft(buffer_size, + aml_int(3)), buffer_size)); + + aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0), + buffer_size , "OBUF")); + aml_append(method, aml_concatenate(aml_buffer(0, NULL), + aml_name("OBUF"), aml_local(1))); + aml_append(method, aml_return(aml_local(1))); + } + aml_append(dev, method); + + BUILD_STA_METHOD(dev, method); + + /* + * Chapter 3: _DSM Interface for NVDIMM Root Device - Example in DSM + * Spec Rev1. + */ + BUILD_DSM_METHOD(dev, method, + 0 /* 0 is reserved for NVDIMM Root Device*/, + "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" + /* UUID for NVDIMM Root Devices. */); + + build_nvdimm_devices(device_list, dev); + + aml_append(sb_scope, dev); +} + +static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, + GArray *table_data, GArray *linker) +{ + Aml *ssdt, *sb_scope; + + acpi_add_table(table_offsets, table_data); + + ssdt = init_aml_allocator(); + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); + + sb_scope = aml_scope("\\_SB"); + nvdimm_build_acpi_devices(device_list, sb_scope); + + 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); + free_aml_allocator(); +} + void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, GArray *linker) { @@ -414,5 +597,6 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, } 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 We reserve handle 0 for root device. In this patch, we save handle, handle, arg1 and arg2 to dsm memory. Arg3 is conditionally saved in later patch Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- hw/acpi/nvdimm.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+)