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

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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