From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37885) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2MDz-0004rO-BV for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:54:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2MDw-0004Vi-9Q for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:54:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40472) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2MDw-0004VA-2I for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:54:48 -0400 Date: Thu, 3 Nov 2016 18:54:37 +0100 From: Igor Mammedov Message-ID: <20161103185437.2d423716@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> <20161103154934.2a9321f7@Igors-MacBook-Pro.local> <20161103171323.7825e926@Igors-MacBook-Pro.local> <20161103174908.798679be@Igors-MacBook-Pro.local> <8cf437f5-a2fe-1250-9ef3-19493cfceced@linux.intel.com> <20161103182952.6206d2fd@Igors-MacBook-Pro.local> 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 Fri, 4 Nov 2016 01:39:31 +0800 Xiao Guangrong wrote: > > > On 11/04/2016 01:29 AM, Igor Mammedov wrote: > > On Fri, 4 Nov 2016 00:53:06 +0800 > > Xiao Guangrong wrote: > > > >> > >> > >> On 11/04/2016 12:49 AM, Igor Mammedov wrote: > >>> On Fri, 4 Nov 2016 00:17:00 +0800 > >>> Xiao Guangrong wrote: > >>> > >>>> > >>>> > >>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote: > >>>>> On Thu, 3 Nov 2016 22:53:43 +0800 > >>>>> Xiao Guangrong wrote: > >>>>> > >>>>>> > >>>>>> > >>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote: > >>>>>>> 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. > >>>>>> > >>>>>> > >>>>>> We can not do this. > >>>>>> > >>>>>> @len is used by QEMU emulation to count the size of the buffer > >>>>>> that _DSM should return. It's only used in NVDIMM_COMMON_DSM > >>>>>> method which is shared by the DSM method from VM and Read_Fit. > >>>>> spec describes buffer layout independently from AML that uses it, > >>>>> so it should describe whole data structure. > >>>>> > >>>>> Then it's upto guest how to read this data, it could be QEMU > >>>>> generated AML (as it's here) or some other driver or even BIOS. > >>>> > >>>> However, what we are talking about is Read_FIT method, so this is > >>>> the layout of Read_FIT output rather than all memory in the dsm > >>>> page. > >>>> > >>>> Actually, in the spec we already have documented the common len > >>>> field: > >>>> > >>>> QEMU Writes Output Data (based on the offset in the page): > >>>> [0x0 - 0x3]: 4 bytes, the length of result > >>>> [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU > >>>> > >>>> Also, i really do not hope to use this field to count the buffer > >>>> size returned by Read_FIT, we'd try the best to reuse existing DSM > >>>> method (NVDIMM_COMMON_DSM), i.e, treat Read_FIT as normal DSM > >>>> method. > >>> buffer layout describes interface between QEMU and firmware (AML) > >>> and it should describe it completely every time to avoid confusion. > >>> > >>> See for example ACPI spec, NFIT table, SRAT table, ... > >>> all table descriptions start with complete header. > >> > >> Okay. Understood. :) > >> > >>> > >>> If you skip length it rises question how much fit data are there, > >>> meaning interface description isn't complete. > >> > >> So how about introduce a length field in the output buffer just > >> as this patch did? I understand we are able to count the size > >> from dsm len, however, it can simplify the code a lot... > > it's redundant as there already is length for whole buffer, > > interface should be kept as simple as possible and not include > > redundant data for convenience. > > Okay. > > So the doc should be changed to: > > Output layout in the dsm memory page: > > +----------+--------+--------+-------------------------------------------+ > | Field | Length | Offset | Description | > +----------+--------+--------+-------------------------------------------+ > | length | 4 | 4 | length of entire returned data | > | | | | (including the header) | wrong offset > +----------+-----------------+-------------------------------------------+ > | | | | return status codes | > | | | | 0x100 - error caused by NFIT update while | > | status | 4 | 4 | read by _FIT wasn't completed, other | > | | | | codes follow Chapter 3 in DSM Spec Rev1 | it wouldn't be bad to specify success status code here too. > +----------+--------+--------+-------------------------------------------+ > | fit data | Varies | 8 | FIT data, the remaining size is used by | > | | | | fit data if status is success; | > | | | | otherwise it is not valid | > +----------+--------+--------+-------------------------------------------+ contains FIT data, this field is present if status field is [concrete number here] > > As you know, i am not good at doc, any suggestion is welcome. :) > >