From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2Lr0-0005Dw-I1 for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:31:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2Lqw-0007Q4-KQ for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:31:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55692) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2Lqw-0007PP-DE for qemu-devel@nongnu.org; Thu, 03 Nov 2016 13:31:02 -0400 Date: Thu, 3 Nov 2016 18:29:52 +0100 From: Igor Mammedov Message-ID: <20161103182952.6206d2fd@Igors-MacBook-Pro.local> In-Reply-To: <8cf437f5-a2fe-1250-9ef3-19493cfceced@linux.intel.com> 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> 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 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. > > > > > if you want to describe AML there you can do it after interface > > description saying that common NCAL method extracts status and fit > > data form dsm page and returns that as buffer object, but it's AML > > impl. specific. I could write an alternative AML code that would > > deal with dms page in its own way as long as I would know what > > write/read at what offset. > > Yes, i agree with you. >