Re: [PATCH v6 6/7] hw/cxl/events: Add injection of DRAM events

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Jonathan,

On 19/5/23 16:30, Jonathan Cameron wrote:
> Defined in CXL r3.0 8.2.9.2.1.2 DRAM Event Record, this event
> provides information related to DRAM devices.
> 
> Example injection command in QMP:
> 
> { "execute": "cxl-inject-dram-event",
>      "arguments": {
>          "path": "/machine/peripheral/cxl-mem0",
>          "log": "informational",
>          "flags": 1,
>          "physaddr": 1000,
>          "descriptor": 3,
>          "type": 3,
>          "transaction-type": 192,
>          "channel": 3,
>          "rank": 17,
>          "nibble-mask": 37421234,
>          "bank-group": 7,
>          "bank": 11,
>          "row": 2,
>          "column": 77,
>          "correction-mask": [33, 44, 55,66]
>      }}
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/mem/cxl_type3.c          | 116 ++++++++++++++++++++++++++++++++++++
>   hw/mem/cxl_type3_stubs.c    |  13 ++++
>   include/hw/cxl/cxl_events.h |  23 +++++++
>   qapi/cxl.json               |  35 +++++++++++
>   4 files changed, 187 insertions(+)


> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index 7e1e6257ce..5e82097e76 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -55,6 +55,41 @@
>               '*device': 'uint32', '*component-id': 'str'
>               }}
>   
> +##
> +# @cxl-inject-dram-event:
> +#
> +# Inject an event record for a DRAM Event (CXL r3.0 8.2.9.2.1.2)
> +# 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

Could this be a clearer description?

"Physical Address (relative to @path device)"

> +# @descriptor: Descriptor
> +# @type: Type
> +# @transaction-type: Transaction Type
> +# @channel: Channel
> +# @rank: Rank
> +# @nibble-mask: Identify one or more nibbles that the error affects

> +# @bank-group: Bank group
> +# @bank: Bank
> +# @row: Row
> +# @column: Column

Why do we need bank/raw/col if we have physaddr?

These are optional. Shouldn't we check they are valid
in qmp_cxl_inject_dram_event()? (No clue, just wondering
if there is some duplication here).

> +# @correction-mask: Bits within each nibble. Used in order of bits set
> +#                   in the nibble-mask.  Up to 4 nibbles may be covered.
> +#
> +# Since: 8.1
> +##
> +{ 'command': 'cxl-inject-dram-event',
> +  'data': { 'path': 'str', 'log': 'CxlEventLog', 'flags': 'uint8',
> +            'physaddr': 'uint64', 'descriptor': 'uint8',
> +            'type': 'uint8', 'transaction-type': 'uint8',
> +            '*channel': 'uint8', '*rank': 'uint8', '*nibble-mask': 'uint32',
> +            '*bank-group': 'uint8', '*bank': 'uint8', '*row': 'uint32',
> +            '*column': 'uint16', '*correction-mask': [ 'uint64' ]
> +           }}
> +
>   ##
>   # @cxl-inject-poison:
>   #