Re: [RFC QEMU PATCH 8/8] qmp: add a qmp command 'query-nvdimms' to get plugged NVDIMM devices

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 10/11/16 10:22, Markus Armbruster wrote:
> Haozhong Zhang <haozhong.zhang@intel.com> writes:
> 
> > On 10/10/16 14:16, Eric Blake wrote:
> >> On 10/09/2016 07:34 PM, Haozhong Zhang wrote:
> >> > Xen uses this command to get the backend resource, guest SPA and size of
> >> > NVDIMM devices so as to map them to guest.
> >> > 
> >> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> > ---
> >> > Cc: Markus Armbruster <armbru@redhat.com>
> >> 
> >> > +++ b/docs/qmp-commands.txt
> >> > @@ -3800,3 +3800,39 @@ Example for pc machine type started with
> >> >              "props": {"core-id": 0, "socket-id": 0, "thread-id": 0}
> >> >           }
> >> >         ]}
> >> > +
> >> > +EQMP
> >> > +
> >> > +    {
> >> > +        .name       = "query-nvdimms",
> >> > +        .args_type  = "",
> >> > +        .mhandler.cmd_new = qmp_marshal_query_nvdimms,
> >> 
> >> Needs rebasing - we no longer need SQMP/EQMP sections or callouts to the
> >> initializers, now that commit bd6092e4 has automated the mapping of QAPI
> >> to command registration.
> >>
> >
> > good to know this, will change in next version
> >
> >> > +    },
> >> > +
> >> > +SQMP
> >> > +Show plugged NVDIMM devices
> >> > +---------------------------
> >> > +
> >> > +Arguments: None.
> >> > +
> >> > +Example for pc machine type started with
> >> > +-object memory-backend-file,id=mem1,mem-path=/path/to/nvm1,size=4G
> >> > +-device nvdimm,id=nvdimm1,memdev=mem1
> >> > +-object memory-backend-file,id=mem2,mem-path=/path/to/nvm2,size=8G
> >> > +-device nvdimm,id=nvdimm2,memdev=mem2:
> >> > +
> >> > +-> { "execute": "query-nvdimms" }
> >> > +<- { "returns": [
> >> > +      {
> >> > +         "mem-path": "/path/to/nvm1",
> >> > +	 "slot": 0,
> >> 
> >> TAB damage; please fix.
> >>
> >
> > will fix
> >
> >> > +	 "spa": 17179869184,
> >> > +	 "length": 4294967296
> >> > +      },
> >> > +      {
> >> > +         "mem-path": "/path/to/nvm2",
> >> > +	 "slot": 1,
> >> > +	 "spa": 21474836480,
> >> > +	 "length": 8589934592
> >> > +      }
> >> > +   ]}
> >> 
> >> > +++ b/qapi-schema.json
> >> > @@ -4646,3 +4646,32 @@
> >> >  # Since: 2.7
> >> >  ##
> >> >  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> >> > +
> >> > +##
> >> > +# @NvdimmInfo
> >> > +#
> >> > +# Information about an NVDIMM device.
> >> > +#
> >> > +# @mem-path: the backend file of the NVDIMM device
> >> > +#
> >> > +# @slot: the slot index of the NVDIMM device
> >> > +#
> >> > +# @spa: the 64-bit SPA base address of the NVDIMM device
> >> > +#
> >> > +# @length: the 64-bit size in bytes of the NVDIMM device
> >> > +#
> >> > +# Since 2.8
> >> > +##
> >> > +{ 'struct': 'NvdimmInfo',
> >> > +  'data': {'mem-path' : 'str', 'slot': 'int', 'spa': 'int', 'length': 'int'} }
> >> > +
> >> > +##
> >> > +# @query-nvdimms:
> >> > +#
> >> > +# Returns information about each NVDIMM device
> >> > +#
> >> > +# Returns: a list of @NvdimmInfo for each device
> >> > +#
> >> > +# Since: 2.8
> >> > +##
> >> > +{ 'command': 'query-nvdimms', 'returns': ['NvdimmInfo'] }
> >> > 
> >> 
> >> Is this something that can be added to the existing query-memdev or
> >> query-memory-devices command, instead of needing a new command?
> >>
> >
> > Both query-memdev and query-memory-devices do not tell whether a
> > device is NVDIMM. If adding an optional argument to existing a qmp
> > command does not break backwards compatibility, I propose to add an
> > argument "nvdimm" to query-memory-devices which controls whether this
> > command to return all memory devices or only NVDIMM devices.
> 
> query-memory-devices returns a list of MemoryDeviceInfo:
> 
>     ##
>     # @MemoryDeviceInfo:
>     #
>     # Union containing information about a memory device
>     #
>     # Since: 2.1
>     ##
>     { 'union': 'MemoryDeviceInfo', 'data': {'dimm': 'PCDIMMDeviceInfo'} }
> 
> This is a union, designed to be extended for other types of memory
> device frontends.
> 
> Sadly, it's a "simple" union (I dislike those).
> 
> You could add a new member to be used for the NVDIMM case.  It would
> probably duplicate some or all of PCDIMMDeviceInfo, though.
>

Cool, I didn't notice it's a union.

> If it needs all of PCDIMMDeviceInfo, you could make the new member's
> type have PCDIMMDeviceInfo as base.

Information so far queried in my patch is just a subset of
PCDIMMDeviceInfo.  Additional information (e.g. location of NVDIMM
label area) may be needed in the future, so I would like to add a new
type NVDIMMDeviceInfo with PCDIMMDeviceInfo as base.

Thanks,
Haozhong

> 
> If we can identify information *any* memory device frontend should have,
> the appropriate design would be a flat union with common information
> common, and type-specific information in union branches.  Could
> MemoryDeviceInfo be backward-compatibly morphed into such a type?
> 
> We also have query-memdev, which is about memory backends.
> PCDIMMDeviceInfo member memdev links from the frontend to the backend.
> 
> Aside: calling the backends "memdev" and the frontends "memory-devices"
> is very bad taste.  It's also very traditional for QEMU: we call
> everything "device" or maybe "driver".
>