From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42740) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2JKt-0003Sn-FL for qemu-devel@nongnu.org; Thu, 03 Nov 2016 10:49:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2JKq-0002SP-Cj for qemu-devel@nongnu.org; Thu, 03 Nov 2016 10:49:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33962) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2JKq-0002RC-5H for qemu-devel@nongnu.org; Thu, 03 Nov 2016 10:49:44 -0400 Date: Thu, 3 Nov 2016 15:49:34 +0100 From: Igor Mammedov Message-ID: <20161103154934.2a9321f7@Igors-MacBook-Pro.local> In-Reply-To: References: <1478145090-11987-1-git-send-email-guangrong.xiao@linux.intel.com> <1478145090-11987-3-git-send-email-guangrong.xiao@linux.intel.com> <20161103125808.11857a35@nial.brq.redhat.com> <2f770790-f3e9-ce56-e11b-e9f7e73e7078@linux.intel.com> <20161103140015.5f07b009@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org On Thu, 3 Nov 2016 21:02:22 +0800 Xiao Guangrong wrote: > > > On 11/03/2016 09:00 PM, Igor Mammedov wrote: > > > > > >>> just drop this and describe properly 'len' in spec section > >>> i.e. len: length of entire returned data (including the header) > >> > >> Okay, i will change the spec like this: > >> > >> QEMU Writes Output Data (based on the offset in the page): > >> [0x0 - 0x3]: 4 bytes, length of entire returned data > >> (including the header) > >> > >> And drop the length field in Read_Fit return buffer, doc > >> the fit buffer like this: > >> > >> +----------+--------+--------+-------------------------------------------+ > >> | Field | Length | Offset | > >> Description | > >> +----------+--------+--------+-------------------------------------------+ > > you need to add length here, otherwise this table is not correct > > Ah, so i am confused. > > struct NvdimmFuncReadFITOut definition is based on the layout of > Read_FI output. You suggested to drop the length filed in > NvdimmFuncReadFITOut but keep it in the layout, it is not consistent. > > I missed something? +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; -------------------------------- | field | len | off | desc... -------------------------------- | length | 4 | 0 | .... -------------------------------- | status | 4 | 4 | .... -------------------------------- | fit data | ................ i.e. you were forgetting to add length in spec so offsets were wrong even for described fields. > > > > > > >> | | | | return status > >> codes | | | | | 0x100 > >> - error caused by NFIT update while | | status | 4 | 0 > >> | read by _FIT wasn't completed, other | | | > >> | | codes follow Chapter 3 in DSM Spec Rev1 | > >> +----------+--------+--------+-------------------------------------------+ > >> | fit data | Varies | 8 | FIT data, The remaining size in > >> the | | | | | returned buffer is used > >> by FIT | > >> +----------+--------+--------+-------------------------------------------+ > >> > >> > >> > >>>> +} > >>>> + > >>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, > >>>> NvdimmDsmIn *in, > >>>> + hwaddr dsm_mem_addr) > >>> function name doesn't make any sense to me > >> > >> As i explained above, handle 0x10000 indicates the reserved _DSM > >> method is called on the root device... > >> > >> It makes sense now? :) > > function name should reflect what it does, > > i.e use verb and I see only nouns here. > > Got it, will change it to: nvdimm_handle_reserved_root_method(). >