| Message ID | 20180305065710.25876-3-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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 <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. 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
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.
On Mon, 5 Mar 2018 14:57:07 +0800 Haozhong Zhang <haozhong.zhang@intel.com> 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. Maybe add to commit message that: It also fixes "info memory-devices"/query-memory-devices which currently show nvdimm devices as dimm devices since object_dynamic_cast(obj, TYPE_PC_DIMM) happily cast nvdimm to TYPE_PC_DIMM which it's been inherited from. Otherwise patch looks good, I'll ack it after rebase with is necessary for this patch. > 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 +++++++++++++++++- > 4 files changed, 59 insertions(+), 12 deletions(-) > > diff --git a/hmp.c b/hmp.c > index ae86bfbade..3f06407c5e 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -2413,7 +2413,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict) > switch (value->type) { > case MEMORY_DEVICE_INFO_KIND_DIMM: > di = value->u.dimm.data; > + break; > + > + case MEMORY_DEVICE_INFO_KIND_NVDIMM: > + di = qapi_NVDIMMDeviceInfo_base(value->u.nvdimm.data); > + break; > + > + default: > + di = NULL; > + break; > + } > > + if (di) { > monitor_printf(mon, "Memory device [%s]: \"%s\"\n", > MemoryDeviceInfoKind_str(value->type), > di->id ? di->id : ""); > @@ -2426,9 +2437,6 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict) > di->hotplugged ? "true" : "false"); > monitor_printf(mon, " hotpluggable: %s\n", > di->hotpluggable ? "true" : "false"); > - break; > - default: > - break; > } > } > } > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 4d050fe2cd..866ecc699a 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -20,6 +20,7 @@ > > #include "qemu/osdep.h" > #include "hw/mem/pc-dimm.h" > +#include "hw/mem/nvdimm.h" > #include "qapi/error.h" > #include "qemu/config-file.h" > #include "qapi/visitor.h" > @@ -249,10 +250,19 @@ MemoryDeviceInfoList *qmp_pc_dimm_device_list(void) > Object *obj = OBJECT(dimm); > MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1); > MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1); > - PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1); > + PCDIMMDeviceInfo *di; > + NVDIMMDeviceInfo *ndi; > + bool is_nvdimm = object_dynamic_cast(obj, TYPE_NVDIMM); > DeviceClass *dc = DEVICE_GET_CLASS(obj); > DeviceState *dev = DEVICE(obj); > > + if (!is_nvdimm) { > + di = g_new0(PCDIMMDeviceInfo, 1); > + } else { > + ndi = g_new0(NVDIMMDeviceInfo, 1); > + di = qapi_NVDIMMDeviceInfo_base(ndi); > + } > + > if (dev->id) { > di->has_id = true; > di->id = g_strdup(dev->id); > @@ -265,7 +275,13 @@ MemoryDeviceInfoList *qmp_pc_dimm_device_list(void) > di->size = object_property_get_uint(obj, PC_DIMM_SIZE_PROP, NULL); > di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem)); > > - info->u.dimm.data = di; > + if (!is_nvdimm) { > + info->u.dimm.data = di; > + info->type = MEMORY_DEVICE_INFO_KIND_DIMM; > + } else { > + info->u.nvdimm.data = ndi; > + info->type = MEMORY_DEVICE_INFO_KIND_NVDIMM; > + } > elem->value = info; > elem->next = NULL; > if (prev) { > diff --git a/numa.c b/numa.c > index c6734ceb8c..23c4371e51 100644 > --- a/numa.c > +++ b/numa.c > @@ -529,18 +529,25 @@ static void numa_stat_memory_devices(NumaNodeMem node_mem[]) > > if (value) { > switch (value->type) { > - case MEMORY_DEVICE_INFO_KIND_DIMM: { > + case MEMORY_DEVICE_INFO_KIND_DIMM: > pcdimm_info = value->u.dimm.data; > + break; > + > + case MEMORY_DEVICE_INFO_KIND_NVDIMM: > + pcdimm_info = qapi_NVDIMMDeviceInfo_base(value->u.nvdimm.data); > + break; > + > + default: > + pcdimm_info = NULL; > + break; > + } > + > + if (pcdimm_info) { > node_mem[pcdimm_info->node].node_mem += pcdimm_info->size; > if (pcdimm_info->hotpluggable && pcdimm_info->hotplugged) { > node_mem[pcdimm_info->node].node_plugged_mem += > pcdimm_info->size; > } > - break; > - } > - > - default: > - break; > } > } > } > diff --git a/qapi-schema.json b/qapi-schema.json > index cd98a94388..1c2d281749 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2920,6 +2920,18 @@ > } > } > > +## > +# @NVDIMMDeviceInfo: > +# > +# NVDIMMDevice state information > +# > +# Since: 2.12 > +## > +{ 'struct': 'NVDIMMDeviceInfo', > + 'base': 'PCDIMMDeviceInfo', > + 'data': {} > +} > + > ## > # @MemoryDeviceInfo: > # > @@ -2927,7 +2939,11 @@ > # > # Since: 2.1 > ## > -{ 'union': 'MemoryDeviceInfo', 'data': {'dimm': 'PCDIMMDeviceInfo'} } > +{ 'union': 'MemoryDeviceInfo', > + 'data': { 'dimm': 'PCDIMMDeviceInfo', > + 'nvdimm': 'NVDIMMDeviceInfo' > + } > +} > > ## > # @query-memory-devices:
diff --git a/hmp.c b/hmp.c index ae86bfbade..3f06407c5e 100644 --- a/hmp.c +++ b/hmp.c @@ -2413,7 +2413,18 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict) switch (value->type) { case MEMORY_DEVICE_INFO_KIND_DIMM: di = value->u.dimm.data; + break; + + case MEMORY_DEVICE_INFO_KIND_NVDIMM: + di = qapi_NVDIMMDeviceInfo_base(value->u.nvdimm.data); + break; + + default: + di = NULL; + break; + } + if (di) { monitor_printf(mon, "Memory device [%s]: \"%s\"\n", MemoryDeviceInfoKind_str(value->type), di->id ? di->id : ""); @@ -2426,9 +2437,6 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict) di->hotplugged ? "true" : "false"); monitor_printf(mon, " hotpluggable: %s\n", di->hotpluggable ? "true" : "false"); - break; - default: - break; } } } diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 4d050fe2cd..866ecc699a 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -20,6 +20,7 @@ #include "qemu/osdep.h" #include "hw/mem/pc-dimm.h" +#include "hw/mem/nvdimm.h" #include "qapi/error.h" #include "qemu/config-file.h" #include "qapi/visitor.h" @@ -249,10 +250,19 @@ MemoryDeviceInfoList *qmp_pc_dimm_device_list(void) Object *obj = OBJECT(dimm); MemoryDeviceInfoList *elem = g_new0(MemoryDeviceInfoList, 1); MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1); - PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1); + PCDIMMDeviceInfo *di; + NVDIMMDeviceInfo *ndi; + bool is_nvdimm = object_dynamic_cast(obj, TYPE_NVDIMM); DeviceClass *dc = DEVICE_GET_CLASS(obj); DeviceState *dev = DEVICE(obj); + if (!is_nvdimm) { + di = g_new0(PCDIMMDeviceInfo, 1); + } else { + ndi = g_new0(NVDIMMDeviceInfo, 1); + di = qapi_NVDIMMDeviceInfo_base(ndi); + } + if (dev->id) { di->has_id = true; di->id = g_strdup(dev->id); @@ -265,7 +275,13 @@ MemoryDeviceInfoList *qmp_pc_dimm_device_list(void) di->size = object_property_get_uint(obj, PC_DIMM_SIZE_PROP, NULL); di->memdev = object_get_canonical_path(OBJECT(dimm->hostmem)); - info->u.dimm.data = di; + if (!is_nvdimm) { + info->u.dimm.data = di; + info->type = MEMORY_DEVICE_INFO_KIND_DIMM; + } else { + info->u.nvdimm.data = ndi; + info->type = MEMORY_DEVICE_INFO_KIND_NVDIMM; + } elem->value = info; elem->next = NULL; if (prev) { diff --git a/numa.c b/numa.c index c6734ceb8c..23c4371e51 100644 --- a/numa.c +++ b/numa.c @@ -529,18 +529,25 @@ static void numa_stat_memory_devices(NumaNodeMem node_mem[]) if (value) { switch (value->type) { - case MEMORY_DEVICE_INFO_KIND_DIMM: { + case MEMORY_DEVICE_INFO_KIND_DIMM: pcdimm_info = value->u.dimm.data; + break; + + case MEMORY_DEVICE_INFO_KIND_NVDIMM: + pcdimm_info = qapi_NVDIMMDeviceInfo_base(value->u.nvdimm.data); + break; + + default: + pcdimm_info = NULL; + break; + } + + if (pcdimm_info) { node_mem[pcdimm_info->node].node_mem += pcdimm_info->size; if (pcdimm_info->hotpluggable && pcdimm_info->hotplugged) { node_mem[pcdimm_info->node].node_plugged_mem += pcdimm_info->size; } - break; - } - - default: - break; } } } diff --git a/qapi-schema.json b/qapi-schema.json index cd98a94388..1c2d281749 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2920,6 +2920,18 @@ } } +## +# @NVDIMMDeviceInfo: +# +# NVDIMMDevice state information +# +# Since: 2.12 +## +{ 'struct': 'NVDIMMDeviceInfo', + 'base': 'PCDIMMDeviceInfo', + 'data': {} +} + ## # @MemoryDeviceInfo: # @@ -2927,7 +2939,11 @@ # # Since: 2.1 ## -{ 'union': 'MemoryDeviceInfo', 'data': {'dimm': 'PCDIMMDeviceInfo'} } +{ 'union': 'MemoryDeviceInfo', + 'data': { 'dimm': 'PCDIMMDeviceInfo', + 'nvdimm': 'NVDIMMDeviceInfo' + } +} ## # @query-memory-devices:
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 +++++++++++++++++- 4 files changed, 59 insertions(+), 12 deletions(-)