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

[Haozhong Zhang]

On 03/05/18 13:14 -0600, Eric Blake 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 <address@hidden>
> > ---
> >   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.

will do

> 
> > +++ 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?
>

The purpose of this NVDIMMDeviceInfo is to introduce
MEMORY_DEVICE_INFO_KIND_NVDIMM, which can be used to distinguish
NVDIMM from PC-DIMM in the list returned from query-memory-device.

If 'data' of NVDIMMDeviceInfo is filled with NVDIMM-specific
information (there does have some), would it make this type less
pointless?

Thanks,
Haozhong