On Wed, 3 Jun 2026 09:34:19 +0800 Junjie Cao <[email protected]> wrote:
> HDM decoder programming and CDAT table ingestion are two > debugging-prone paths in the CXL stack: the former drives > address-decoding behaviour, the latter is parsed before any guest > interaction so wrong inputs surface late. > > Add four trace points: > > * cxl_hdm_decoder_write - guest write to a decoder CTRL register, > capturing the raw value and whether a > commit is being requested > * cxl_hdm_decoder_commit - actual commit, with the resolved base, > size, IW and IG fields > * cxl_cdat_init - CDAT initialisation, distinguishing the > file-backed vs built-in source > * cxl_cdat_loaded - successful file load with size and > number of entries parsed The 'and' in the commit log suggests to me this is two largely unrelated things. Can we have one patch for hdm decoder actions and one for cdat actions. I suspect the hdm decoder one will at least have some fuzz due to Alireza's work. Ideally I'd like to get that in first given it's usefulness. (not the DCD case probably as that may need more review but the static fast path case). I'm not really convinced the cdat ones provide as much value as the others you have added. Perhaps provide more info on why you find that one useful - unless we have a silly bug the source should be obvious as it is commandline controlled for instance. > > The hw/mem/cxl_type3.c trace points are declared in the existing > hw/mem/trace-events. The cxl_ prefix keeps the namespace unified for > filtering with -trace 'cxl_*'. > > Signed-off-by: Junjie Cao <[email protected]> > --- > hw/cxl/cxl-cdat.c | 3 +++ > hw/cxl/trace-events | 4 ++++ > hw/mem/cxl_type3.c | 17 ++++++++++++++++- > hw/mem/trace-events | 4 ++++ > 4 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c > index 959a55518e..5274ac8c12 100644 > --- a/hw/cxl/cxl-cdat.c > +++ b/hw/cxl/cxl-cdat.c > @@ -12,6 +12,7 @@ > #include "hw/cxl/cxl.h" > #include "qapi/error.h" > #include "qemu/error-report.h" > +#include "trace.h" > > static void cdat_len_check(CDATSubHeader *hdr, Error **errp) > { > @@ -186,6 +187,7 @@ static bool ct3_load_cdat(CDATObject *cdat, Error **errp) > cdat->entry_len = num_ent; > cdat->entry = g_steal_pointer(&cdat_st); > cdat->buf = g_steal_pointer(&buf); > + trace_cxl_cdat_loaded(cdat->filename, file_size, num_ent); > return true; > } > > @@ -193,6 +195,7 @@ bool cxl_doe_cdat_init(CXLComponentState *cxl_cstate, > Error **errp) > { > CDATObject *cdat = &cxl_cstate->cdat; > > + trace_cxl_cdat_init(cdat->filename ?: "<built-in>"); > if (cdat->filename) { > return ct3_load_cdat(cdat, errp); > } else { > diff --git a/hw/cxl/trace-events b/hw/cxl/trace-events > index abca116ed3..15977dce6e 100644 > --- a/hw/cxl/trace-events > +++ b/hw/cxl/trace-events > @@ -6,3 +6,7 @@ > 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" > + > +# cxl-cdat.c > +cxl_cdat_init(const char *source) "source=%s" > +cxl_cdat_loaded(const char *filename, size_t length, int num_entries) > "filename=%s length=%zu entries=%d" > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c > index 4739239da3..8130abeb37 100644 > --- a/hw/mem/cxl_type3.c > +++ b/hw/mem/cxl_type3.c > @@ -30,6 +30,7 @@ > #include "system/numa.h" > #include "hw/cxl/cxl.h" > #include "hw/pci/msix.h" > +#include "trace.h" > > /* type3 device private */ > enum CXL_T3_MSIX_VECTOR { > @@ -419,9 +420,20 @@ static void hdm_decoder_commit(CXLType3Dev *ct3d, int > which) > int hdm_inc = R_CXL_HDM_DECODER1_BASE_LO - R_CXL_HDM_DECODER0_BASE_LO; > ComponentRegisters *cregs = &ct3d->cxl_cstate.crb; > uint32_t *cache_mem = cregs->cache_mem_registers; > - uint32_t ctrl; > + uint32_t ctrl, low, high; > + uint64_t base, size; > > ctrl = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_CTRL + which * hdm_inc); > + low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_LO + which * hdm_inc); > + high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_BASE_HI + which * > hdm_inc); > + base = (low & 0xf0000000) | ((uint64_t)high << 32); > + low = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_LO + which * hdm_inc); > + high = ldl_le_p(cache_mem + R_CXL_HDM_DECODER0_SIZE_HI + which * > hdm_inc); > + size = (low & 0xf0000000) | ((uint64_t)high << 32); > + trace_cxl_hdm_decoder_commit(which, base, size, > + FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, IW), > + FIELD_EX32(ctrl, CXL_HDM_DECODER0_CTRL, > IG)); > + > /* TODO: Sanity checks that the decoder is possible */ > ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, ERR, 0); > ctrl = FIELD_DP32(ctrl, CXL_HDM_DECODER0_CTRL, COMMITTED, 1); > @@ -623,6 +635,9 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, > uint64_t value, > } > > stl_le_p((uint8_t *)cache_mem + offset, value); > + if (which_hdm >= 0) { > + trace_cxl_hdm_decoder_write(which_hdm, value, should_commit); > + } > if (should_commit) { > hdm_decoder_commit(ct3d, which_hdm); > } else if (should_uncommit) { > diff --git a/hw/mem/trace-events b/hw/mem/trace-events > index 8b6b02b5bf..770831f701 100644 > --- a/hw/mem/trace-events > +++ b/hw/mem/trace-events > @@ -6,3 +6,7 @@ mhp_pc_dimm_assigned_slot(int slot) "%d" > memory_device_pre_plug(const char *id, uint64_t addr) "id=%s addr=0x%"PRIx64 > memory_device_plug(const char *id, uint64_t addr) "id=%s addr=0x%"PRIx64 > memory_device_unplug(const char *id, uint64_t addr) "id=%s addr=0x%"PRIx64 > + > +# cxl_type3.c > +cxl_hdm_decoder_write(int which, uint32_t value, bool commit) "decoder=%d > ctrl=0x%08x commit=%d" > +cxl_hdm_decoder_commit(int which, uint64_t base, uint64_t size, uint32_t > iw_enc, uint32_t ig_enc) "decoder=%d base=0x%"PRIx64" size=0x%"PRIx64" > iw_enc=0x%x ig_enc=0x%x"
