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

[Igor Mammedov]

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.

> - @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.

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.

[...]
> +++ 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

[...]
> +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 ...

[...]