Re: [PATCH 36/37] qapi: Add MPAM cache introspection schema and stub

[Markus Armbruster <armbru@redhat.com>]

Jinjie Ruan <2118537295@qq.com> writes:

> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> 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 <Jonathan.Cameron@huawei.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

[...]

> 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' }