Re: [Qemu-devel] [PATCH v2 06/18] pc: implement NVDIMM device abstract

[Xiao Guangrong]

On 09/02/2015 05:58 PM, Igor Mammedov wrote:
> On Fri, 14 Aug 2015 22:51:59 +0800
> Xiao Guangrong <address@hidden> 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!