Re: [PATCH 2/2] hw/cxl: Allow tracing component I/O accesses

[Jonathan Cameron]

On Thu, 23 Jan 2025 09:51:51 +0000
Jonathan Cameron via <qemu-devel@nongnu.org> wrote:

> On Wed, 22 Jan 2025 07:56:24 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> > Map the component I/O region as UnimplementedDevice
> > to be able to trace guest I/O accesses with '-d unimp'.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>  
> I'm not that familiar with this infrastructure but seems
> fine to me.
> 
> I'd definitely be curious if anything is touching this space so
> tracing may be helpful for that!
Hi Philippe

> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Retract that.  

Can't instantiate a CXL device with this patch because:

Device unimplemented-device can not be dynamically instantiated.

Reverting this patch on my tree fixes that.

> > ---
> >  include/hw/cxl/cxl_component.h |  3 ++-
> >  hw/cxl/cxl-component-utils.c   | 14 +++++++++++---
> >  hw/cxl/Kconfig                 |  1 +
> >  3 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> > index ac61c3f33a5..54fd369a838 100644
> > --- a/include/hw/cxl/cxl_component.h
> > +++ b/include/hw/cxl/cxl_component.h
> > @@ -18,6 +18,7 @@
> >  #include "qemu/range.h"
> >  #include "hw/cxl/cxl_cdat.h"
> >  #include "hw/register.h"
> > +#include "hw/misc/unimp.h"
> >  #include "qapi/error.h"
> >  
> >  enum reg_type {
> > @@ -218,7 +219,7 @@ typedef struct component_registers {
> >       *   0xe000 - 0xe3ff CXL ARB/MUX registers
> >       *   0xe400 - 0xffff RSVD
> >       */
> > -    MemoryRegion io;
> > +    UnimplementedDeviceState io;
> >  
> >      uint32_t cache_mem_registers[CXL2_COMPONENT_CM_REGION_SIZE >> 2];
> >      uint32_t cache_mem_regs_write_mask[CXL2_COMPONENT_CM_REGION_SIZE >> 2];
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index cd116c04012..6d593475d00 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -192,17 +192,25 @@ void cxl_component_register_block_init(Object *obj,
> >                                         const char *type)
> >  {
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > +    DeviceState *io_dev;
> > +    SysBusDevice *io_sbd;
> >  
> >      memory_region_init(&cregs->component_registers, obj, type,
> >                         CXL2_COMPONENT_BLOCK_SIZE);
> >  
> >      /* io registers controls link which we don't care about in QEMU */
> > -    memory_region_init_io(&cregs->io, obj, NULL, NULL, ".io",
> > -                          CXL2_COMPONENT_IO_REGION_SIZE);
> > +    object_initialize_child(obj, "io", &cregs->io, 
> > TYPE_UNIMPLEMENTED_DEVICE);
> > +    io_dev = DEVICE(&cregs->io);
> > +    io_sbd = SYS_BUS_DEVICE(&cregs->io);
> > +    qdev_prop_set_string(io_dev, "name", ".io");
> > +    qdev_prop_set_uint64(io_dev, "size", CXL2_COMPONENT_IO_REGION_SIZE);
> > +    sysbus_realize(io_sbd, &error_fatal);
> > +
> >      memory_region_init_io(&cregs->cache_mem, obj, &cache_mem_ops, 
> > cxl_cstate,
> >                            ".cache_mem", CXL2_COMPONENT_CM_REGION_SIZE);
> >  
> > -    memory_region_add_subregion(&cregs->component_registers, 0, 
> > &cregs->io);
> > +    memory_region_add_subregion(&cregs->component_registers, 0,
> > +                                sysbus_mmio_get_region(io_sbd, 0));
> >      memory_region_add_subregion(&cregs->component_registers,
> >                                  CXL2_COMPONENT_IO_REGION_SIZE,
> >                                  &cregs->cache_mem);
> > diff --git a/hw/cxl/Kconfig b/hw/cxl/Kconfig
> > index 8e67519b161..d6c7536001e 100644
> > --- a/hw/cxl/Kconfig
> > +++ b/hw/cxl/Kconfig
> > @@ -1,3 +1,4 @@
> >  config CXL
> >      bool
> >      default y if PCI_EXPRESS
> > +    select UNIMP  
> 
>