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

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

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...

>
> 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.