Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT

[Igor Mammedov]

On Fri, 4 Nov 2016 01:39:31 +0800
Xiao Guangrong <address@hidden> wrote:

> 
> 
> On 11/04/2016 01:29 AM, Igor Mammedov wrote:
> > On Fri, 4 Nov 2016 00:53:06 +0800
> > Xiao Guangrong <address@hidden> wrote:
> >
> >>
> >>
> >> On 11/04/2016 12:49 AM, Igor Mammedov wrote:
> >>> On Fri, 4 Nov 2016 00:17:00 +0800
> >>> Xiao Guangrong <address@hidden> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 11/04/2016 12:13 AM, Igor Mammedov wrote:
> >>>>> On Thu, 3 Nov 2016 22:53:43 +0800
> >>>>> Xiao Guangrong <address@hidden> wrote:
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 11/03/2016 10:49 PM, Igor Mammedov wrote:
> >>>>>>> On Thu, 3 Nov 2016 21:02:22 +0800
> >>>>>>> Xiao Guangrong <address@hidden> 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. :)
> 
>