From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57009) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZX5V7-0003ZJ-O1 for qemu-devel@nongnu.org; Wed, 02 Sep 2015 06:42:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZX5V3-0004Gf-Kf for qemu-devel@nongnu.org; Wed, 02 Sep 2015 06:42:45 -0400 Received: from mga14.intel.com ([192.55.52.115]:10146) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZX5V3-0004FR-Ed for qemu-devel@nongnu.org; Wed, 02 Sep 2015 06:42:41 -0400 References: <1439563931-12352-1-git-send-email-guangrong.xiao@linux.intel.com> <1439563931-12352-7-git-send-email-guangrong.xiao@linux.intel.com> <20150902115845.01472189@nial.brq.redhat.com> From: Xiao Guangrong Message-ID: <55E6D13B.5070106@linux.intel.com> Date: Wed, 2 Sep 2015 18:36:43 +0800 MIME-Version: 1.0 In-Reply-To: <20150902115845.01472189@nial.brq.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/18] pc: implement NVDIMM device abstract List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com, rth@twiddle.net On 09/02/2015 05:58 PM, Igor Mammedov wrote: > On Fri, 14 Aug 2015 22:51:59 +0800 > Xiao Guangrong wrote: > >> Introduce "pc-nvdimm" device and it has two parameters: > Why do you use prefix "pc-", I suppose we potentially > could use this device not only with x86 targets but with > other targets as well. > I'd just drop 'pc' prefix through out patchset. Yeah, the prefix is stolen from pc-dimm, will drop this prefix as your suggestion. > >> - @file, which is the backed memory file for NVDIMM device > Could you try to split device into backend/frontend parts, > like it's done with pc-dimm. As I understand it's preferred > way to implement this kind of devices. > Then you could reuse memory backends that we already have > including file backend. I considered it too and Stefan, Paolo got the some idea in V1's review, however: | However, file-based memory used by NVDIMM is special, it divides the file | to two parts, one part is used as PMEM and another part is used to store | NVDIMM's configure data. | | Maybe we can introduce "end-reserved" property to reserve specified size | at the end of the file. Or create a new class type based on | memory-backend-file (named nvdimm-backend-file) class to hide this magic | thing? Your idea? > > So CLI could look like: > -object memory-backend-file,id=mem0,file=/storage/foo > -device nvdimm,memdev=mem0,configdata=on > >> >> - @configdata, specify if we need to reserve 128k at the end of >> @file for nvdimm device's config data. Default is false >> >> If @configdata is false, Qemu will build a static and readonly >> namespace in memory and use it serveing for >> DSM GET_CONFIG_SIZE/GET_CONFIG_DATA requests. >> This is good for the user who want to pass whole nvdimm device >> and make its data is complete visible to guest >> >> We can use "-device pc-nvdimm,file=/dev/pmem,configdata" in the >> Qemu command to create NVDIMM device for the guest > PS: > please try to fix commit message spelling/grammar wise. Sorry for my careless, will try it fix them. > > [...] >> +++ b/hw/mem/nvdimm/pc-nvdimm.c >> @@ -0,0 +1,99 @@ >> +/* >> + * NVDIMM (A Non-Volatile Dual In-line Memory Module) Virtualization Implement > s/Implement/Implementation/ in all new files > an maybe s/NVDIMM (A // as it's reduntant Okay, will drop it. > > [...] >> +static bool has_configdata(Object *obj, Error **errp) >> +{ >> + PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj); >> + >> + return nvdimm->configdata; >> +} >> + >> +static void set_configdata(Object *obj, bool value, Error **errp) >> +{ >> + PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj); >> + >> + nvdimm->configdata = value; >> +} > usually for property setters/getters we use form: > "device_prefix"_[g|s]et_foo > so > nvdim_get_configdata ... Good to me. Thanks for your review, Igor!