On Fri, 4 Nov 2016 01:39:31 +0800
Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
>
>
> On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:53:06 +0800
> > Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> >>> On Fri, 4 Nov 2016 00:17:00 +0800
> >>> Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>>>> Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>>>> Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> 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. :)
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
