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

[Igor Mammedov]

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.

> 
> >
> >
> >>     |          |        |        | return status
> >> codes                       | |          |        |        | 0x100
> >> - error caused by NFIT update while | | status   |   4    |   0
> >> | read by _FIT wasn't completed, other      | |          |
> >> |        | codes follow Chapter 3 in DSM Spec Rev1   |
> >> +----------+--------+--------+-------------------------------------------+
> >> | fit data | Varies |   8    | FIT data, The remaining size in
> >> the       | |          |        |        | returned buffer is used
> >> by FIT            |
> >> +----------+--------+--------+-------------------------------------------+
> >>
> >>
> >>
> >>>> +}
> >>>> +
> >>>> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state,
> >>>> NvdimmDsmIn *in,
> >>>> +                                     hwaddr dsm_mem_addr)
> >>> function name doesn't make any sense to me
> >>
> >> As i explained above, handle 0x10000 indicates the reserved _DSM
> >> method is called on the root device...
> >>
> >> It makes sense now? :)
> > function name should reflect what it does,
> > i.e use verb and I see only nouns here.
> 
> Got it, will change it to: nvdimm_handle_reserved_root_method().
>