Re: [Qemu-devel] [PATCH v3 2/5] qmp: distinguish PC-DIMM and NVDIMM in MemoryDeviceInfoList

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 5 Mar 2018 13:14:34 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/05/2018 12:57 AM, Haozhong Zhang wrote:
> > It may need to treat PC-DIMM and NVDIMM differently, e.g., when
> > deciding the necessity of non-volatile flag bit in SRAT memory
> > affinity structures.
> > 
> > NVDIMMDeviceInfo, which inherits from PCDIMMDeviceInfo, is added to
> > union type MemoryDeviceInfo to record information of NVDIMM devices.
> > The NVDIMM-specific data is currently left empty and will be filled
> > when necessary in the future.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> >   hmp.c            | 14 +++++++++++---
> >   hw/mem/pc-dimm.c | 20 ++++++++++++++++++--
> >   numa.c           | 19 +++++++++++++------
> >   qapi-schema.json | 18 +++++++++++++++++-  
> 
> Will need rebasing now that the contents live in qapi/misc.json.
> 
> > +++ b/qapi-schema.json
> > @@ -2920,6 +2920,18 @@
> >             }
> >   }
> >   
> > +##
> > +# @NVDIMMDeviceInfo:
> > +#
> > +# NVDIMMDevice state information
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'NVDIMMDeviceInfo',
> > +  'base': 'PCDIMMDeviceInfo',
> > +  'data': {}
> > +}  
> 
> You added no data, so why did you need the type?
> 
> > +
> >   ##
> >   # @MemoryDeviceInfo:
> >   #
> > @@ -2927,7 +2939,11 @@
> >   #
> >   # Since: 2.1
> >   ##
> > -{ 'union': 'MemoryDeviceInfo', 'data': {'dimm': 'PCDIMMDeviceInfo'} }
> > +{ 'union': 'MemoryDeviceInfo',
> > +  'data': { 'dimm': 'PCDIMMDeviceInfo',
> > +            'nvdimm': 'NVDIMMDeviceInfo'  
> 
> Names aren't part of the interface; would it be better to rename 
> PCDIMMDeviceInfo into something that can be generically shared between 
> both the 'dimm' and 'nvdimm' branches without having to create a 
> pointless subtype?
Currently we are lying on query-memory-devices and reporting nvdimm
devices as pcdimm (I haven't noticed it when nvdimms were introduced),
this patch fixes it so it will be clear what type of device is used where.

Later we can extend data{} section with nvdimm specific info as needed.

The second goal of this patch (well, the actual reason I've asked for it),
is to reuse qmp_pc_dimm_device_list() internally in QEMU to get
list of dimm based memory devices, instead of creating yet another
API to do that for internal usage.