[PATCH 0/2] hw/cxl: Add tracing for component I/O region

[PATCH 0/2] hw/cxl: Add tracing for component I/O region

[Philippe Mathieu-Daudé]

Allow tracing component I/O accesses,
remove unused io_registers[] array.

Philippe Mathieu-Daudé (2):
  hw/cxl: Remove unused component_registers::io_registers[] array
  hw/cxl: Allow tracing component I/O accesses

 include/hw/cxl/cxl_component.h |  4 ++--
 hw/cxl/cxl-component-utils.c   | 14 +++++++++++---
 hw/cxl/Kconfig                 |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.47.1

[PATCH 1/2] hw/cxl: Remove unused component_registers::io_registers[] array

[Philippe Mathieu-Daudé]

Avoid wasting 4K for each component, remove unused io_registers[].

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/cxl/cxl_component.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 945ee6ffd04..ac61c3f33a5 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -218,7 +218,6 @@ typedef struct component_registers {
      *   0xe000 - 0xe3ff CXL ARB/MUX registers
      *   0xe400 - 0xffff RSVD
      */
-    uint32_t io_registers[CXL2_COMPONENT_IO_REGION_SIZE >> 2];
     MemoryRegion io;
 
     uint32_t cache_mem_registers[CXL2_COMPONENT_CM_REGION_SIZE >> 2];
-- 
2.47.1

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

[Philippe Mathieu-Daudé]

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(-)

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
-- 
2.47.1

Re: [PATCH 1/2] hw/cxl: Remove unused component_registers::io_registers[] array

[Jonathan Cameron via]

On Wed, 22 Jan 2025 07:56:23 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> Avoid wasting 4K for each component, remove unused io_registers[].
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

These may need to come back for correctness if anyone starts
actually accessing them in the OS, but for now this is fine.
We'd have to deal with filling them with plausible values anyway
if that becomes an issue.

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

I can pick these up and send on in a set to mst, or mst pick up
directly, or got through another tree. Don't mind!
> ---
>  include/hw/cxl/cxl_component.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 945ee6ffd04..ac61c3f33a5 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -218,7 +218,6 @@ typedef struct component_registers {
>       *   0xe000 - 0xe3ff CXL ARB/MUX registers
>       *   0xe400 - 0xffff RSVD
>       */
> -    uint32_t io_registers[CXL2_COMPONENT_IO_REGION_SIZE >> 2];
>      MemoryRegion io;
>  
>      uint32_t cache_mem_registers[CXL2_COMPONENT_CM_REGION_SIZE >> 2];

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

[Jonathan Cameron via]

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

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

[Jonathan Cameron via]

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  
> 
> 

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

[Philippe Mathieu-Daudé]

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(-)

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

[Jonathan Cameron via]

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(-)  
>