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

[Markus Armbruster]

Haozhong Zhang <address@hidden> 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 <address@hidden>
>> > ---
>> > Cc: Markus Armbruster <address@hidden>
>> 
>> > +++ 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.

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

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".