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

[Xiao Guangrong]

On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> On Thu,  3 Nov 2016 11:51:29 +0800
> Xiao Guangrong <address@hidden> wrote:
>
> > _FIT is required for hotplug support, guest will inquire the updated
> > device info from it if a hotplug event is received
> s/_FIT/_FIT method/
>
> the same applies to subj. line

Okay.

> > As FIT buffer is not completely mapped into guest address space, so a
> > new function, Read FIT whose UUID is UUID
> > 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, handle 0x10000, function index
> > is 0x1, is reserved by QEMU to read the piece of FIT buffer. The buffer
> > is concatenated before _FIT return
>
> Commit message hard to read/parse, it might be better if I'd use simple
> short sentences.

Okay, will change it to:

As FIT buffer is not completely mapped into guest address space, Read_FIT
method is introduced to read NFIT structures blob from QEMU, The buffer
is concatenated before _FIT return

> > Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >  docs/specs/acpi_nvdimm.txt |  63 ++++++++++++-
> >  hw/acpi/nvdimm.c           | 225 ++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 271 insertions(+), 17 deletions(-)
> >
> > diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> > index 0fdd251..364e832 100644
> > --- a/docs/specs/acpi_nvdimm.txt
> > +++ b/docs/specs/acpi_nvdimm.txt
> > @@ -65,8 +65,8 @@ _FIT(Firmware Interface Table)
> >     The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
> >     NVDIMM Firmware Interface Table (NFIT).
> >
> > -QEMU NVDIMM Implemention
> > -========================
> > +QEMU NVDIMM Implementation
> > +==========================
> >  QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
> >  for NVDIMM ACPI.
> >
> > @@ -82,6 +82,16 @@ Memory:
> >     ACPI writes _DSM Input Data (based on the offset in the page):
> >     [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
> >                  Root device.
> > +
> > +                The handle is completely QEMU internal thing, the values in
> > +                range [0, 0xFFFF] indicate nvdimm device (O means nvdimm
> [1, 0xFFFF] indicate nvdimm device
> and move 0 to reserved section

Okay.

> > +                root device named NVDR), other values are reserved by other
> s/by/for/

okay.

> > +                purpose.
> s/purpose/purposes/

okay.

> > +                Current reserved handle:
> s/Current reserved handle/Reserved handles/
>
> > +                0x10000 is reserved for QEMU internal DSM function called on
> > +                the root device.
> description is too obscure, I wonder if it could be more specific

I would like to make these reserved values more generic in order to
support more QEMU reserved _DSM methods in the further. So, i planed:
UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is dedicated for QEMU reserved
methods. Handle 0x10000 indicates the method is for the root device.
0x10001 ~ 0x1FFFF indicates the method is for the nvdimm devices.

As currently we do not have reserved method on nvdimm device, so i
only document 0x1000 for the root device.

> >     [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
> >     [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
> >     [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
> > @@ -127,6 +137,49 @@ _DSM process diagram:
> >   | result from the page     |      |              |
> >   +--------------------------+      +--------------+
> >
> > - _FIT implementation
> > - -------------------
> > - TODO (will fill it when nvdimm hotplug is introduced)
> > +QEMU internal use only _DSM function
> > +------------------------------------
> > +There is the function introduced by QEMU and only used by QEMU internal.
> drop it
>
> > +1) Read FIT
> > +   UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM
> > +   function (private QEMU function)
> not necessary, drop it. Maybe extend UUID description in
> "Input parameters:" section

okay.

> > +   _FIT method uses Read_FIT function to fetch NFIT structures blob from
> s/Read_FIT function/_DSM method/

okay.

> > +   QEMU in 1 page sized increments which are then concatenated and returned
> > +   as _FIT method result.
> > +
> > +   Input parameters:
> > +   Arg0 – UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62}
> > +   Arg1 – Revision ID (set to 1)
> > +   Arg2 - Function Index, 0x1
> > +   Arg3 - A package containing a buffer whose layout is as follows:
> > +
> > +   +----------+--------+--------+-------------------------------------------+
> > +   |  Field   | Length | Offset |                 Description               |
> > +   +----------+--------+--------+-------------------------------------------+
> > +   | offset   |   4    |   0    | offset in QEMU's NFIT structures blob to  |
> > +   |          |        |        | read from                                 |
> > +   +----------+--------+--------+-------------------------------------------+
> > +
> > +   Output:
> > +   +----------+--------+--------+-------------------------------------------+
> > +   |  Field   | Length | Offset |                 Description               |
> > +   +----------+--------+--------+-------------------------------------------+
> > +   |          |        |        | 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   |
> > +   +----------+--------+--------+-------------------------------------------+
> > +   | length   |   4    |   4    | The fit size                              |
> > +   +----------+-----------------+-------------------------------------------+
> > +   | fit data | Varies |   8    | FIT data, its size is indicated by length |
> > +   |          |        |        | filed above                               |
> > +   +----------+--------+--------+-------------------------------------------+
> > +
> > +   The FIT offset is maintained by the OSPM itself, current offset plus
> > +   the length returned by the function is the next offset we should read.
> there shouldn't be 'we' or anything personal in spec

Okay, change it to OSPM. :)

> > +   When all the FIT data has been read out, zero length is returned.
> > +
> > +   If it returns 0x100, OSPM should restart to read FIT (read from offset 0
> > +   again).
>
> PS:
>  fix typos and fix spelling/grammatical errors you are adding in above text.

Sorry for the poor English, will check it carefully.

> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 9fee077..593ac0d 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -484,6 +484,23 @@ typedef struct NvdimmFuncSetLabelDataIn 
> > NvdimmFuncSetLabelDataIn;
> >  QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) +
> >                    offsetof(NvdimmDsmIn, arg3) > 4096);
> >
> > +struct NvdimmFuncReadFITIn {
> > +    uint32_t offset; /* the offset into FIT buffer. */
> > +} QEMU_PACKED;
> > +typedef struct NvdimmFuncReadFITIn NvdimmFuncReadFITIn;
> > +QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITIn) +
> > +                  offsetof(NvdimmDsmIn, arg3) > 4096);
> > +
> > +struct NvdimmFuncReadFITOut {
> > +    /* the size of buffer filled by QEMU. */
> > +    uint32_t len;
> > +    uint32_t func_ret_status; /* return status code. */
> > +    uint32_t length; /* the length of fit. */
> redundant as len field above already has it all.
>
> 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               |
   +----------+--------+--------+-------------------------------------------+
   |          |        |        | 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? :)