On Thu, 3 Nov 2016 20:21:05 +0800
Xiao Guangrong <guangrong.x...@linux.intel.com> wrote:
> On 11/03/2016 07:58 PM, Igor Mammedov wrote:
> > On Thu, 3 Nov 2016 11:51:29 +0800
> > Xiao Guangrong <guangrong.x...@linux.intel.com> 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 <guangrong.x...@linux.intel.com>
> >> ---
> >> 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 |
> +----------+--------+--------+-------------------------------------------+
you need to add length here, otherwise this table is not correct
> | | | | 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.
