Re: [PATCH v5 5/7] hw/cxl/events: Add injection of General Media Events

[Markus Armbruster <armbru@redhat.com>]

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> From: Ira Weiny <ira.weiny@intel.com>
>
> To facilitate testing provide a QMP command to inject a general media
> event.  The event can be added to the log specified.
>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[...]

> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index ca3af3f0b2..9dcd308a49 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -5,6 +5,56 @@
>  # = CXL devices
>  ##
>  
> +##
> +# @CxlEventLog:
> +#
> +# CXL has a number of separate event logs for different types of event.

types of events

> +# Each such event log is handled and signaled independently.
> +#
> +# @informational: Information Event Log
> +# @warning: Warning Event Log
> +# @failure: Failure Event Log
> +# @fatal: Fatal Event Log

Are these proper nouns?  If not, the words should not be capitalized.

> +#
> +# Since: 8.1
> +##

Please format like

   ##
   # @CxlEventLog:
   #
   # CXL has a number of separate event logs for different types of
   # events.  Each such event log is handled and signaled independently.
   #
   # @informational: Information Event Log
   #
   # @warning: Warning Event Log
   #
   # @failure: Failure Event Log
   #
   # @fatal: Fatal Event Log
   #
   # Since: 8.1
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

The blank lines help with catching certain errors.  rST loves to
surprise...

> +{ 'enum': 'CxlEventLog',
> +  'data': ['informational',
> +           'warning',
> +           'failure',
> +           'fatal'
> +           ]

Make that
              'fatal']

> + }
> +
> +##
> +# @cxl-inject-gen-media-event:

Suggest cxl-inject-general-media-event, because we traditionally avoid
abbreviations in QMP, for better or worse.

> +#
> +# Inject an event record for a General Media Event (CXL r3.0 8.2.9.2.1.1)

What's "CXL r3.0", and where could a reader find it?

Aside: the idea of a document with numbered section nested six levels
deep is kind of horrifying :)

Again, capitalize "General Media Event" only if it's a proper noun.  If
"CXL r3.0" capitalizes it this way, it is.

> +# This event type is reported via one of the event logs specified via
> +# the log parameter.
> +#
> +# @path: CXL type 3 device canonical QOM path
> +# @log: Event Log to add the event to

event log

> +# @flags: header flags

Either specify the header flags here, or point to specification.

> +# @physaddr: Physical Address

Perhaps "Guest physical address"

Address of what?

We have no consistent naming convention for guest physical addresses.  I
see @addr, @memaddr, @gpa.  Let's not add yet another name for the same
thing without need.

> +# @descriptor: Descriptor

No.

> +# @type: Type

No.

> +# @transaction-type: Transaction Type

No.

> +# @channel: Channel

No.

> +# @rank: Rank

No.

> +# @device: Device

No.

See, I can do too terse, too!

> +# @component-id: Device specific string

Probably no.

> +#
> +# Since: 8.1
> +##

Again, please format like

   ##
   # @cxl-inject-general-media-event:
   #
   # Inject an event record for a General Media Event (CXL r3.0
   # 8.2.9.2.1.1) This event type is reported via one of the event logs
   # specified via the log parameter.
   #
   # @path: CXL type 3 device canonical QOM path
   #
   # @log: Event Log to add the event to
   #
   # @flags: header flags
   #
   # @physaddr: Physical Address
   #
   # @descriptor: Descriptor
   #
   # @type: Type
   #
   # @transaction-type: Transaction Type
   #
   # @channel: Channel
   #
   # @rank: Rank
   #
   # @device: Device
   #
   # @component-id: Device specific string
   #
   # Since: 8.1
   ##

> +{ 'command': 'cxl-inject-gen-media-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
> +            'physaddr': 'uint64', 'descriptor': 'uint8',
> +            'type': 'uint8', 'transaction-type': 'uint8',
> +            '*channel': 'uint8', '*rank': 'uint8',
> +            '*device': 'uint32', '*component-id': 'str'
> +            }}

Make that

               '*device': 'uint32', '*component-id': 'str' } }

> +
>  ##
>  # @cxl-inject-poison:
>  #