From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztcal-000306-7i for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:29:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ztcah-0004XI-Nk for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:29:43 -0500 Received: from mga01.intel.com ([192.55.52.88]:10918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ztcah-0004XE-BC for qemu-devel@nongnu.org; Tue, 03 Nov 2015 09:29:39 -0500 References: <1446455617-129562-1-git-send-email-guangrong.xiao@linux.intel.com> <1446455617-129562-28-git-send-email-guangrong.xiao@linux.intel.com> <20151103141355.3897786c@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <5638C330.7020606@linux.intel.com> Date: Tue, 3 Nov 2015 22:22:40 +0800 MIME-Version: 1.0 In-Reply-To: <20151103141355.3897786c@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 27/35] nvdimm acpi: build ACPI nvdimm devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: vsementsov@virtuozzo.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 11/03/2015 09:13 PM, Igor Mammedov wrote: > On Mon, 2 Nov 2015 17:13:29 +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 >> >> 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 >> --- >> 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.