From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35943) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2M6K-0000Ye-6v for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:46:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2M6H-0000LA-33 for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:46:56 -0400 Received: from mga09.intel.com ([134.134.136.24]:37668) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2M6G-0000Ke-Nr for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:46:53 -0400 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> From: Xiao Guangrong Message-ID: Date: Fri, 4 Nov 2016 01:39:31 +0800 MIME-Version: 1.0 In-Reply-To: <20161103182952.6206d2fd@Igors-MacBook-Pro.local> Content-Type: text/plain; charset=utf-8; format=flowed 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: Igor Mammedov 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 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) | +----------+-----------------+-------------------------------------------+ | | | | 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 | +----------+--------+--------+-------------------------------------------+ | fit data | Varies | 8 | FIT data, the remaining size is used by | | | | | fit data if status is success; | | | | | otherwise it is not valid | +----------+--------+--------+-------------------------------------------+ As you know, i am not good at doc, any suggestion is welcome. :)