Re: [Qemu-devel] [PATCH v3 3/4] nvdimm acpi: introduce _FIT

[Xiao Guangrong]

On 11/02/2016 12:24 AM, Igor Mammedov wrote:
> On Sat, 29 Oct 2016 00:35:39 +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
> >
> > 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
> >
> > Refer to docs/specs/acpi-nvdimm.txt for detailed design
> >
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >  docs/specs/acpi_nvdimm.txt |  58 ++++++++++++-
> >  hw/acpi/nvdimm.c           | 204 ++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 257 insertions(+), 5 deletions(-)
> >
> > diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
> > index 0fdd251..4aa5e3d 100644
> > --- a/docs/specs/acpi_nvdimm.txt
> > +++ b/docs/specs/acpi_nvdimm.txt
> > @@ -127,6 +127,58 @@ _DSM process diagram:
> >   | result from the page     |      |              |
> >   +--------------------------+      +--------------+
> >
> > - _FIT implementation
> > - -------------------
> > - TODO (will fill it when nvdimm hotplug is introduced)
> > +Device Handle Reservation
> > +-------------------------
> > +As we mentioned above, byte 0 ~ byte 3 in the DSM memory save NVDIMM device
> > +handle. The handle is completely QEMU internal thing, the values in range
> > +[0, 0xFFFF] indicate nvdimm device (O means nvdimm root device named NVDR),
> > +other values are reserved by other purpose.
> > +
> > +Current reserved handle:
> > +0x10000 is reserved for QEMU internal DSM function called on the root
> > +device.
> Above part should go to section where 'handle' is defined, i.e. earlier in the 
> file:
>
>    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.

Okay.

> > +QEMU internal use only _DSM function
> > +------------------------------------
> > +UUID, 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62, is reserved for QEMU internal
> > +DSM function.
> > +
> > +There is the function introduced by QEMU and only used by QEMU internal.
> > +
> > +1) Read FIT
> UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT DSM function
> (private QEMU function)

okay.

> > +   As we only reserved one page for NVDIMM ACPI it is impossible to map the
> > +   whole FIT data to guest's address space. This function is used by _FIT
> > +   method to read a piece of FIT data from QEMU.
>  _FIT method uses Read_FIT function to fetch NFIT structures blob from QEMU
>  in 1 page sized increments which are then concatenated and returned as _FIT 
> method result.

okay.

> > +
> > +   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:
> > +
> > +   +----------+-------------+-------------+-----------------------------------+
> > +   |  Filed   | Byte Length | Byte Offset | Description                       |
>          ^ field,   s/Byte//,    s/Byte//
> > +   +----------+-------------+-------------+-----------------------------------+
> > +   | offset   |     4       |    0        | the offset of FIT buffer          |
> offset in QEMU's NFIT structures blob to read from

okay.

> > +   +----------+-------------+-------------+-----------------------------------+
> > +
> > +   Output:
> > +   +----------+-------------+-------------+-----------------------------------+
> > +   |  Filed   | Byte Length | Byte Offset | Description                       |
> > +   +----------+-------------+-------------+-----------------------------------+
> > +   |          |             |             | return status codes               |
> > +   |          |             |             |   0x100 indicates fit has been    |
> > +   | status   |     4       |    0        |   updated                         |
> 0x100 - error caused by NFIT update while read by _FIT wasn't completed

okay.

> > +   |          |             |             | other follows Chapter 3 in DSM    |
> s/other follows/other codes follow/

okay.

> > +   |          |             |             | Spec Rev1                         |
> > +   +----------+-------------+-------------+-----------------------------------+
> > +   | fit data |  Varies     |    4        | FIT data                          |
> > +   |          |             |             |                                   |
> > +   +----------+-------------+-------------+-----------------------------------+
> what does "Varies" mean, how would I know reading this how much data Read_FIT 
> should read
> from shared page?

I mean other bytes in the buffer returned by this function except the 'status' 
field
will be used as fit data. Maybe it is has a 'length' field?

> > +
> > +   The FIT offset is maintained by the caller itself,
> probably is not necessary sentence, or specify a caller (for example OSPM)

Okay.

> > current offset plugs
>                  ^^^^?

Typo. should be plus.

> > +   the length returned by the function is the next offset we should read.
> > +   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).
> [...]
> that's all for doc part, I'll do the code part later.

Thank you, Igor.