qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Xiao Guangrong
Subject: Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT
Date: Fri, 4 Nov 2016 01:39:31 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0



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)                    |
   +----------+-----------------+-------------------------------------------+
   |          |        |        | 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. :)





reply via email to

[Prev in Thread] Current Thread [Next in Thread]