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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 19 May 2023 17:34:20 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

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

Makes sense.

> 
> > +# @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?

Yes we need them. We don't know the device geometry / internal interleaving
/ address hashing applied to smooth out access patterns etc.

I really don't want to put that level of complexity into the command
line for a device - so just left it to the test tools to squirt in
something valid.

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

Validation is really hard for these as depends on the above
device implementation complexity.  There is a note on trying to
strike the balance in the cover letter. I'm not sure I have it
right! They are optional in records coming from the device, so
we set validity flags for them in the device record.

Aim here is to be able to inject whatever might be seen on a real device
without having to have QEMU emulate a bunch of device internals
such as mappings to particular DRAM FRU, chip, column, row etc.


> 
> > +# @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:
> >   #  
>