|
| From: | Xiao Guangrong |
| Subject: | Re: [Qemu-devel] [PATCH v7 27/35] nvdimm acpi: build ACPI nvdimm devices |
| Date: | Wed, 4 Nov 2015 22:11:50 +0800 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/04/2015 04:56 PM, Igor Mammedov wrote:
On Tue, 3 Nov 2015 22:22:40 +0800 Xiao Guangrong <address@hidden> wrote:On 11/03/2015 09:13 PM, Igor Mammedov wrote:On Mon, 2 Nov 2015 17:13:29 +0800 Xiao Guangrong <address@hidden> 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 <address@hidden> --- 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 hereEr, 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.
| [Prev in Thread] | Current Thread | [Next in Thread] |