From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT Date: Wed, 2 Nov 2016 23:54:05 +0800 Message-ID: <9eac7475-ba9a-e854-accc-593baa81628f@linux.intel.com> References: <1477672540-27952-1-git-send-email-guangrong.xiao@linux.intel.com> <1477672540-27952-4-git-send-email-guangrong.xiao@linux.intel.com> <20161102145651.215433ff@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, dan.j.williams@intel.com, rth@twiddle.net To: Igor Mammedov Return-path: Received: from mga02.intel.com ([134.134.136.20]:15188 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753055AbcKBQDJ (ORCPT ); Wed, 2 Nov 2016 12:03:09 -0400 In-Reply-To: <20161102145651.215433ff@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 11/02/2016 09:56 PM, Igor Mammedov wrote: > On Sat, 29 Oct 2016 00:35:39 +0800 > Xiao Guangrong wrote: > >> _FIT is required for hotplug support, guest will inquire the updated >> device info from it if a hotplug event is received >> >> As FIT buffer is not completely mapped into guest address space, so a >> new function, Read FIT whose UUID is UUID >> 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index >> is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer >> is concatenated before _FIT return >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design >> >> Signed-off-by: Xiao Guangrong >> --- > [...] > >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 5f728a6..fc1a012 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -496,6 +496,22 @@ typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn; >> QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + >> offsetof(NvdimmDsmIn, arg3) > 4096); >> >> +struct NvdimmFuncReadFITIn { >> + uint32_t offset; /* the offset of FIT buffer. */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn; >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) + >> + offsetof(NvdimmDsmIn, arg3) > 4096); >> + >> +struct NvdimmFuncReadFITOut { >> + /* the size of buffer filled by QEMU. */ >> + uint32_t len; >> + uint32_t func_ret_status; /* return status code. */ >> + uint8_t fit[0]; /* the FIT data. */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut; >> +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > 4096); >> + >> static void >> nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr) >> { >> @@ -516,6 +532,74 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr) >> cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out)); >> } >> >> +#define NVDIMM_QEMU_RSVD_HANDLE_ROOT 0x10000 >> + >> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ >> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> + hwaddr dsm_mem_addr) >> +{ >> + NvdimmFitBuffer *fit_buf = &state->fit_buf; >> + NvdimmFuncReadFITIn *read_fit; >> + NvdimmFuncReadFITOut *read_fit_out; >> + GArray *fit; >> + uint32_t read_len = 0, func_ret_status; >> + int size; >> + >> + read_fit = (NvdimmFuncReadFITIn *)in->arg3; >> + le32_to_cpus(&read_fit->offset); > I'd prefer if you'd not do inplace conversion, just do > offset = le32_to_cpu(read_fit->offset); okay. > >> + >> + qemu_mutex_lock(&fit_buf->lock); >> + fit = fit_buf->fit; >> + >> + nvdimm_debug("Read FIT: offset %#x FIT size %#x Dirty %s.\n", >> + read_fit->offset, fit->len, fit_buf->dirty ? "Yes" : "No"); > as follow up path replace nvdimm_debug() with trace events I will do it as a separate patch. > >> + >> + if (read_fit->offset > fit->len) { >> + func_ret_status = 3 /* Invalid Input Parameters */; > should be macros instead of magic value Yes. > >> + goto exit; >> + } >> + >> + /* It is the first time to read FIT. */ >> + if (!read_fit->offset) { >> + fit_buf->dirty = false; >> + } else if (fit_buf->dirty) { /* FIT has been changed during RFIT. */ >> + func_ret_status = 0x100 /* fit changed */; > should be macros instead of magic value okay. > >> + goto exit; >> + } >> + >> + func_ret_status = 0 /* Success */; >> + read_len = MIN(fit->len - read_fit->offset, >> + 4096 - sizeof(NvdimmFuncReadFITOut)); >> + >> +exit: >> + size = sizeof(NvdimmFuncReadFITOut) + read_len; >> + read_fit_out = g_malloc(size); >> + >> + read_fit_out->len = cpu_to_le32(size); >> + read_fit_out->func_ret_status = cpu_to_le32(func_ret_status); >> + memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len); >> + >> + cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size); >> + >> + g_free(read_fit_out); >> + qemu_mutex_unlock(&fit_buf->lock); >> +} >> + >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> + hwaddr dsm_mem_addr) >> +{ >> + switch (in->function) { >> + case 0x0: >> + nvdimm_dsm_function0(0x1 | 1 << 1 /* Read FIT */, dsm_mem_addr); >> + return; >> + case 0x1 /*Read FIT */: >> + nvdimm_dsm_func_read_fit(state, in, dsm_mem_addr); >> + return; >> + } >> + >> + nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr); > should be macros instead of magic value Okay. > >> +} >> + >> static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr) >> { >> /* >> @@ -742,6 +826,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >> static void >> nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> { >> + AcpiNVDIMMState *state = opaque; >> NvdimmDsmIn *in; >> hwaddr dsm_mem_addr = val; >> >> @@ -769,6 +854,11 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> goto exit; >> } >> >> + if (in->handle == NVDIMM_QEMU_RSVD_HANDLE_ROOT) { >> + nvdimm_dsm_reserved_root(state, in, dsm_mem_addr); >> + goto exit; >> + } >> + >> /* Handle 0 is reserved for NVDIMM Root Device. */ >> if (!in->handle) { >> nvdimm_dsm_root(in, dsm_mem_addr); >> @@ -821,9 +911,13 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, >> #define NVDIMM_DSM_OUT_BUF_SIZE "RLEN" >> #define NVDIMM_DSM_OUT_BUF "ODAT" >> >> +#define NVDIMM_DSM_RFIT_STATUS "RSTA" >> + >> +#define NVDIMM_QEMU_RSVD_UUID "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62" >> + >> static void nvdimm_build_common_dsm(Aml *dev) >> { >> - Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem; >> + Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2; >> Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid; >> Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size; >> uint8_t byte_list[1]; >> @@ -912,9 +1006,15 @@ static void nvdimm_build_common_dsm(Aml *dev) >> /* UUID for NVDIMM Root Device */, expected_uuid)); >> aml_append(method, ifctx); >> elsectx = aml_else(); >> - aml_append(elsectx, aml_store( >> + ifctx = aml_if(aml_equal(handle, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT))); >> + aml_append(ifctx, aml_store(aml_touuid(NVDIMM_QEMU_RSVD_UUID >> + /* UUID for QEMU internal use */), expected_uuid)); >> + aml_append(elsectx, ifctx); >> + elsectx2 = aml_else(); >> + aml_append(elsectx2, aml_store( >> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66") >> /* UUID for NVDIMM Devices */, expected_uuid)); >> + aml_append(elsectx, elsectx2); >> aml_append(method, elsectx); >> >> uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid)); >> @@ -994,6 +1094,105 @@ static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle) >> aml_append(dev, method); >> } >> >> +static void nvdimm_build_fit(Aml *dev) > nvdimm_build_fit_method() okay. > >> +{ >> + Aml *method, *pkg, *buf, *buf_size, *offset, *call_result; >> + Aml *whilectx, *ifcond, *ifctx, *elsectx, *fit; >> + >> + buf = aml_local(0); >> + buf_size = aml_local(1); >> + fit = aml_local(2); >> + >> + aml_append(dev, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), NVDIMM_DSM_RFIT_STATUS)); > it doesn't have to be buffer as it's internal ASL integer object > so it could be just named variable. Let me try. > > I'd also move it to _FIT method instead of making it device global We can not as it is used both in _FIT method and RFIT method. > > and if it could work try to pass it as argument to RFIT > RefOf/DerefOf may help here > or make return value instead of buffer and pass buffer as reference. > Let me try. > Alternatively you can return buffer from RFIT with status field included > and check/discard status value there. > As we can not create name object in a while-loop, it is not easy to check the status in _FIT. >> + >> + /* build helper function, RFIT. */ >> + method = aml_method("RFIT", 1, AML_SERIALIZED); >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), "OFST")); >> + >> + /* prepare input package. */ >> + pkg = aml_package(1); >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); >> + aml_append(pkg, aml_name("OFST")); >> + >> + /* call Read_FIT function. */ >> + call_result = aml_call5(NVDIMM_COMMON_DSM, >> + aml_touuid(NVDIMM_QEMU_RSVD_UUID), >> + aml_int(1) /* Revision 1 */, >> + aml_int(0x1) /* Read FIT */, >> + pkg, aml_int(NVDIMM_QEMU_RSVD_HANDLE_ROOT)); >> + aml_append(method, aml_store(call_result, buf)); >> + >> + /* handle _DSM result. */ >> + aml_append(method, aml_create_dword_field(buf, >> + aml_int(0) /* offset at byte 0 */, "STAU")); >> + >> + aml_append(method, aml_store(aml_name("STAU"), >> + aml_name(NVDIMM_DSM_RFIT_STATUS))); >> + >> + /* if something is wrong during _DSM. */ >> + ifcond = aml_equal(aml_int(0 /* Success */), aml_name("STAU")); >> + ifctx = aml_if(aml_lnot(ifcond)); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_store(aml_sizeof(buf), buf_size)); >> + aml_append(method, aml_subtract(buf_size, >> + aml_int(4) /* the size of "STAU" */, >> + buf_size)); >> + >> + /* if we read the end of fit. */ >> + ifctx = aml_if(aml_equal(buf_size, aml_int(0))); >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_store(aml_shiftleft(buf_size, aml_int(3)), >> + buf_size)); > there isn't need to convert bytes to bits and store it in the same variable > it creates confusion Okay, i will introduce a new variable named buf_size_bits. > >> + aml_append(method, aml_create_field(buf, >> + aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/ >> + buf_size, "BUFF")); > just inline conversion in aml_create_field() okay.