From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Mammedov Subject: Re: [PATCH v4 2/3] nvdimm acpi: introduce _FIT Date: Thu, 3 Nov 2016 15:49:34 +0100 Message-ID: <20161103154934.2a9321f7@Igors-MacBook-Pro.local> 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 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 To: Xiao Guangrong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60348 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751944AbcKCOtn (ORCPT ); Thu, 3 Nov 2016 10:49:43 -0400 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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(). >