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

[Eric Blake]

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.

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

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org