Re: [PATCH v7 2/4] hw/cxl: QMP based poison injection support

[Markus Armbruster]

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

> Inject poison using qmp command cxl-inject-poison to add an entry to the

QMP command

> poison list.
>
> For now, the poison is not returned CXL.mem reads, but only via the
> mailbox command Get Poison List. So a normal memory read to an address
> that is on the poison list will not yet result in a synchronous exception
> (and similar for partial cacheline writes).
> That is left for a future patch.
>
> See CXL rev 3.0, sec 8.2.9.8.4.1 Get Poison list (Opcode 4300h)
>
> Kernel patches to use this interface here:
> https://lore.kernel.org/linux-cxl/cover.1665606782.git.alison.schofield@intel.com/
>
> To inject poison using qmp (telnet to the qmp port)
> { "execute": "qmp_capabilities" }
>
> { "execute": "cxl-inject-poison",
>     "arguments": {
>          "path": "/machine/peripheral/cxl-pmem0",
>          "start": 2048,
>          "length": 256
>     }
> }
>
> Adjusted to select a device on your machine.
>
> Note that the poison list supported is kept short enough to avoid the
> complexity of state machine that is needed to handle the MORE flag.
>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Acked-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> v7:
>   Formatting changes requested by Markus Armbruster
> ---
>  qapi/cxl.json               | 21 +++++++++
>  include/hw/cxl/cxl.h        |  1 +
>  include/hw/cxl/cxl_device.h | 20 +++++++++
>  hw/cxl/cxl-mailbox-utils.c  | 90 +++++++++++++++++++++++++++++++++++++
>  hw/mem/cxl_type3.c          | 56 +++++++++++++++++++++++
>  hw/mem/cxl_type3_stubs.c    |  6 +++
>  6 files changed, 194 insertions(+)
>
> diff --git a/qapi/cxl.json b/qapi/cxl.json
> index b21c9b4c1c..4849fca776 100644
> --- a/qapi/cxl.json
> +++ b/qapi/cxl.json
> @@ -5,6 +5,27 @@
>  # = CXL devices
>  ##
>  
> +##
> +# @cxl-inject-poison:
> +#
> +# Poison records indicate that a CXL memory device knows that a
> +# particular # memory region may be corrupted. This may be because of
> +# locally detected # errors (e.g. ECC failure) or poisoned writes
> +# received from other components in the system. This injection
> +# mechanism enables testing of the OS handling of poison records which
> +# may be queried via the CXL mailbox.

Delete the two stray '#' characters.

Two spaces between sentences for consistency, please.

Together:

   # Poison records indicate that a CXL memory device knows that a
   # particular memory region may be corrupted.  This may be because of
   # locally detected errors (e.g. ECC failure) or poisoned writes
   # received from other components in the system.  This injection
   # mechanism enables testing of the OS handling of poison records which
   # may be queried via the CXL mailbox.


> +#
> +# @path: CXL type 3 device canonical QOM path
> +#
> +# @start: Start address - must be 64 byte aligned.
> +#
> +# @length: Length of poison to inject - must be a multiple of 64 bytes.

The ASCII minus character will be rendered as minus, not as dash.  Ugly.
You could use Unicode dash, but I'd recommend

   # @length: Length of poison to inject; must be a multiple of 64 bytes.

> +#
> +# Since: 8.1
> +##
> +{ 'command': 'cxl-inject-poison',
> +  'data': { 'path': 'str', 'start': 'uint64', 'length': 'size' }}
> +
>  ##
>  # @CxlUncorErrorType:
>  #

With these tweaks
Acked-by: Markus Armbruster <armbru@redhat.com>

[...]