On Fri, 19 May 2023 17:57:31 +0200
Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
> On 19/5/23 17:45, Jonathan Cameron wrote:
> > On Fri, 19 May 2023 17:34:20 +0200
> > Philippe Mathieu-Daudé <phi...@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.we...@intel.com>
> >>> Signed-off-by: Jonathan Cameron <jonathan.came...@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.
This got changed to dpa to avoid some confusion with other uses of
address and DPA is tightly defined in the CXL specification.
> >
> >>
> >>> +# @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
>
Got it. That doesn't work unfortunately because there may well be a bunch
of interleaving and hashing inbetween.
> 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.
Great
Jonathan
>
> >
> >
> >>
> >>> +# @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:
> >>> #
> >>
> >
>
