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

[Philippe Mathieu-Daudé]

On 19/5/23 17:45, Jonathan Cameron wrote:
> 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.

I was expecting some check like:

  ROUND_DOWN(physaddr, 8) == bank * row * column * 8

But indeed this isn't really useful for your tests, since we want to
check sanity for values from the guest, not from human via QMP.
So FWIW overall LGTM.

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