From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c2Hd4-0006nO-La for qemu-devel@nongnu.org; Thu, 03 Nov 2016 09:00:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c2Hcy-0004YF-Se for qemu-devel@nongnu.org; Thu, 03 Nov 2016 09:00:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41456) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c2Hcy-0004YA-Jw for qemu-devel@nongnu.org; Thu, 03 Nov 2016 09:00:20 -0400 Date: Thu, 3 Nov 2016 14:00:15 +0100 From: Igor Mammedov Message-ID: <20161103140015.5f07b009@nial.brq.redhat.com> In-Reply-To: <2f770790-f3e9-ce56-e11b-e9f7e73e7078@linux.intel.com> References: <1478145090-11987-1-git-send-email-guangrong.xiao@linux.intel.com> <1478145090-11987-3-git-send-email-guangrong.xiao@linux.intel.com> <20161103125808.11857a35@nial.brq.redhat.com> <2f770790-f3e9-ce56-e11b-e9f7e73e7078@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 2/3] nvdimm acpi: introduce _FIT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong Cc: pbonzini@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, mst@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org On Thu, 3 Nov 2016 20:21:05 +0800 Xiao Guangrong wrote: > On 11/03/2016 07:58 PM, Igor Mammedov wrote: > > On Thu, 3 Nov 2016 11:51:29 +0800 > > Xiao Guangrong wrote: > > =20 > >> _FIT is required for hotplug support, guest will inquire the updated > >> device info from it if a hotplug event is received =20 > > s/_FIT/_FIT method/ > > > > the same applies to subj. line =20 >=20 > Okay. >=20 > > =20 > >> > >> 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 =20 > > > > Commit message hard to read/parse, it might be better if I'd use simple > > short sentences. =20 >=20 > Okay, will change it to: >=20 > 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 >=20 > > > > =20 > >> Refer to docs/specs/acpi-nvdimm.txt for detailed design > >> > >> Signed-off-by: Xiao Guangrong > >> --- > >> 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 > >> -=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D > >> +QEMU NVDIMM Implementation > >> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > >> 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 NVDI= MM > >> Root device. > >> + > >> + The handle is completely QEMU internal thing, the val= ues in > >> + range [0, 0xFFFF] indicate nvdimm device (O means nvd= imm =20 > > [1, 0xFFFF] indicate nvdimm device > > and move 0 to reserved section =20 >=20 > Okay. >=20 > > =20 > >> + root device named NVDR), other values are reserved by= other =20 > > s/by/for/ =20 >=20 > okay. >=20 > > =20 > >> + purpose. =20 > > s/purpose/purposes/ =20 >=20 > okay. >=20 > > =20 > >> + Current reserved handle: =20 > > s/Current reserved handle/Reserved handles/ > > =20 > >> + 0x10000 is reserved for QEMU internal DSM function ca= lled on > >> + the root device. =20 > > description is too obscure, I wonder if it could be more specific =20 >=20 > 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. >=20 > As currently we do not have reserved method on nvdimm device, so i > only document 0x1000 for the root device. >=20 > > > > =20 > >> [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 met= hod. > >> [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 intern= al. =20 > > drop it > > =20 > >> +1) Read FIT > >> + UUID 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62 is reserved for Read_FIT= DSM > >> + function (private QEMU function) =20 > > not necessary, drop it. Maybe extend UUID description in > > "Input parameters:" section > > =20 >=20 > okay. >=20 > > =20 > >> + _FIT method uses Read_FIT function to fetch NFIT structures blob f= rom =20 > > s/Read_FIT function/_DSM method/ =20 >=20 > okay. >=20 > > =20 > >> + QEMU in 1 page sized increments which are then concatenated and re= turned > >> + as _FIT method result. > >> + > >> + Input parameters: > >> + Arg0 =E2=80=93 UUID {set to 648B9CF2-CDA1-4312-8AD9-49C4AF32BD62} > >> + Arg1 =E2=80=93 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 bl= ob 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, othe= r | > >> + | | | | 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 pl= us > >> + the length returned by the function is the next offset we should r= ead. =20 > > there shouldn't be 'we' or anything personal in spec =20 >=20 > Okay, change it to OSPM. :) >=20 > > =20 > >> + 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 of= fset 0 > >> + again). =20 > > > > PS: > > fix typos and fix spelling/grammatical errors you are adding in above = text. =20 >=20 > Sorry for the poor English, will check it carefully. >=20 > > > > =20 > >> 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 NvdimmFun= cSetLabelDataIn; > >> 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. */ =20 > > 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) =20 >=20 > Okay, i will change the spec like this: >=20 > QEMU Writes Output Data (based on the offset in the page): > [0x0 - 0x3]: 4 bytes, length of entire returned data (including the h= eader) >=20 > And drop the length field in Read_Fit return buffer, doc > the fit buffer like this: >=20 > +----------+--------+--------+---------------------------------------= ----+ > | 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 wh= ile | > | status | 4 | 0 | read by _FIT wasn't completed, other = | > | | | | codes follow Chapter 3 in DSM Spec Rev= 1 | > +----------+--------+--------+---------------------------------------= ----+ > | fit data | Varies | 8 | FIT data, The remaining size in the = | > | | | | returned buffer is used by FIT = | > +----------+--------+--------+---------------------------------------= ----+ >=20 >=20 >=20 > >> +} > >> + > >> +static void nvdimm_dsm_reserved_root(AcpiNVDIMMState *state, NvdimmDs= mIn *in, > >> + hwaddr dsm_mem_addr) =20 > > function name doesn't make any sense to me =20 >=20 > As i explained above, handle 0x10000 indicates the reserved _DSM method is > called on the root device... >=20 > It makes sense now? :) function name should reflect what it does, i.e use verb and I see only nouns here.