On Wed, 3 Jun 2026 09:34:18 +0800 Junjie Cao <[email protected]> wrote:
> The CXL subsystem currently has no trace coverage. Debugging mailbox > command flow requires either printf or qemu_log_mask, neither of which > is well suited for following the dispatch path that is exercised by > every guest driver probe. > > Introduce a dedicated trace-events file under hw/cxl/ and wire it into > the build via the trace_events_subdirs list in the top-level > meson.build. Add three trace points covering command entry, payload > validation, and handler completion in cxl_process_cci_message(): > > * cxl_mailbox_command - command entry (opcode, len_in) > * cxl_mailbox_invalid_payload - payload length check rejection > * cxl_mailbox_handler_return - handler completion with return code > and len_out > > The early-exit paths for unimplemented commands, in-progress background > operations, and disabled media skip handler dispatch and are therefore > not paired with cxl_mailbox_handler_return; an absent return event for > a given cxl_mailbox_command identifies these cases in the trace stream. > > Trace points use the cxl_ prefix so future patches in this subsystem > share a single namespace, allowing -trace 'cxl_*' to match all of them. > > Signed-off-by: Junjie Cao <[email protected]> Seems likely to be a useful addition. My main request is to make sure all possible places we might exit due to a given condition are covered rather than just the simplest one (for length checks that is!) Jonathan > --- > hw/cxl/cxl-mailbox-utils.c | 5 +++++ > hw/cxl/trace-events | 8 ++++++++ > hw/cxl/trace.h | 4 ++++ > meson.build | 1 + > 4 files changed, 18 insertions(+) > create mode 100644 hw/cxl/trace-events > create mode 100644 hw/cxl/trace.h > > diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c > index d8ba7e8625..521b8c6334 100644 > --- a/hw/cxl/cxl-mailbox-utils.c > +++ b/hw/cxl/cxl-mailbox-utils.c > @@ -27,6 +27,7 @@ > #include "system/hostmem.h" > #include "qemu/range.h" > #include "qapi/qapi-types-cxl.h" > +#include "trace.h" > > #define CXL_CAPACITY_MULTIPLIER (256 * MiB) > #define CXL_DC_EVENT_LOG_SIZE 8 > @@ -4580,6 +4581,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, > uint8_t cmd, > CXLDeviceState *cxl_dstate; > > *len_out = 0; > + trace_cxl_mailbox_command((set << 8) | cmd, len_in); Given the opcode is always made up of those two smaller fields, it might be nicer to just pass them as separate parameters? Potentially combine them into the single print in the tracepoint, or do it as set:cmd e.g 0x01:03 or something like that? > cxl_cmd = &cci->cxl_cmd_set[set][cmd]; > h = cxl_cmd->handler; > if (!h) { > @@ -4589,6 +4591,8 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, > uint8_t cmd, > } > > if (len_in != cxl_cmd->in && cxl_cmd->in != ~0) { > + trace_cxl_mailbox_invalid_payload((set << 8) | cmd, len_in, > + cxl_cmd->in); > return CXL_MBOX_INVALID_PAYLOAD_LENGTH; This is one of a lot of places that we fail with that error code - because many commands have variable lengths. I'd want to see this on all of the places where we end up bailing out with that error code. > > @@ -4642,6 +4646,7 @@ int cxl_process_cci_message(CXLCCI *cci, uint8_t set, > uint8_t cmd, > timer_mod(cci->bg.timer, now + CXL_MBOX_BG_UPDATE_FREQ); > } > > + trace_cxl_mailbox_handler_return((set << 8) | cmd, ret, *len_out); > return ret; > } > > diff --git a/hw/cxl/trace-events b/hw/cxl/trace-events > new file mode 100644 > index 0000000000..abca116ed3 > --- /dev/null > +++ b/hw/cxl/trace-events > @@ -0,0 +1,8 @@ > +# See docs/devel/tracing.rst for syntax documentation. > +# > +# SPDX-License-Identifier: GPL-2.0-or-later > + > +# cxl-mailbox-utils.c > +cxl_mailbox_command(uint16_t opcode, size_t len_in) "opcode=0x%04x > len_in=%zu" > +cxl_mailbox_invalid_payload(uint16_t opcode, size_t len_in, size_t expected) > "opcode=0x%04x len_in=%zu expected=%zu" > +cxl_mailbox_handler_return(uint16_t opcode, int ret, size_t len_out) > "opcode=0x%04x ret=%d len_out=%zu" > diff --git a/hw/cxl/trace.h b/hw/cxl/trace.h > new file mode 100644 > index 0000000000..84e79d50c0 > --- /dev/null > +++ b/hw/cxl/trace.h > @@ -0,0 +1,4 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > +#include "trace/trace-hw_cxl.h" > diff --git a/meson.build b/meson.build > index 19e123423b..c229199d5d 100644 > --- a/meson.build > +++ b/meson.build > @@ -3596,6 +3596,7 @@ if have_system > 'hw/audio', > 'hw/block', > 'hw/char', > + 'hw/cxl', > 'hw/display', > 'hw/dma', > 'hw/fsi',
