| Message ID | 20250122065624.34203-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/cxl: Add tracing for component I/O region | expand |
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! Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > 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
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 > >
On 24/1/25 17:20, Jonathan Cameron wrote: > 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. Hmm the qtests using CXL devices pass, how do you trigger that? > > 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(-)
On Fri, 24 Jan 2025 17:28:02 +0100 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 24/1/25 17:20, Jonathan Cameron wrote: > > 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. > > Hmm the qtests using CXL devices pass, how do you trigger that? Seems oddly enough to be arm64 only (well not x86 - there is no support for other arches than those two) which isn't upstream yet hence no tests upstream - I'll get back to that sometime soonish as the blocker of dt-bindings for PXBs may be less of an issue than it was. source of that print seems to be hw/core/sysbus-fdt.c add_fdt_node(). Seems it is looking for sysbus devices and gets confused when it finds this one. I'm not sure if it is in /peripheral or /peripheral-anon. For now you could try my gitlab tree gitlab.com/jic23/qemu.git cxl-* whatever has latest date. Or shout if you want me to try anything. Jonathan > > > > > 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
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> --- 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(-)