From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34060) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvfhm-0002B6-89 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:13:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zvfhh-00069x-75 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:13:26 -0500 Received: from mga14.intel.com ([192.55.52.115]:61488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zvfhg-00069t-SJ for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:13:21 -0500 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-26-git-send-email-guangrong.xiao@linux.intel.com> <20151108193447-mutt-send-email-mst@redhat.com> From: Xiao Guangrong Message-ID: <56403805.4070208@linux.intel.com> Date: Mon, 9 Nov 2015 14:07:01 +0800 MIME-Version: 1.0 In-Reply-To: <20151108193447-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 25/33] nvdimm acpi: build ACPI nvdimm devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 11/09/2015 01:38 AM, Michael S. Tsirkin wrote: > On Fri, Oct 30, 2015 at 01:56:19PM +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"); >> } >> >> 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.*/ \ > > check that UUID is what we expect? Yes, it is, better English indeed. > >> + 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)))); \ > > So name NCAL here matches the name below. > Pls define a macro for it so we aren't limited > by silly 4-letter limitations of AML. > Same applies to all other names you use here and elsewhere. Okay, it's good to me. > >> + 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_) >> + > > why are these macros? Make them functions pls. Since Igor thought it hidden the details and it was not good to the readers. I will just drop this macros and inline it in the place where it is used.