Jinjie Ruan <[email protected]> writes:
> From: Jonathan Cameron <[email protected]>
>
> Add the query-mpam-cache QMP command schema: the MpamBm bitmap
> type, the per-RIS MpamRegs state and the MpamCacheInfo summary of
> a cache MSC, plus an optional cache level filter. The command is
> stubbed to return NULL so the generated marshal links; the real
> implementation walks the device tree in the next patch.
>
> Based on an RFC series by Jonathan Cameron, adapted to the current
> qapi documentation rules.
>
> Signed-off-by: Jonathan Cameron <[email protected]>
> Signed-off-by: Jinjie Ruan <[email protected]>
[...]
> diff --git a/qapi/meson.build b/qapi/meson.build
> index a46269b..c6663e5 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -42,6 +42,7 @@ qapi_all_modules = [
> 'machine-s390x',
> 'migration',
> 'misc',
> + 'mpam',
> 'net',
> 'pragma',
> 'qom',
> diff --git a/qapi/mpam.json b/qapi/mpam.json
> new file mode 100644
> index 0000000..ff476a1
> --- /dev/null
> +++ b/qapi/mpam.json
> @@ -0,0 +1,120 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +##
> +# = ARM MPAM State Introspection
> +##
Special QAPI doc markup that hasn't been recognized in more than a year.
Please build with --enable-docs, and examine
docs/manual/interop/qemu-qmp-ref.html to guard against such mishaps.
Use standard reST markup instead:
##
# ****************************
# ARM MPAM State Introspection
# ****************************
##
Are you *sure* this ought to be between "Miscellanea" and "Audio" and at
the same heading level? Or should it be deeper and/or elsewhere?
> +
> +##
> +# @MpamBm:
> +#
> +# A bitmap control value, as used by MPAM cache-portion (CPBM) and
> +# memory-bandwidth-portion (MBW_PBM) controls. The value is
> +# carried as an array of 32-bit words, least significant first.
> +#
Anti-pattern: the introduction (above) discusses the value of a member
(below).
> +# @words: The bitmap, one element per 32-bits of the control
> +#
> +# Since: 10.1
11.2 now. Same elsewhere, not noting it again.
> +##
> +{ 'struct': 'MpamBm',
> + 'data' : { 'words' : [ 'int' ]
The description says it's 32 bit words. Shouldn't you use 'uint32'
then?
> + }
> +}
> +
> +##
> +# @MpamRegs:
> +#
> +# Per RIS register state of an emulated MPAM Memory System Component.
Please use the current syntax for introductions:
##
# @name: introduction, best to keep it brief, but you may continue it
# like this if necessary. If your text need to refer to
# arguments, it's almost certainly not brief introduction
# anymore.
Same elsewhere, not noting it again.
> +#
> +# @idr: MPAMF_IDR, the MPAM features identification register
> +#
> +# @iidr: MPAMF_IIDR, the MPAM implementer identification register
> +#
> +# @aidr: MPAMF_AIDR, the MPAM architecture identification register
> +#
> +# @cpor-idr: MPAMF_CPOR_IDR, cache-portion feature register
> +#
> +# @ccap-idr: MPAMF_CCAP_IDR, cache-capacity feature register
> +#
> +# @mbw-idr: MPAMF_MBW_IDR, memory-bandwidth feature register
> +#
> +# @pri-idr: MPAMF_PRI_IDR, priority feature register
> +#
> +# @partid-nrw-idr: MPAMF_PARTID_NRW_IDR, PARTID narrowing register
> +#
> +# @msmon-idr: MPAMF_MSMON_IDR, monitor feature register
> +#
> +# @csumon-idr: MPAMF_CSUMON_IDR, cache-usage monitor register
> +#
> +# @mbwumon-idr: MPAMF_MBWUMON_IDR, memory-bandwidth monitor register
> +#
> +# @ecr: MPAMF_ECR, the error control register
> +#
> +# @esr: MPAMF_ESR, the error status register
> +#
> +# @cfg-part-sel: MPAMCFG_PART_SEL, the partition-selection register
> +#
> +# @cfg-cpbm: The configured cache-portion bitmaps, one per
> +# internal PARTID
> +#
> +# Since: 10.1
> +##
> +{ 'struct' : 'MpamRegs',
> + 'data' : { 'idr' : 'int',
> + 'iidr' : 'int',
> + 'aidr' : 'int',
> + 'cpor-idr': 'int',
> + 'ccap-idr': 'int',
> + 'mbw-idr': 'int',
> + 'pri-idr': 'int',
> + 'partid-nrw-idr': 'int',
> + 'msmon-idr': 'int',
> + 'csumon-idr': 'int',
> + 'mbwumon-idr': 'int',
> + 'ecr': 'int',
> + 'esr': 'int',
> + 'cfg-part-sel': 'int',
> + 'cfg-cpbm': ['MpamBm']#lazy
Is the comment an accident?
How wide are these registers? Is 'int' the appropriate type?
> + }
> +}
> +
> +##
> +# @MpamCacheInfo:
> +#
> +# Information about an MPAM cache Memory System Component.
> +#
> +# @cpu: First CPU of the set associated with the cache
> +#
> +# @level: Level of cache
> +#
> +# @type: Type of cache
> +#
> +# @regs: The per RIS register state
> +#
> +# Since: 10.1
> +##
> +{ 'struct': 'MpamCacheInfo',
> + 'data' : { 'cpu' : 'int',
> + 'level' : 'int',
> + 'type' : 'int',
> + 'regs' : ['MpamRegs']
> + }
> +}
> +
> +##
> +# @query-mpam-cache:
> +#
> +# Get a list of MpamCacheInfo for all Cache related MSC
> +#
> +# @level: Provide a cache level to filter against
What exactly does this argument filter out?
Is it really needed? How large can unfiltered output become?
> +#
> +# Returns: a list of @MpamCacheInfo describing all
> +# the MPAM cache MSC instances
This gets rendered like
Return: [MpamCacheInfo] – a list of MpamCacheInfo describing all the
MPAM cache MSC instances
As is, the description is mostly redundant. Do you need a description?
Try building without it, and see how it comes out.
> +#
> +# Since: 10.1
> +##
> +{ 'command': 'query-mpam-cache',
> + 'data': { '*level': 'int' },
> + 'returns' : ['MpamCacheInfo'],
> + 'allow-preconfig': true }
> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> index b93dd68..cef175d 100644
> --- a/qapi/qapi-schema.json
> +++ b/qapi/qapi-schema.json
> @@ -64,6 +64,7 @@
> { 'include': 'replay.json' }
> { 'include': 'yank.json' }
> { 'include': 'misc.json' }
> +{ 'include': 'mpam.json' }
> { 'include': 'misc-arm.json' }
> { 'include': 'misc-i386.json' }
> { 'include': 'audio.json' }
