On 11/03/2015 09:13 PM, Igor Mammedov wrote:
On Mon, 2 Nov 2015 17:13:29 +0800
Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
