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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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