[PATCH v4 0/4] hw/cxl: Line length reduction and related

[PATCH v4 0/4] hw/cxl: Line length reduction and related

[Jonathan Cameron via]

No dependencies. Does not overlap with the other CXL series
[PATCH v5 0/3] hw/cxl: Add dummy ACPI QTG DSM
so either order is fine, or they can go via different paths.

v4: Use QEMU_BUILD_BUG_ON() rather than static_assert() with
    missing message. Thanks to Michael Tsirkin who caught this in
    a clang build failure.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>

Michael observed that the CXL code regularly went above the 80 character
recommendation and in many cases this was not necessary for readability.

This series is focused on tidying this up for the existing code so that
we can maintain the preferred formatting going forwards.

Jonathan Cameron (4):
  hw/cxl: Use a switch to explicitly check size in caps_reg_read()
  hw/cxl: Use switch statements for read and write of cachemem registers
  hw/cxl: CXLDVSECPortExtensions renamed to CXLDVSECPortExt
  hw/cxl: Line length reductions

 include/hw/cxl/cxl_component.h |   3 +-
 include/hw/cxl/cxl_device.h    |   5 +-
 include/hw/cxl/cxl_events.h    |   3 +-
 include/hw/cxl/cxl_pci.h       |   6 +-
 hw/cxl/cxl-cdat.c              |   3 +-
 hw/cxl/cxl-component-utils.c   | 128 ++++++++++++++++++++-------------
 hw/cxl/cxl-device-utils.c      |  11 +--
 hw/cxl/cxl-events.c            |   9 ++-
 hw/cxl/cxl-mailbox-utils.c     |  21 ++++--
 hw/mem/cxl_type3.c             |  31 ++++----
 hw/mem/cxl_type3_stubs.c       |   5 +-
 hw/pci-bridge/cxl_downstream.c |   2 +-
 hw/pci-bridge/cxl_root_port.c  |   2 +-
 hw/pci-bridge/cxl_upstream.c   |   2 +-
 14 files changed, 143 insertions(+), 88 deletions(-)

-- 
2.39.2

[PATCH v4 0/4] hw/cxl: Line length reduction and related

[Jonathan Cameron]

No dependencies. Does not overlap with the other CXL series
[PATCH v5 0/3] hw/cxl: Add dummy ACPI QTG DSM
so either order is fine, or they can go via different paths.

v4: Use QEMU_BUILD_BUG_ON() rather than static_assert() with
    missing message. Thanks to Michael Tsirkin who caught this in
    a clang build failure.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>

Michael observed that the CXL code regularly went above the 80 character
recommendation and in many cases this was not necessary for readability.

This series is focused on tidying this up for the existing code so that
we can maintain the preferred formatting going forwards.

Jonathan Cameron (4):
  hw/cxl: Use a switch to explicitly check size in caps_reg_read()
  hw/cxl: Use switch statements for read and write of cachemem registers
  hw/cxl: CXLDVSECPortExtensions renamed to CXLDVSECPortExt
  hw/cxl: Line length reductions

 include/hw/cxl/cxl_component.h |   3 +-
 include/hw/cxl/cxl_device.h    |   5 +-
 include/hw/cxl/cxl_events.h    |   3 +-
 include/hw/cxl/cxl_pci.h       |   6 +-
 hw/cxl/cxl-cdat.c              |   3 +-
 hw/cxl/cxl-component-utils.c   | 128 ++++++++++++++++++++-------------
 hw/cxl/cxl-device-utils.c      |  11 +--
 hw/cxl/cxl-events.c            |   9 ++-
 hw/cxl/cxl-mailbox-utils.c     |  21 ++++--
 hw/mem/cxl_type3.c             |  31 ++++----
 hw/mem/cxl_type3_stubs.c       |   5 +-
 hw/pci-bridge/cxl_downstream.c |   2 +-
 hw/pci-bridge/cxl_root_port.c  |   2 +-
 hw/pci-bridge/cxl_upstream.c   |   2 +-
 14 files changed, 143 insertions(+), 88 deletions(-)

-- 
2.39.2

[PATCH v4 1/4] hw/cxl: Use a switch to explicitly check size in caps_reg_read()

[Jonathan Cameron via]

Bring this read function inline with the others that do
check for unexpected size values.

Also reduces line lengths to sub 80 chars.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
---
 hw/cxl/cxl-device-utils.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index bd68328032..eb7195272e 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -32,10 +32,13 @@ static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
 {
     CXLDeviceState *cxl_dstate = opaque;
 
-    if (size == 4) {
-        return cxl_dstate->caps_reg_state32[offset / sizeof(*cxl_dstate->caps_reg_state32)];
-    } else {
-        return cxl_dstate->caps_reg_state64[offset / sizeof(*cxl_dstate->caps_reg_state64)];
+    switch (size) {
+    case 4:
+        return cxl_dstate->caps_reg_state32[offset / size];
+    case 8:
+        return cxl_dstate->caps_reg_state64[offset / size];
+    default:
+        g_assert_not_reached();
     }
 }
 
-- 
2.39.2

[PATCH v4 1/4] hw/cxl: Use a switch to explicitly check size in caps_reg_read()

[Jonathan Cameron]

Bring this read function inline with the others that do
check for unexpected size values.

Also reduces line lengths to sub 80 chars.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
---
 hw/cxl/cxl-device-utils.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/cxl/cxl-device-utils.c b/hw/cxl/cxl-device-utils.c
index bd68328032..eb7195272e 100644
--- a/hw/cxl/cxl-device-utils.c
+++ b/hw/cxl/cxl-device-utils.c
@@ -32,10 +32,13 @@ static uint64_t caps_reg_read(void *opaque, hwaddr offset, unsigned size)
 {
     CXLDeviceState *cxl_dstate = opaque;
 
-    if (size == 4) {
-        return cxl_dstate->caps_reg_state32[offset / sizeof(*cxl_dstate->caps_reg_state32)];
-    } else {
-        return cxl_dstate->caps_reg_state64[offset / sizeof(*cxl_dstate->caps_reg_state64)];
+    switch (size) {
+    case 4:
+        return cxl_dstate->caps_reg_state32[offset / size];
+    case 8:
+        return cxl_dstate->caps_reg_state64[offset / size];
+    default:
+        g_assert_not_reached();
     }
 }
 
-- 
2.39.2

[PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

Establishing that only register accesses of size 4 and 8 can occur
using these functions requires looking at their callers. Make it
easier to see that by using switch statements.
Assertions are used to enforce that the register storage is of the
matching size, allowing fixed values to be used for divisors of
the array indices.

Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>

---
v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
    error message.

 hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index f3bbf0fd13..d1997a52e6 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
     CXLComponentState *cxl_cstate = opaque;
     ComponentRegisters *cregs = &cxl_cstate->crb;
 
-    if (size == 8) {
+    switch (size) {
+    case 4:
+        if (cregs->special_ops && cregs->special_ops->read) {
+            return cregs->special_ops->read(cxl_cstate, offset, 4);
+        } else {
+            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
+            return cregs->cache_mem_registers[offset / 4];
+        }
+    case 8:
         qemu_log_mask(LOG_UNIMP,
                       "CXL 8 byte cache mem registers not implemented\n");
         return 0;
-    }
-
-    if (cregs->special_ops && cregs->special_ops->read) {
-        return cregs->special_ops->read(cxl_cstate, offset, size);
-    } else {
-        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
+    default:
+        /*
+         * In line with specifiction limitaions on access sizes, this
+         * routine is not called with other sizes.
+         */
+        g_assert_not_reached();
     }
 }
 
@@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
     ComponentRegisters *cregs = &cxl_cstate->crb;
     uint32_t mask;
 
-    if (size == 8) {
+    switch (size) {
+    case 4:
+        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
+        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
+        mask = cregs->cache_mem_regs_write_mask[offset / 4];
+        value &= mask;
+        /* RO bits should remain constant. Done by reading existing value */
+        value |= ~mask & cregs->cache_mem_registers[offset / 4];
+        if (cregs->special_ops && cregs->special_ops->write) {
+            cregs->special_ops->write(cxl_cstate, offset, value, size);
+            return;
+        }
+
+        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
+            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
+            dumb_hdm_handler(cxl_cstate, offset, value);
+        } else {
+            cregs->cache_mem_registers[offset / 4] = value;
+        }
+        return;
+    case 8:
         qemu_log_mask(LOG_UNIMP,
                       "CXL 8 byte cache mem registers not implemented\n");
         return;
-    }
-    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
-    value &= mask;
-    /* RO bits should remain constant. Done by reading existing value */
-    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
-    if (cregs->special_ops && cregs->special_ops->write) {
-        cregs->special_ops->write(cxl_cstate, offset, value, size);
-        return;
-    }
-
-    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
-        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
-        dumb_hdm_handler(cxl_cstate, offset, value);
-    } else {
-        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
+    default:
+        /*
+         * In line with specifiction limitaions on access sizes, this
+         * routine is not called with other sizes.
+         */
+        g_assert_not_reached();
     }
 }
 
-- 
2.39.2

[PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

Establishing that only register accesses of size 4 and 8 can occur
using these functions requires looking at their callers. Make it
easier to see that by using switch statements.
Assertions are used to enforce that the register storage is of the
matching size, allowing fixed values to be used for divisors of
the array indices.

Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Fan Ni <fan.ni@samsung.com>

---
v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
    error message.

 hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 23 deletions(-)

diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index f3bbf0fd13..d1997a52e6 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
     CXLComponentState *cxl_cstate = opaque;
     ComponentRegisters *cregs = &cxl_cstate->crb;
 
-    if (size == 8) {
+    switch (size) {
+    case 4:
+        if (cregs->special_ops && cregs->special_ops->read) {
+            return cregs->special_ops->read(cxl_cstate, offset, 4);
+        } else {
+            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
+            return cregs->cache_mem_registers[offset / 4];
+        }
+    case 8:
         qemu_log_mask(LOG_UNIMP,
                       "CXL 8 byte cache mem registers not implemented\n");
         return 0;
-    }
-
-    if (cregs->special_ops && cregs->special_ops->read) {
-        return cregs->special_ops->read(cxl_cstate, offset, size);
-    } else {
-        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
+    default:
+        /*
+         * In line with specifiction limitaions on access sizes, this
+         * routine is not called with other sizes.
+         */
+        g_assert_not_reached();
     }
 }
 
@@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
     ComponentRegisters *cregs = &cxl_cstate->crb;
     uint32_t mask;
 
-    if (size == 8) {
+    switch (size) {
+    case 4:
+        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
+        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
+        mask = cregs->cache_mem_regs_write_mask[offset / 4];
+        value &= mask;
+        /* RO bits should remain constant. Done by reading existing value */
+        value |= ~mask & cregs->cache_mem_registers[offset / 4];
+        if (cregs->special_ops && cregs->special_ops->write) {
+            cregs->special_ops->write(cxl_cstate, offset, value, size);
+            return;
+        }
+
+        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
+            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
+            dumb_hdm_handler(cxl_cstate, offset, value);
+        } else {
+            cregs->cache_mem_registers[offset / 4] = value;
+        }
+        return;
+    case 8:
         qemu_log_mask(LOG_UNIMP,
                       "CXL 8 byte cache mem registers not implemented\n");
         return;
-    }
-    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
-    value &= mask;
-    /* RO bits should remain constant. Done by reading existing value */
-    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
-    if (cregs->special_ops && cregs->special_ops->write) {
-        cregs->special_ops->write(cxl_cstate, offset, value, size);
-        return;
-    }
-
-    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
-        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
-        dumb_hdm_handler(cxl_cstate, offset, value);
-    } else {
-        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
+    default:
+        /*
+         * In line with specifiction limitaions on access sizes, this
+         * routine is not called with other sizes.
+         */
+        g_assert_not_reached();
     }
 }
 
-- 
2.39.2

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Michael S. Tsirkin]

On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:
> Establishing that only register accesses of size 4 and 8 can occur
> using these functions requires looking at their callers. Make it
> easier to see that by using switch statements.
> Assertions are used to enforce that the register storage is of the
> matching size, allowing fixed values to be used for divisors of
> the array indices.
> 
> Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Fan Ni <fan.ni@samsung.com>

Fails with clang:

../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
        ^
../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
#define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
                             ^
../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
#define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
                                   ^
1 error generated.

See e.g.

https://gitlab.com/mstredhat/qemu/-/jobs/5320879531



> ---
> v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
>     error message.
> 
>  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
>  1 file changed, 42 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index f3bbf0fd13..d1997a52e6 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
>      CXLComponentState *cxl_cstate = opaque;
>      ComponentRegisters *cregs = &cxl_cstate->crb;
>  
> -    if (size == 8) {
> +    switch (size) {
> +    case 4:
> +        if (cregs->special_ops && cregs->special_ops->read) {
> +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> +        } else {
> +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> +            return cregs->cache_mem_registers[offset / 4];
> +        }
> +    case 8:
>          qemu_log_mask(LOG_UNIMP,
>                        "CXL 8 byte cache mem registers not implemented\n");
>          return 0;
> -    }
> -
> -    if (cregs->special_ops && cregs->special_ops->read) {
> -        return cregs->special_ops->read(cxl_cstate, offset, size);
> -    } else {
> -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> +    default:
> +        /*
> +         * In line with specifiction limitaions on access sizes, this
> +         * routine is not called with other sizes.
> +         */
> +        g_assert_not_reached();
>      }
>  }
>  
> @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
>      ComponentRegisters *cregs = &cxl_cstate->crb;
>      uint32_t mask;
>  
> -    if (size == 8) {
> +    switch (size) {
> +    case 4:
> +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> +        value &= mask;
> +        /* RO bits should remain constant. Done by reading existing value */
> +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> +        if (cregs->special_ops && cregs->special_ops->write) {
> +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> +            return;
> +        }
> +
> +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> +            dumb_hdm_handler(cxl_cstate, offset, value);
> +        } else {
> +            cregs->cache_mem_registers[offset / 4] = value;
> +        }
> +        return;
> +    case 8:
>          qemu_log_mask(LOG_UNIMP,
>                        "CXL 8 byte cache mem registers not implemented\n");
>          return;
> -    }
> -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> -    value &= mask;
> -    /* RO bits should remain constant. Done by reading existing value */
> -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> -    if (cregs->special_ops && cregs->special_ops->write) {
> -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> -        return;
> -    }
> -
> -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> -        dumb_hdm_handler(cxl_cstate, offset, value);
> -    } else {
> -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> +    default:
> +        /*
> +         * In line with specifiction limitaions on access sizes, this
> +         * routine is not called with other sizes.
> +         */
> +        g_assert_not_reached();
>      }
>  }
>  
> -- 
> 2.39.2

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

On Wed, 18 Oct 2023 10:40:45 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:
> > Establishing that only register accesses of size 4 and 8 can occur
> > using these functions requires looking at their callers. Make it
> > easier to see that by using switch statements.
> > Assertions are used to enforce that the register storage is of the
> > matching size, allowing fixed values to be used for divisors of
> > the array indices.
> > 
> > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Reviewed-by: Fan Ni <fan.ni@samsung.com>  
> 
> Fails with clang:
> 
> ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
>         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
>         ^
> ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
>                              ^
> ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
>                                    ^
> 1 error generated.
> 
> See e.g.
> 
> https://gitlab.com/mstredhat/qemu/-/jobs/5320879531

Thanks. Will see if I can figure out what is going wrong.
This code seems cursed.  I need to figure out a way to run the CI again.
Ran into credit problems I couldn't solve last time I tried.

In meantime I can replicate this locally but not immediately obvious to me
why it is failing.

Jonathan

> 
> 
> 
> > ---
> > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> >     error message.
> > 
> >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> >  1 file changed, 42 insertions(+), 23 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index f3bbf0fd13..d1997a52e6 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> >      CXLComponentState *cxl_cstate = opaque;
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> >  
> > -    if (size == 8) {
> > +    switch (size) {
> > +    case 4:
> > +        if (cregs->special_ops && cregs->special_ops->read) {
> > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > +        } else {
> > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > +            return cregs->cache_mem_registers[offset / 4];
> > +        }
> > +    case 8:
> >          qemu_log_mask(LOG_UNIMP,
> >                        "CXL 8 byte cache mem registers not implemented\n");
> >          return 0;
> > -    }
> > -
> > -    if (cregs->special_ops && cregs->special_ops->read) {
> > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > -    } else {
> > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > +    default:
> > +        /*
> > +         * In line with specifiction limitaions on access sizes, this
> > +         * routine is not called with other sizes.
> > +         */
> > +        g_assert_not_reached();
> >      }
> >  }
> >  
> > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> >      uint32_t mask;
> >  
> > -    if (size == 8) {
> > +    switch (size) {
> > +    case 4:
> > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > +        value &= mask;
> > +        /* RO bits should remain constant. Done by reading existing value */
> > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > +        if (cregs->special_ops && cregs->special_ops->write) {
> > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > +            return;
> > +        }
> > +
> > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > +        } else {
> > +            cregs->cache_mem_registers[offset / 4] = value;
> > +        }
> > +        return;
> > +    case 8:
> >          qemu_log_mask(LOG_UNIMP,
> >                        "CXL 8 byte cache mem registers not implemented\n");
> >          return;
> > -    }
> > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > -    value &= mask;
> > -    /* RO bits should remain constant. Done by reading existing value */
> > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > -    if (cregs->special_ops && cregs->special_ops->write) {
> > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > -        return;
> > -    }
> > -
> > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > -    } else {
> > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > +    default:
> > +        /*
> > +         * In line with specifiction limitaions on access sizes, this
> > +         * routine is not called with other sizes.
> > +         */
> > +        g_assert_not_reached();
> >      }
> >  }
> >  
> > -- 
> > 2.39.2  
> 
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

On Wed, 18 Oct 2023 10:40:45 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:
> > Establishing that only register accesses of size 4 and 8 can occur
> > using these functions requires looking at their callers. Make it
> > easier to see that by using switch statements.
> > Assertions are used to enforce that the register storage is of the
> > matching size, allowing fixed values to be used for divisors of
> > the array indices.
> > 
> > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Reviewed-by: Fan Ni <fan.ni@samsung.com>  
> 
> Fails with clang:
> 
> ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
>         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
>         ^
> ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
>                              ^
> ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
>                                    ^
> 1 error generated.
> 
> See e.g.
> 
> https://gitlab.com/mstredhat/qemu/-/jobs/5320879531

Thanks. Will see if I can figure out what is going wrong.
This code seems cursed.  I need to figure out a way to run the CI again.
Ran into credit problems I couldn't solve last time I tried.

In meantime I can replicate this locally but not immediately obvious to me
why it is failing.

Jonathan

> 
> 
> 
> > ---
> > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> >     error message.
> > 
> >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> >  1 file changed, 42 insertions(+), 23 deletions(-)
> > 
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index f3bbf0fd13..d1997a52e6 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> >      CXLComponentState *cxl_cstate = opaque;
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> >  
> > -    if (size == 8) {
> > +    switch (size) {
> > +    case 4:
> > +        if (cregs->special_ops && cregs->special_ops->read) {
> > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > +        } else {
> > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > +            return cregs->cache_mem_registers[offset / 4];
> > +        }
> > +    case 8:
> >          qemu_log_mask(LOG_UNIMP,
> >                        "CXL 8 byte cache mem registers not implemented\n");
> >          return 0;
> > -    }
> > -
> > -    if (cregs->special_ops && cregs->special_ops->read) {
> > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > -    } else {
> > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > +    default:
> > +        /*
> > +         * In line with specifiction limitaions on access sizes, this
> > +         * routine is not called with other sizes.
> > +         */
> > +        g_assert_not_reached();
> >      }
> >  }
> >  
> > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> >      ComponentRegisters *cregs = &cxl_cstate->crb;
> >      uint32_t mask;
> >  
> > -    if (size == 8) {
> > +    switch (size) {
> > +    case 4:
> > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > +        value &= mask;
> > +        /* RO bits should remain constant. Done by reading existing value */
> > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > +        if (cregs->special_ops && cregs->special_ops->write) {
> > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > +            return;
> > +        }
> > +
> > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > +        } else {
> > +            cregs->cache_mem_registers[offset / 4] = value;
> > +        }
> > +        return;
> > +    case 8:
> >          qemu_log_mask(LOG_UNIMP,
> >                        "CXL 8 byte cache mem registers not implemented\n");
> >          return;
> > -    }
> > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > -    value &= mask;
> > -    /* RO bits should remain constant. Done by reading existing value */
> > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > -    if (cregs->special_ops && cregs->special_ops->write) {
> > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > -        return;
> > -    }
> > -
> > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > -    } else {
> > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > +    default:
> > +        /*
> > +         * In line with specifiction limitaions on access sizes, this
> > +         * routine is not called with other sizes.
> > +         */
> > +        g_assert_not_reached();
> >      }
> >  }
> >  
> > -- 
> > 2.39.2  
> 
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

On Wed, 18 Oct 2023 17:22:39 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 18 Oct 2023 10:40:45 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:  
> > > Establishing that only register accesses of size 4 and 8 can occur
> > > using these functions requires looking at their callers. Make it
> > > easier to see that by using switch statements.
> > > Assertions are used to enforce that the register storage is of the
> > > matching size, allowing fixed values to be used for divisors of
> > > the array indices.
> > > 
> > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Reviewed-by: Fan Ni <fan.ni@samsung.com>    
> > 
> > Fails with clang:
> > 
> > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> >         ^
> > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> >                              ^
> > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> >                                    ^
> > 1 error generated.
> > 
> > See e.g.
> > 
> > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531  
> 
> Thanks. Will see if I can figure out what is going wrong.
> This code seems cursed.  I need to figure out a way to run the CI again.
> Ran into credit problems I couldn't solve last time I tried.
> 
> In meantime I can replicate this locally but not immediately obvious to me
> why it is failing.

Any clang experts around?  This seems to be fixed by reducing the scope

case 4:
{
	QEMU_BUILD_BUG_ON();
}
I have no idea why though...


> 
> Jonathan
> 
> > 
> > 
> >   
> > > ---
> > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > >     error message.
> > > 
> > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > index f3bbf0fd13..d1997a52e6 100644
> > > --- a/hw/cxl/cxl-component-utils.c
> > > +++ b/hw/cxl/cxl-component-utils.c
> > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > >      CXLComponentState *cxl_cstate = opaque;
> > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > >  
> > > -    if (size == 8) {
> > > +    switch (size) {
> > > +    case 4:
> > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > +        } else {
> > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > +            return cregs->cache_mem_registers[offset / 4];
> > > +        }
> > > +    case 8:
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "CXL 8 byte cache mem registers not implemented\n");
> > >          return 0;
> > > -    }
> > > -
> > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > -    } else {
> > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > +    default:
> > > +        /*
> > > +         * In line with specifiction limitaions on access sizes, this
> > > +         * routine is not called with other sizes.
> > > +         */
> > > +        g_assert_not_reached();
> > >      }
> > >  }
> > >  
> > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > >      uint32_t mask;
> > >  
> > > -    if (size == 8) {
> > > +    switch (size) {
> > > +    case 4:
> > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > +        value &= mask;
> > > +        /* RO bits should remain constant. Done by reading existing value */
> > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > +            return;
> > > +        }
> > > +
> > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > +        } else {
> > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > +        }
> > > +        return;
> > > +    case 8:
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "CXL 8 byte cache mem registers not implemented\n");
> > >          return;
> > > -    }
> > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > -    value &= mask;
> > > -    /* RO bits should remain constant. Done by reading existing value */
> > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > -        return;
> > > -    }
> > > -
> > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > -    } else {
> > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > +    default:
> > > +        /*
> > > +         * In line with specifiction limitaions on access sizes, this
> > > +         * routine is not called with other sizes.
> > > +         */
> > > +        g_assert_not_reached();
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.39.2    
> > 
> >   
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

On Wed, 18 Oct 2023 17:22:39 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 18 Oct 2023 10:40:45 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:  
> > > Establishing that only register accesses of size 4 and 8 can occur
> > > using these functions requires looking at their callers. Make it
> > > easier to see that by using switch statements.
> > > Assertions are used to enforce that the register storage is of the
> > > matching size, allowing fixed values to be used for divisors of
> > > the array indices.
> > > 
> > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Reviewed-by: Fan Ni <fan.ni@samsung.com>    
> > 
> > Fails with clang:
> > 
> > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> >         ^
> > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> >                              ^
> > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> >                                    ^
> > 1 error generated.
> > 
> > See e.g.
> > 
> > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531  
> 
> Thanks. Will see if I can figure out what is going wrong.
> This code seems cursed.  I need to figure out a way to run the CI again.
> Ran into credit problems I couldn't solve last time I tried.
> 
> In meantime I can replicate this locally but not immediately obvious to me
> why it is failing.

Any clang experts around?  This seems to be fixed by reducing the scope

case 4:
{
	QEMU_BUILD_BUG_ON();
}
I have no idea why though...


> 
> Jonathan
> 
> > 
> > 
> >   
> > > ---
> > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > >     error message.
> > > 
> > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > index f3bbf0fd13..d1997a52e6 100644
> > > --- a/hw/cxl/cxl-component-utils.c
> > > +++ b/hw/cxl/cxl-component-utils.c
> > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > >      CXLComponentState *cxl_cstate = opaque;
> > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > >  
> > > -    if (size == 8) {
> > > +    switch (size) {
> > > +    case 4:
> > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > +        } else {
> > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > +            return cregs->cache_mem_registers[offset / 4];
> > > +        }
> > > +    case 8:
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "CXL 8 byte cache mem registers not implemented\n");
> > >          return 0;
> > > -    }
> > > -
> > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > -    } else {
> > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > +    default:
> > > +        /*
> > > +         * In line with specifiction limitaions on access sizes, this
> > > +         * routine is not called with other sizes.
> > > +         */
> > > +        g_assert_not_reached();
> > >      }
> > >  }
> > >  
> > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > >      uint32_t mask;
> > >  
> > > -    if (size == 8) {
> > > +    switch (size) {
> > > +    case 4:
> > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > +        value &= mask;
> > > +        /* RO bits should remain constant. Done by reading existing value */
> > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > +            return;
> > > +        }
> > > +
> > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > +        } else {
> > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > +        }
> > > +        return;
> > > +    case 8:
> > >          qemu_log_mask(LOG_UNIMP,
> > >                        "CXL 8 byte cache mem registers not implemented\n");
> > >          return;
> > > -    }
> > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > -    value &= mask;
> > > -    /* RO bits should remain constant. Done by reading existing value */
> > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > -        return;
> > > -    }
> > > -
> > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > -    } else {
> > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > +    default:
> > > +        /*
> > > +         * In line with specifiction limitaions on access sizes, this
> > > +         * routine is not called with other sizes.
> > > +         */
> > > +        g_assert_not_reached();
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.39.2    
> > 
> >   
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

On Wed, 18 Oct 2023 17:31:42 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 18 Oct 2023 17:22:39 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Wed, 18 Oct 2023 10:40:45 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:    
> > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > using these functions requires looking at their callers. Make it
> > > > easier to see that by using switch statements.
> > > > Assertions are used to enforce that the register storage is of the
> > > > matching size, allowing fixed values to be used for divisors of
> > > > the array indices.
> > > > 
> > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>      
> > > 
> > > Fails with clang:
> > > 
> > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > >         ^
> > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > >                              ^
> > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > >                                    ^
> > > 1 error generated.
> > > 
> > > See e.g.
> > > 
> > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531    
> > 
> > Thanks. Will see if I can figure out what is going wrong.
> > This code seems cursed.  I need to figure out a way to run the CI again.
> > Ran into credit problems I couldn't solve last time I tried.
> > 
> > In meantime I can replicate this locally but not immediately obvious to me
> > why it is failing.  
> 
> Any clang experts around?  This seems to be fixed by reducing the scope
> 
> case 4:
> {
> 	QEMU_BUILD_BUG_ON();
> }
> I have no idea why though...
> 

For anyone following along I asked a friendly compiler guy...

https://github.com/llvm/llvm-project/issues/69572

Given that we realistically need to work around this whatever the outcome
I'll add some brackets and send an update.

thanks,

Jonathan

> 
> > 
> > Jonathan
> >   
> > > 
> > > 
> > >     
> > > > ---
> > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > >     error message.
> > > > 
> > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > index f3bbf0fd13..d1997a52e6 100644
> > > > --- a/hw/cxl/cxl-component-utils.c
> > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > >      CXLComponentState *cxl_cstate = opaque;
> > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > >  
> > > > -    if (size == 8) {
> > > > +    switch (size) {
> > > > +    case 4:
> > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > +        } else {
> > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > +        }
> > > > +    case 8:
> > > >          qemu_log_mask(LOG_UNIMP,
> > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > >          return 0;
> > > > -    }
> > > > -
> > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > -    } else {
> > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > +    default:
> > > > +        /*
> > > > +         * In line with specifiction limitaions on access sizes, this
> > > > +         * routine is not called with other sizes.
> > > > +         */
> > > > +        g_assert_not_reached();
> > > >      }
> > > >  }
> > > >  
> > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > >      uint32_t mask;
> > > >  
> > > > -    if (size == 8) {
> > > > +    switch (size) {
> > > > +    case 4:
> > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > +        value &= mask;
> > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > +            return;
> > > > +        }
> > > > +
> > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > +        } else {
> > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > +        }
> > > > +        return;
> > > > +    case 8:
> > > >          qemu_log_mask(LOG_UNIMP,
> > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > >          return;
> > > > -    }
> > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > -    value &= mask;
> > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > -        return;
> > > > -    }
> > > > -
> > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > -    } else {
> > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > +    default:
> > > > +        /*
> > > > +         * In line with specifiction limitaions on access sizes, this
> > > > +         * routine is not called with other sizes.
> > > > +         */
> > > > +        g_assert_not_reached();
> > > >      }
> > > >  }
> > > >  
> > > > -- 
> > > > 2.39.2      
> > > 
> > >     
> >   
> 
> 
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

On Wed, 18 Oct 2023 17:31:42 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 18 Oct 2023 17:22:39 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Wed, 18 Oct 2023 10:40:45 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:    
> > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > using these functions requires looking at their callers. Make it
> > > > easier to see that by using switch statements.
> > > > Assertions are used to enforce that the register storage is of the
> > > > matching size, allowing fixed values to be used for divisors of
> > > > the array indices.
> > > > 
> > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>      
> > > 
> > > Fails with clang:
> > > 
> > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > >         ^
> > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > >                              ^
> > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > >                                    ^
> > > 1 error generated.
> > > 
> > > See e.g.
> > > 
> > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531    
> > 
> > Thanks. Will see if I can figure out what is going wrong.
> > This code seems cursed.  I need to figure out a way to run the CI again.
> > Ran into credit problems I couldn't solve last time I tried.
> > 
> > In meantime I can replicate this locally but not immediately obvious to me
> > why it is failing.  
> 
> Any clang experts around?  This seems to be fixed by reducing the scope
> 
> case 4:
> {
> 	QEMU_BUILD_BUG_ON();
> }
> I have no idea why though...
> 

For anyone following along I asked a friendly compiler guy...

https://github.com/llvm/llvm-project/issues/69572

Given that we realistically need to work around this whatever the outcome
I'll add some brackets and send an update.

thanks,

Jonathan

> 
> > 
> > Jonathan
> >   
> > > 
> > > 
> > >     
> > > > ---
> > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > >     error message.
> > > > 
> > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > index f3bbf0fd13..d1997a52e6 100644
> > > > --- a/hw/cxl/cxl-component-utils.c
> > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > >      CXLComponentState *cxl_cstate = opaque;
> > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > >  
> > > > -    if (size == 8) {
> > > > +    switch (size) {
> > > > +    case 4:
> > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > +        } else {
> > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > +        }
> > > > +    case 8:
> > > >          qemu_log_mask(LOG_UNIMP,
> > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > >          return 0;
> > > > -    }
> > > > -
> > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > -    } else {
> > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > +    default:
> > > > +        /*
> > > > +         * In line with specifiction limitaions on access sizes, this
> > > > +         * routine is not called with other sizes.
> > > > +         */
> > > > +        g_assert_not_reached();
> > > >      }
> > > >  }
> > > >  
> > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > >      uint32_t mask;
> > > >  
> > > > -    if (size == 8) {
> > > > +    switch (size) {
> > > > +    case 4:
> > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > +        value &= mask;
> > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > +            return;
> > > > +        }
> > > > +
> > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > +        } else {
> > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > +        }
> > > > +        return;
> > > > +    case 8:
> > > >          qemu_log_mask(LOG_UNIMP,
> > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > >          return;
> > > > -    }
> > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > -    value &= mask;
> > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > -        return;
> > > > -    }
> > > > -
> > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > -    } else {
> > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > +    default:
> > > > +        /*
> > > > +         * In line with specifiction limitaions on access sizes, this
> > > > +         * routine is not called with other sizes.
> > > > +         */
> > > > +        g_assert_not_reached();
> > > >      }
> > > >  }
> > > >  
> > > > -- 
> > > > 2.39.2      
> > > 
> > >     
> >   
> 
> 
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Michael S. Tsirkin]

On Thu, Oct 19, 2023 at 10:09:24AM +0100, Jonathan Cameron wrote:
> On Wed, 18 Oct 2023 17:31:42 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Wed, 18 Oct 2023 17:22:39 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > 
> > > On Wed, 18 Oct 2023 10:40:45 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:    
> > > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > > using these functions requires looking at their callers. Make it
> > > > > easier to see that by using switch statements.
> > > > > Assertions are used to enforce that the register storage is of the
> > > > > matching size, allowing fixed values to be used for divisors of
> > > > > the array indices.
> > > > > 
> > > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>      
> > > > 
> > > > Fails with clang:
> > > > 
> > > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > >         ^
> > > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > > >                              ^
> > > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > > >                                    ^
> > > > 1 error generated.
> > > > 
> > > > See e.g.
> > > > 
> > > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531    
> > > 
> > > Thanks. Will see if I can figure out what is going wrong.
> > > This code seems cursed.  I need to figure out a way to run the CI again.
> > > Ran into credit problems I couldn't solve last time I tried.
> > > 
> > > In meantime I can replicate this locally but not immediately obvious to me
> > > why it is failing.  
> > 
> > Any clang experts around?  This seems to be fixed by reducing the scope
> > 
> > case 4:
> > {
> > 	QEMU_BUILD_BUG_ON();
> > }
> > I have no idea why though...
> > 
> 
> For anyone following along I asked a friendly compiler guy...
> 
> https://github.com/llvm/llvm-project/issues/69572
> 
> Given that we realistically need to work around this whatever the outcome
> I'll add some brackets and send an update.
> 
> thanks,
> 
> Jonathan

We don't seem to use QEMU_BUILD_BUG_ON outside of functions - just add
brackets inside it?

> > 
> > > 
> > > Jonathan
> > >   
> > > > 
> > > > 
> > > >     
> > > > > ---
> > > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > > >     error message.
> > > > > 
> > > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > > index f3bbf0fd13..d1997a52e6 100644
> > > > > --- a/hw/cxl/cxl-component-utils.c
> > > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > > >      CXLComponentState *cxl_cstate = opaque;
> > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > >  
> > > > > -    if (size == 8) {
> > > > > +    switch (size) {
> > > > > +    case 4:
> > > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > > +        } else {
> > > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > > +        }
> > > > > +    case 8:
> > > > >          qemu_log_mask(LOG_UNIMP,
> > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > >          return 0;
> > > > > -    }
> > > > > -
> > > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > > -    } else {
> > > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > +    default:
> > > > > +        /*
> > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > +         * routine is not called with other sizes.
> > > > > +         */
> > > > > +        g_assert_not_reached();
> > > > >      }
> > > > >  }
> > > > >  
> > > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > >      uint32_t mask;
> > > > >  
> > > > > -    if (size == 8) {
> > > > > +    switch (size) {
> > > > > +    case 4:
> > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > > +        value &= mask;
> > > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > +            return;
> > > > > +        }
> > > > > +
> > > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > +        } else {
> > > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > > +        }
> > > > > +        return;
> > > > > +    case 8:
> > > > >          qemu_log_mask(LOG_UNIMP,
> > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > >          return;
> > > > > -    }
> > > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > > -    value &= mask;
> > > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > -        return;
> > > > > -    }
> > > > > -
> > > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > -    } else {
> > > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > > +    default:
> > > > > +        /*
> > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > +         * routine is not called with other sizes.
> > > > > +         */
> > > > > +        g_assert_not_reached();
> > > > >      }
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.39.2      
> > > > 
> > > >     
> > >   
> > 
> > 
> > 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

On Thu, 19 Oct 2023 05:23:04 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 19, 2023 at 10:09:24AM +0100, Jonathan Cameron wrote:
> > On Wed, 18 Oct 2023 17:31:42 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Wed, 18 Oct 2023 17:22:39 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >   
> > > > On Wed, 18 Oct 2023 10:40:45 -0400
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:      
> > > > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > > > using these functions requires looking at their callers. Make it
> > > > > > easier to see that by using switch statements.
> > > > > > Assertions are used to enforce that the register storage is of the
> > > > > > matching size, allowing fixed values to be used for divisors of
> > > > > > the array indices.
> > > > > > 
> > > > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>        
> > > > > 
> > > > > Fails with clang:
> > > > > 
> > > > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > > > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > >         ^
> > > > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > > > >                              ^
> > > > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > > > >                                    ^
> > > > > 1 error generated.
> > > > > 
> > > > > See e.g.
> > > > > 
> > > > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531      
> > > > 
> > > > Thanks. Will see if I can figure out what is going wrong.
> > > > This code seems cursed.  I need to figure out a way to run the CI again.
> > > > Ran into credit problems I couldn't solve last time I tried.
> > > > 
> > > > In meantime I can replicate this locally but not immediately obvious to me
> > > > why it is failing.    
> > > 
> > > Any clang experts around?  This seems to be fixed by reducing the scope
> > > 
> > > case 4:
> > > {
> > > 	QEMU_BUILD_BUG_ON();
> > > }
> > > I have no idea why though...
> > >   
> > 
> > For anyone following along I asked a friendly compiler guy...
> > 
> > https://github.com/llvm/llvm-project/issues/69572
> > 
> > Given that we realistically need to work around this whatever the outcome
> > I'll add some brackets and send an update.
> > 
> > thanks,
> > 
> > Jonathan  
> 
> We don't seem to use QEMU_BUILD_BUG_ON outside of functions - just add
> brackets inside it?

Makes sense.  I'll give that a go and see if anything odd happens.

Jonathan

> 
> > >   
> > > > 
> > > > Jonathan
> > > >     
> > > > > 
> > > > > 
> > > > >       
> > > > > > ---
> > > > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > > > >     error message.
> > > > > > 
> > > > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > > > index f3bbf0fd13..d1997a52e6 100644
> > > > > > --- a/hw/cxl/cxl-component-utils.c
> > > > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > > > >      CXLComponentState *cxl_cstate = opaque;
> > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > >  
> > > > > > -    if (size == 8) {
> > > > > > +    switch (size) {
> > > > > > +    case 4:
> > > > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > > > +        } else {
> > > > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > > > +        }
> > > > > > +    case 8:
> > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > >          return 0;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > > > -    } else {
> > > > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > +    default:
> > > > > > +        /*
> > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > +         * routine is not called with other sizes.
> > > > > > +         */
> > > > > > +        g_assert_not_reached();
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > >      uint32_t mask;
> > > > > >  
> > > > > > -    if (size == 8) {
> > > > > > +    switch (size) {
> > > > > > +    case 4:
> > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > > > +        value &= mask;
> > > > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > +            return;
> > > > > > +        }
> > > > > > +
> > > > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > +        } else {
> > > > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > > > +        }
> > > > > > +        return;
> > > > > > +    case 8:
> > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > >          return;
> > > > > > -    }
> > > > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > > > -    value &= mask;
> > > > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > -        return;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > -    } else {
> > > > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > > > +    default:
> > > > > > +        /*
> > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > +         * routine is not called with other sizes.
> > > > > > +         */
> > > > > > +        g_assert_not_reached();
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > -- 
> > > > > > 2.39.2        
> > > > > 
> > > > >       
> > > >     
> > > 
> > > 
> > >   
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

On Thu, 19 Oct 2023 05:23:04 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 19, 2023 at 10:09:24AM +0100, Jonathan Cameron wrote:
> > On Wed, 18 Oct 2023 17:31:42 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Wed, 18 Oct 2023 17:22:39 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >   
> > > > On Wed, 18 Oct 2023 10:40:45 -0400
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >     
> > > > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:      
> > > > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > > > using these functions requires looking at their callers. Make it
> > > > > > easier to see that by using switch statements.
> > > > > > Assertions are used to enforce that the register storage is of the
> > > > > > matching size, allowing fixed values to be used for divisors of
> > > > > > the array indices.
> > > > > > 
> > > > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>        
> > > > > 
> > > > > Fails with clang:
> > > > > 
> > > > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > > > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > >         ^
> > > > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > > > >                              ^
> > > > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > > > >                                    ^
> > > > > 1 error generated.
> > > > > 
> > > > > See e.g.
> > > > > 
> > > > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531      
> > > > 
> > > > Thanks. Will see if I can figure out what is going wrong.
> > > > This code seems cursed.  I need to figure out a way to run the CI again.
> > > > Ran into credit problems I couldn't solve last time I tried.
> > > > 
> > > > In meantime I can replicate this locally but not immediately obvious to me
> > > > why it is failing.    
> > > 
> > > Any clang experts around?  This seems to be fixed by reducing the scope
> > > 
> > > case 4:
> > > {
> > > 	QEMU_BUILD_BUG_ON();
> > > }
> > > I have no idea why though...
> > >   
> > 
> > For anyone following along I asked a friendly compiler guy...
> > 
> > https://github.com/llvm/llvm-project/issues/69572
> > 
> > Given that we realistically need to work around this whatever the outcome
> > I'll add some brackets and send an update.
> > 
> > thanks,
> > 
> > Jonathan  
> 
> We don't seem to use QEMU_BUILD_BUG_ON outside of functions - just add
> brackets inside it?

Makes sense.  I'll give that a go and see if anything odd happens.

Jonathan

> 
> > >   
> > > > 
> > > > Jonathan
> > > >     
> > > > > 
> > > > > 
> > > > >       
> > > > > > ---
> > > > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > > > >     error message.
> > > > > > 
> > > > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > > > index f3bbf0fd13..d1997a52e6 100644
> > > > > > --- a/hw/cxl/cxl-component-utils.c
> > > > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > > > >      CXLComponentState *cxl_cstate = opaque;
> > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > >  
> > > > > > -    if (size == 8) {
> > > > > > +    switch (size) {
> > > > > > +    case 4:
> > > > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > > > +        } else {
> > > > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > > > +        }
> > > > > > +    case 8:
> > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > >          return 0;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > > > -    } else {
> > > > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > +    default:
> > > > > > +        /*
> > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > +         * routine is not called with other sizes.
> > > > > > +         */
> > > > > > +        g_assert_not_reached();
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > >      uint32_t mask;
> > > > > >  
> > > > > > -    if (size == 8) {
> > > > > > +    switch (size) {
> > > > > > +    case 4:
> > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > > > +        value &= mask;
> > > > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > +            return;
> > > > > > +        }
> > > > > > +
> > > > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > +        } else {
> > > > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > > > +        }
> > > > > > +        return;
> > > > > > +    case 8:
> > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > >          return;
> > > > > > -    }
> > > > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > > > -    value &= mask;
> > > > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > -        return;
> > > > > > -    }
> > > > > > -
> > > > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > -    } else {
> > > > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > > > +    default:
> > > > > > +        /*
> > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > +         * routine is not called with other sizes.
> > > > > > +         */
> > > > > > +        g_assert_not_reached();
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > -- 
> > > > > > 2.39.2        
> > > > > 
> > > > >       
> > > >     
> > > 
> > > 
> > >   
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron via]

On Thu, 19 Oct 2023 12:24:48 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 19 Oct 2023 05:23:04 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Oct 19, 2023 at 10:09:24AM +0100, Jonathan Cameron wrote:  
> > > On Wed, 18 Oct 2023 17:31:42 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >     
> > > > On Wed, 18 Oct 2023 17:22:39 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > >     
> > > > > On Wed, 18 Oct 2023 10:40:45 -0400
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >       
> > > > > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:        
> > > > > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > > > > using these functions requires looking at their callers. Make it
> > > > > > > easier to see that by using switch statements.
> > > > > > > Assertions are used to enforce that the register storage is of the
> > > > > > > matching size, allowing fixed values to be used for divisors of
> > > > > > > the array indices.
> > > > > > > 
> > > > > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>          
> > > > > > 
> > > > > > Fails with clang:
> > > > > > 
> > > > > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > > > > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > >         ^
> > > > > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > > > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > > > > >                              ^
> > > > > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > > > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > > > > >                                    ^
> > > > > > 1 error generated.
> > > > > > 
> > > > > > See e.g.
> > > > > > 
> > > > > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531        
> > > > > 
> > > > > Thanks. Will see if I can figure out what is going wrong.
> > > > > This code seems cursed.  I need to figure out a way to run the CI again.
> > > > > Ran into credit problems I couldn't solve last time I tried.
> > > > > 
> > > > > In meantime I can replicate this locally but not immediately obvious to me
> > > > > why it is failing.      
> > > > 
> > > > Any clang experts around?  This seems to be fixed by reducing the scope
> > > > 
> > > > case 4:
> > > > {
> > > > 	QEMU_BUILD_BUG_ON();
> > > > }
> > > > I have no idea why though...
> > > >     
> > > 
> > > For anyone following along I asked a friendly compiler guy...
> > > 
> > > https://github.com/llvm/llvm-project/issues/69572
> > > 
> > > Given that we realistically need to work around this whatever the outcome
> > > I'll add some brackets and send an update.
> > > 
> > > thanks,
> > > 
> > > Jonathan    
> > 
> > We don't seem to use QEMU_BUILD_BUG_ON outside of functions - just add
> > brackets inside it?  
> 
> Makes sense.  I'll give that a go and see if anything odd happens.
It's used outside of functions such as in
include/exec/tlb-common.h

So can't add brackets to the definition. 

Back to papering over the individual cases for now.

Jonathan

> 
> Jonathan
> 
> >   
> > > >     
> > > > > 
> > > > > Jonathan
> > > > >       
> > > > > > 
> > > > > > 
> > > > > >         
> > > > > > > ---
> > > > > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > > > > >     error message.
> > > > > > > 
> > > > > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > > > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > > > > index f3bbf0fd13..d1997a52e6 100644
> > > > > > > --- a/hw/cxl/cxl-component-utils.c
> > > > > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > > > > >      CXLComponentState *cxl_cstate = opaque;
> > > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > > >  
> > > > > > > -    if (size == 8) {
> > > > > > > +    switch (size) {
> > > > > > > +    case 4:
> > > > > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > > > > +        } else {
> > > > > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > > > > +        }
> > > > > > > +    case 8:
> > > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > > >          return 0;
> > > > > > > -    }
> > > > > > > -
> > > > > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > > > > -    } else {
> > > > > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > > +    default:
> > > > > > > +        /*
> > > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > > +         * routine is not called with other sizes.
> > > > > > > +         */
> > > > > > > +        g_assert_not_reached();
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > > >      uint32_t mask;
> > > > > > >  
> > > > > > > -    if (size == 8) {
> > > > > > > +    switch (size) {
> > > > > > > +    case 4:
> > > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > > > > +        value &= mask;
> > > > > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > > +        } else {
> > > > > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > > > > +        }
> > > > > > > +        return;
> > > > > > > +    case 8:
> > > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > > >          return;
> > > > > > > -    }
> > > > > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > > > > -    value &= mask;
> > > > > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > > -        return;
> > > > > > > -    }
> > > > > > > -
> > > > > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > > -    } else {
> > > > > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > > > > +    default:
> > > > > > > +        /*
> > > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > > +         * routine is not called with other sizes.
> > > > > > > +         */
> > > > > > > +        g_assert_not_reached();
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > -- 
> > > > > > > 2.39.2          
> > > > > > 
> > > > > >         
> > > > >       
> > > > 
> > > > 
> > > >     
> >   
> 

Re: [PATCH v4 2/4] hw/cxl: Use switch statements for read and write of cachemem registers

[Jonathan Cameron]

On Thu, 19 Oct 2023 12:24:48 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 19 Oct 2023 05:23:04 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Oct 19, 2023 at 10:09:24AM +0100, Jonathan Cameron wrote:  
> > > On Wed, 18 Oct 2023 17:31:42 +0100
> > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > >     
> > > > On Wed, 18 Oct 2023 17:22:39 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > > >     
> > > > > On Wed, 18 Oct 2023 10:40:45 -0400
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >       
> > > > > > On Thu, Oct 12, 2023 at 03:05:12PM +0100, Jonathan Cameron wrote:        
> > > > > > > Establishing that only register accesses of size 4 and 8 can occur
> > > > > > > using these functions requires looking at their callers. Make it
> > > > > > > easier to see that by using switch statements.
> > > > > > > Assertions are used to enforce that the register storage is of the
> > > > > > > matching size, allowing fixed values to be used for divisors of
> > > > > > > the array indices.
> > > > > > > 
> > > > > > > Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > > Reviewed-by: Fan Ni <fan.ni@samsung.com>          
> > > > > > 
> > > > > > Fails with clang:
> > > > > > 
> > > > > > ../hw/cxl/cxl-component-utils.c:130:9: error: expected expression
> > > > > >         QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > >         ^
> > > > > > ../include/qemu/compiler.h:76:30: note: expanded from macro 'QEMU_BUILD_BUG_ON'
> > > > > > #define QEMU_BUILD_BUG_ON(x) QEMU_BUILD_BUG_MSG(x, "not expecting: " #x)
> > > > > >                              ^
> > > > > > ../include/qemu/compiler.h:74:36: note: expanded from macro 'QEMU_BUILD_BUG_MSG'
> > > > > > #define QEMU_BUILD_BUG_MSG(x, msg) _Static_assert(!(x), msg)
> > > > > >                                    ^
> > > > > > 1 error generated.
> > > > > > 
> > > > > > See e.g.
> > > > > > 
> > > > > > https://gitlab.com/mstredhat/qemu/-/jobs/5320879531        
> > > > > 
> > > > > Thanks. Will see if I can figure out what is going wrong.
> > > > > This code seems cursed.  I need to figure out a way to run the CI again.
> > > > > Ran into credit problems I couldn't solve last time I tried.
> > > > > 
> > > > > In meantime I can replicate this locally but not immediately obvious to me
> > > > > why it is failing.      
> > > > 
> > > > Any clang experts around?  This seems to be fixed by reducing the scope
> > > > 
> > > > case 4:
> > > > {
> > > > 	QEMU_BUILD_BUG_ON();
> > > > }
> > > > I have no idea why though...
> > > >     
> > > 
> > > For anyone following along I asked a friendly compiler guy...
> > > 
> > > https://github.com/llvm/llvm-project/issues/69572
> > > 
> > > Given that we realistically need to work around this whatever the outcome
> > > I'll add some brackets and send an update.
> > > 
> > > thanks,
> > > 
> > > Jonathan    
> > 
> > We don't seem to use QEMU_BUILD_BUG_ON outside of functions - just add
> > brackets inside it?  
> 
> Makes sense.  I'll give that a go and see if anything odd happens.
It's used outside of functions such as in
include/exec/tlb-common.h

So can't add brackets to the definition. 

Back to papering over the individual cases for now.

Jonathan

> 
> Jonathan
> 
> >   
> > > >     
> > > > > 
> > > > > Jonathan
> > > > >       
> > > > > > 
> > > > > > 
> > > > > >         
> > > > > > > ---
> > > > > > > v4: Use QEMU_BUILD_BUG_ON() instead of static_assert with missing
> > > > > > >     error message.
> > > > > > > 
> > > > > > >  hw/cxl/cxl-component-utils.c | 65 +++++++++++++++++++++++-------------
> > > > > > >  1 file changed, 42 insertions(+), 23 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > > > > index f3bbf0fd13..d1997a52e6 100644
> > > > > > > --- a/hw/cxl/cxl-component-utils.c
> > > > > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > > > > @@ -67,16 +67,24 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
> > > > > > >      CXLComponentState *cxl_cstate = opaque;
> > > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > > >  
> > > > > > > -    if (size == 8) {
> > > > > > > +    switch (size) {
> > > > > > > +    case 4:
> > > > > > > +        if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > > +            return cregs->special_ops->read(cxl_cstate, offset, 4);
> > > > > > > +        } else {
> > > > > > > +            QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > > +            return cregs->cache_mem_registers[offset / 4];
> > > > > > > +        }
> > > > > > > +    case 8:
> > > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > > >          return 0;
> > > > > > > -    }
> > > > > > > -
> > > > > > > -    if (cregs->special_ops && cregs->special_ops->read) {
> > > > > > > -        return cregs->special_ops->read(cxl_cstate, offset, size);
> > > > > > > -    } else {
> > > > > > > -        return cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > > +    default:
> > > > > > > +        /*
> > > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > > +         * routine is not called with other sizes.
> > > > > > > +         */
> > > > > > > +        g_assert_not_reached();
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -117,25 +125,36 @@ static void cxl_cache_mem_write_reg(void *opaque, hwaddr offset, uint64_t value,
> > > > > > >      ComponentRegisters *cregs = &cxl_cstate->crb;
> > > > > > >      uint32_t mask;
> > > > > > >  
> > > > > > > -    if (size == 8) {
> > > > > > > +    switch (size) {
> > > > > > > +    case 4:
> > > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_regs_write_mask) != 4);
> > > > > > > +        QEMU_BUILD_BUG_ON(sizeof(*cregs->cache_mem_registers) != 4);
> > > > > > > +        mask = cregs->cache_mem_regs_write_mask[offset / 4];
> > > > > > > +        value &= mask;
> > > > > > > +        /* RO bits should remain constant. Done by reading existing value */
> > > > > > > +        value |= ~mask & cregs->cache_mem_registers[offset / 4];
> > > > > > > +        if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > > +            cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > > +            return;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > > +            offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > > +            dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > > +        } else {
> > > > > > > +            cregs->cache_mem_registers[offset / 4] = value;
> > > > > > > +        }
> > > > > > > +        return;
> > > > > > > +    case 8:
> > > > > > >          qemu_log_mask(LOG_UNIMP,
> > > > > > >                        "CXL 8 byte cache mem registers not implemented\n");
> > > > > > >          return;
> > > > > > > -    }
> > > > > > > -    mask = cregs->cache_mem_regs_write_mask[offset / sizeof(*cregs->cache_mem_regs_write_mask)];
> > > > > > > -    value &= mask;
> > > > > > > -    /* RO bits should remain constant. Done by reading existing value */
> > > > > > > -    value |= ~mask & cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)];
> > > > > > > -    if (cregs->special_ops && cregs->special_ops->write) {
> > > > > > > -        cregs->special_ops->write(cxl_cstate, offset, value, size);
> > > > > > > -        return;
> > > > > > > -    }
> > > > > > > -
> > > > > > > -    if (offset >= A_CXL_HDM_DECODER_CAPABILITY &&
> > > > > > > -        offset <= A_CXL_HDM_DECODER3_TARGET_LIST_HI) {
> > > > > > > -        dumb_hdm_handler(cxl_cstate, offset, value);
> > > > > > > -    } else {
> > > > > > > -        cregs->cache_mem_registers[offset / sizeof(*cregs->cache_mem_registers)] = value;
> > > > > > > +    default:
> > > > > > > +        /*
> > > > > > > +         * In line with specifiction limitaions on access sizes, this
> > > > > > > +         * routine is not called with other sizes.
> > > > > > > +         */
> > > > > > > +        g_assert_not_reached();
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > -- 
> > > > > > > 2.39.2          
> > > > > > 
> > > > > >         
> > > > >       
> > > > 
> > > > 
> > > >     
> >   
> 

[PATCH v4 3/4] hw/cxl: CXLDVSECPortExtensions renamed to CXLDVSECPortExt

[Jonathan Cameron via]

Done to reduce line lengths where this is used.
Ext seems sufficiently obvious that it need not be spelt out
fully.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
---
 include/hw/cxl/cxl_pci.h       |  6 ++---
 hw/cxl/cxl-component-utils.c   | 49 ++++++++++++++++++++--------------
 hw/pci-bridge/cxl_downstream.c |  2 +-
 hw/pci-bridge/cxl_root_port.c  |  2 +-
 hw/pci-bridge/cxl_upstream.c   |  2 +-
 5 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
index 407be95b9e..ddf01a543b 100644
--- a/include/hw/cxl/cxl_pci.h
+++ b/include/hw/cxl/cxl_pci.h
@@ -86,7 +86,7 @@ typedef struct CXLDVSECDevice {
 QEMU_BUILD_BUG_ON(sizeof(CXLDVSECDevice) != 0x38);
 
 /* CXL 2.0 - 8.1.5 (ID 0003) */
-typedef struct CXLDVSECPortExtensions {
+typedef struct CXLDVSECPortExt {
     DVSECHeader hdr;
     uint16_t status;
     uint16_t control;
@@ -100,8 +100,8 @@ typedef struct CXLDVSECPortExtensions {
     uint32_t alt_prefetch_limit_high;
     uint32_t rcrb_base;
     uint32_t rcrb_base_high;
-} CXLDVSECPortExtensions;
-QEMU_BUILD_BUG_ON(sizeof(CXLDVSECPortExtensions) != 0x28);
+} CXLDVSECPortExt;
+QEMU_BUILD_BUG_ON(sizeof(CXLDVSECPortExt) != 0x28);
 
 #define PORT_CONTROL_OFFSET          0xc
 #define PORT_CONTROL_UNMASK_SBR      1
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index d1997a52e6..8ab04dbb01 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -392,26 +392,35 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
     case NON_CXL_FUNCTION_MAP_DVSEC:
         break; /* Not yet implemented */
     case EXTENSIONS_PORT_DVSEC:
-        wmask[offset + offsetof(CXLDVSECPortExtensions, control)] = 0x0F;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, control) + 1] = 0x40;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_bus_base)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_bus_limit)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_base)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_base) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_limit)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_limit) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 2] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 3] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 2] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 3] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, control)] = 0x0F;
+        wmask[offset + offsetof(CXLDVSECPortExt, control) + 1] = 0x40;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_bus_base)] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_bus_limit)] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_base)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_base) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_limit)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_limit) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high)] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 2] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 3] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high)] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 2] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 3] =
+            0xFF;
         break;
     case GPF_PORT_DVSEC:
         wmask[offset + offsetof(CXLDVSECPortGPF, phase1_ctrl)] = 0x0F;
diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 5a2b749c8e..8c0f759add 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -98,7 +98,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 };
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){ 0 };
     cxl_component_create_dvsec(cxl, CXL2_DOWNSTREAM_PORT,
                                EXTENSIONS_PORT_DVSEC_LENGTH,
                                EXTENSIONS_PORT_DVSEC,
diff --git a/hw/pci-bridge/cxl_root_port.c b/hw/pci-bridge/cxl_root_port.c
index 7dfd20aa67..8f97697631 100644
--- a/hw/pci-bridge/cxl_root_port.c
+++ b/hw/pci-bridge/cxl_root_port.c
@@ -107,7 +107,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 };
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){ 0 };
     cxl_component_create_dvsec(cxl, CXL2_ROOT_PORT,
                                EXTENSIONS_PORT_DVSEC_LENGTH,
                                EXTENSIONS_PORT_DVSEC,
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index a57806fb31..b81bb5fec9 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -116,7 +116,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){
         .status = 0x1, /* Port Power Management Init Complete */
     };
     cxl_component_create_dvsec(cxl, CXL2_UPSTREAM_PORT,
-- 
2.39.2

[PATCH v4 3/4] hw/cxl: CXLDVSECPortExtensions renamed to CXLDVSECPortExt

[Jonathan Cameron]

Done to reduce line lengths where this is used.
Ext seems sufficiently obvious that it need not be spelt out
fully.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Fan Ni <fan.ni@samsung.com>
---
 include/hw/cxl/cxl_pci.h       |  6 ++---
 hw/cxl/cxl-component-utils.c   | 49 ++++++++++++++++++++--------------
 hw/pci-bridge/cxl_downstream.c |  2 +-
 hw/pci-bridge/cxl_root_port.c  |  2 +-
 hw/pci-bridge/cxl_upstream.c   |  2 +-
 5 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/include/hw/cxl/cxl_pci.h b/include/hw/cxl/cxl_pci.h
index 407be95b9e..ddf01a543b 100644
--- a/include/hw/cxl/cxl_pci.h
+++ b/include/hw/cxl/cxl_pci.h
@@ -86,7 +86,7 @@ typedef struct CXLDVSECDevice {
 QEMU_BUILD_BUG_ON(sizeof(CXLDVSECDevice) != 0x38);
 
 /* CXL 2.0 - 8.1.5 (ID 0003) */
-typedef struct CXLDVSECPortExtensions {
+typedef struct CXLDVSECPortExt {
     DVSECHeader hdr;
     uint16_t status;
     uint16_t control;
@@ -100,8 +100,8 @@ typedef struct CXLDVSECPortExtensions {
     uint32_t alt_prefetch_limit_high;
     uint32_t rcrb_base;
     uint32_t rcrb_base_high;
-} CXLDVSECPortExtensions;
-QEMU_BUILD_BUG_ON(sizeof(CXLDVSECPortExtensions) != 0x28);
+} CXLDVSECPortExt;
+QEMU_BUILD_BUG_ON(sizeof(CXLDVSECPortExt) != 0x28);
 
 #define PORT_CONTROL_OFFSET          0xc
 #define PORT_CONTROL_UNMASK_SBR      1
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index d1997a52e6..8ab04dbb01 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -392,26 +392,35 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
     case NON_CXL_FUNCTION_MAP_DVSEC:
         break; /* Not yet implemented */
     case EXTENSIONS_PORT_DVSEC:
-        wmask[offset + offsetof(CXLDVSECPortExtensions, control)] = 0x0F;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, control) + 1] = 0x40;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_bus_base)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_bus_limit)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_base)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_base) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_limit)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_memory_limit) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit)] = 0xF0;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 2] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_base_high) + 3] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high)] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 1] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 2] = 0xFF;
-        wmask[offset + offsetof(CXLDVSECPortExtensions, alt_prefetch_limit_high) + 3] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, control)] = 0x0F;
+        wmask[offset + offsetof(CXLDVSECPortExt, control) + 1] = 0x40;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_bus_base)] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_bus_limit)] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_base)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_base) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_limit)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_memory_limit) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base) + 1] = 0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit)] = 0xF0;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high)] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 2] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_base_high) + 3] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high)] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 1] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 2] =
+            0xFF;
+        wmask[offset + offsetof(CXLDVSECPortExt, alt_prefetch_limit_high) + 3] =
+            0xFF;
         break;
     case GPF_PORT_DVSEC:
         wmask[offset + offsetof(CXLDVSECPortGPF, phase1_ctrl)] = 0x0F;
diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 5a2b749c8e..8c0f759add 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -98,7 +98,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 };
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){ 0 };
     cxl_component_create_dvsec(cxl, CXL2_DOWNSTREAM_PORT,
                                EXTENSIONS_PORT_DVSEC_LENGTH,
                                EXTENSIONS_PORT_DVSEC,
diff --git a/hw/pci-bridge/cxl_root_port.c b/hw/pci-bridge/cxl_root_port.c
index 7dfd20aa67..8f97697631 100644
--- a/hw/pci-bridge/cxl_root_port.c
+++ b/hw/pci-bridge/cxl_root_port.c
@@ -107,7 +107,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){ 0 };
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){ 0 };
     cxl_component_create_dvsec(cxl, CXL2_ROOT_PORT,
                                EXTENSIONS_PORT_DVSEC_LENGTH,
                                EXTENSIONS_PORT_DVSEC,
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index a57806fb31..b81bb5fec9 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -116,7 +116,7 @@ static void build_dvsecs(CXLComponentState *cxl)
 {
     uint8_t *dvsec;
 
-    dvsec = (uint8_t *)&(CXLDVSECPortExtensions){
+    dvsec = (uint8_t *)&(CXLDVSECPortExt){
         .status = 0x1, /* Port Power Management Init Complete */
     };
     cxl_component_create_dvsec(cxl, CXL2_UPSTREAM_PORT,
-- 
2.39.2

[PATCH v4 4/4] hw/cxl: Line length reductions

[Jonathan Cameron via]

Michael Tsirkin observed that there were some unnecessarily
long lines in the CXL code in a recent review.
This patch is intended to rectify that where it does not
hurt readability.

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
 include/hw/cxl/cxl_component.h |  3 ++-
 include/hw/cxl/cxl_device.h    |  5 +++--
 include/hw/cxl/cxl_events.h    |  3 ++-
 hw/cxl/cxl-cdat.c              |  3 ++-
 hw/cxl/cxl-component-utils.c   | 14 ++++++++------
 hw/cxl/cxl-events.c            |  9 ++++++---
 hw/cxl/cxl-mailbox-utils.c     | 21 ++++++++++++++-------
 hw/mem/cxl_type3.c             | 31 +++++++++++++++++++------------
 hw/mem/cxl_type3_stubs.c       |  5 +++--
 9 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 3c795a6278..e52dd8d2b9 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -175,7 +175,8 @@ HDM_DECODER_INIT(3);
     (CXL_IDE_REGISTERS_OFFSET + CXL_IDE_REGISTERS_SIZE)
 #define CXL_SNOOP_REGISTERS_SIZE   0x8
 
-QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET + CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
+QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET +
+                    CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
                    "No space for registers");
 
 typedef struct component_registers {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 51cd0d9ce3..007ddaf078 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -192,7 +192,7 @@ void cxl_device_register_init_common(CXLDeviceState *dev);
  * Documented as a 128 bit register, but 64 bit accesses and the second
  * 64 bits are currently reserved.
  */
-REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte accesses */
+REG64(CXL_DEV_CAP_ARRAY, 0)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_ID, 0, 16)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
@@ -361,7 +361,8 @@ struct CXLType3Class {
                         uint64_t offset);
     void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
                     uint64_t offset);
-    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data);
+    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset,
+                          uint8_t *data);
 };
 
 MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 089ba2091f..d778487b7e 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -92,7 +92,8 @@ typedef enum CXLEventIntMode {
     CXL_INT_RES      = 0x03,
 } CXLEventIntMode;
 #define CXL_EVENT_INT_MODE_MASK 0x3
-#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
+#define CXL_EVENT_INT_SETTING(vector) \
+    ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
 typedef struct CXLEventInterruptPolicy {
     uint8_t info_settings;
     uint8_t warn_settings;
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index d246d6885b..639a2db3e1 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -60,7 +60,8 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
         return;
     }
 
-    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf, cdat->private);
+    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
+                                                 cdat->private);
 
     if (!cdat->built_buf_len) {
         /* Build later as not all data available yet */
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 8ab04dbb01..80efbf6365 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -240,7 +240,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 1);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_256B, 1);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_4K, 1);
-    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, POISON_ON_ERR_CAP, 0);
+    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
+                     POISON_ON_ERR_CAP, 0);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_GLOBAL_CONTROL,
                      HDM_DECODER_ENABLE, 0);
     write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
@@ -263,15 +264,16 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
     }
 }
 
-void cxl_component_register_init_common(uint32_t *reg_state, uint32_t *write_msk,
+void cxl_component_register_init_common(uint32_t *reg_state,
+                                        uint32_t *write_msk,
                                         enum reg_type type)
 {
     int caps = 0;
 
     /*
-     * In CXL 2.0 the capabilities required for each CXL component are such that,
-     * with the ordering chosen here, a single number can be used to define
-     * which capabilities should be provided.
+     * In CXL 2.0 the capabilities required for each CXL component are such
+     * that, with the ordering chosen here, a single number can be used to
+     * define which capabilities should be provided.
      */
     switch (type) {
     case CXL2_DOWNSTREAM_PORT:
@@ -448,7 +450,7 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
         default: /* Registers are RO for other component types */
             break;
         }
-        /* There are rw1cs bits in the status register but never set currently */
+        /* There are rw1cs bits in the status register but never set */
         break;
     }
 
diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 3ddd6369ad..e2172b94b9 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -170,8 +170,10 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
     if (log->overflow_err_count) {
         pl->flags |= CXL_GET_EVENT_FLAG_OVERFLOW;
         pl->overflow_err_count = cpu_to_le16(log->overflow_err_count);
-        pl->first_overflow_timestamp = cpu_to_le64(log->first_overflow_timestamp);
-        pl->last_overflow_timestamp = cpu_to_le64(log->last_overflow_timestamp);
+        pl->first_overflow_timestamp =
+            cpu_to_le64(log->first_overflow_timestamp);
+        pl->last_overflow_timestamp =
+            cpu_to_le64(log->last_overflow_timestamp);
     }
 
     pl->record_count = cpu_to_le16(nr);
@@ -180,7 +182,8 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
     return CXL_MBOX_SUCCESS;
 }
 
-CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *pl)
+CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
+                                   CXLClearEventPayload *pl)
 {
     CXLEventLog *log;
     uint8_t log_type;
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 434ccc5f6e..ab082ec9de 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -366,9 +366,12 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
 
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
-    stq_le_p(&id->total_capacity, cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
-    stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
-    stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->total_capacity,
+             cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->persistent_capacity,
+             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->volatile_capacity,
+             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
     stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
     /* 256 poison records */
     st24_le_p(id->poison_list_max_mer, 256);
@@ -396,13 +399,15 @@ static CXLRetCode cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
-    stq_le_p(&part_info->active_vmem, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&part_info->active_vmem,
+             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
     /*
      * When both next_vmem and next_pmem are 0, there is no pending change to
      * partitioning.
      */
     stq_le_p(&part_info->next_vmem, 0);
-    stq_le_p(&part_info->active_pmem, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&part_info->active_pmem,
+             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
     stq_le_p(&part_info->next_pmem, 0);
 
     *len = sizeof(*part_info);
@@ -681,8 +686,10 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
     [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
         cmd_firmware_update_get_info, 0, 0 },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
-    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE },
-    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 },
+    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
+                         8, IMMEDIATE_POLICY_CHANGE },
+    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
+                              0, 0 },
     [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
     [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
         cmd_identify_memory_device, 0, 0 },
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index c02be4ce45..18ad853f5b 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -208,10 +208,9 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
     }
 
     if (nonvolatile_mr) {
+        uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
         rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
-                                           nonvolatile_mr, true,
-                                           (volatile_mr ?
-                                            memory_region_size(volatile_mr) : 0));
+                                           nonvolatile_mr, true, base);
         if (rc < 0) {
             goto error_cleanup;
         }
@@ -514,7 +513,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     case A_CXL_RAS_UNC_ERR_STATUS:
     {
         uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
-        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL, FIRST_ERROR_POINTER);
+        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL,
+                                 FIRST_ERROR_POINTER);
         CXLError *cxl_err;
         uint32_t unc_err;
 
@@ -533,7 +533,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
                  * closest to behavior of hardware not capable of multiple
                  * header recording.
                  */
-                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node, cxl_next) {
+                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node,
+                                    cxl_next) {
                     if ((1 << cxl_err->type) & value) {
                         QTAILQ_REMOVE(&ct3d->error_list, cxl_err, node);
                         g_free(cxl_err);
@@ -1072,7 +1073,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
         if (((start >= p->start) && (start < p->start + p->length)) ||
             ((start + length > p->start) &&
              (start + length <= p->start + p->length))) {
-            error_setg(errp, "Overlap with existing poisoned region not supported");
+            error_setg(errp,
+                       "Overlap with existing poisoned region not supported");
             return;
         }
     }
@@ -1085,7 +1087,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
     p = g_new0(CXLPoison, 1);
     p->length = length;
     p->start = start;
-    p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
+    /* Different from injected via the mbox */
+    p->type = CXL_POISON_TYPE_INTERNAL;
 
     QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
     ct3d->poison_list_cnt++;
@@ -1222,7 +1225,8 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
         return;
     }
     /* If the error is masked, nothting to do here */
-    if (!((1 << cxl_err_type) & ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
+    if (!((1 << cxl_err_type) &
+          ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
         return;
     }
 
@@ -1372,7 +1376,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
                                bool has_bank, uint8_t bank,
                                bool has_row, uint32_t row,
                                bool has_column, uint16_t column,
-                               bool has_correction_mask, uint64List *correction_mask,
+                               bool has_correction_mask,
+                               uint64List *correction_mask,
                                Error **errp)
 {
     Object *obj = object_resolve_path(path, NULL);
@@ -1473,7 +1478,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
                                         int16_t temperature,
                                         uint32_t dirty_shutdown_count,
                                         uint32_t corrected_volatile_error_count,
-                                        uint32_t corrected_persistent_error_count,
+                                        uint32_t corrected_persist_error_count,
                                         Error **errp)
 {
     Object *obj = object_resolve_path(path, NULL);
@@ -1513,8 +1518,10 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
     module.life_used = life_used;
     stw_le_p(&module.temperature, temperature);
     stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
-    stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
-    stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
+    stl_le_p(&module.corrected_volatile_error_count,
+             corrected_volatile_error_count);
+    stl_le_p(&module.corrected_persistent_error_count,
+             corrected_persist_error_count);
 
     if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
         cxl_event_irq_assert(ct3d);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 8ba5d3d1f7..3e1851e32b 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -33,7 +33,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
                                bool has_bank, uint8_t bank,
                                bool has_row, uint32_t row,
                                bool has_column, uint16_t column,
-                               bool has_correction_mask, uint64List *correction_mask,
+                               bool has_correction_mask,
+                               uint64List *correction_mask,
                                Error **errp) {}
 
 void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
@@ -45,7 +46,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
                                         int16_t temperature,
                                         uint32_t dirty_shutdown_count,
                                         uint32_t corrected_volatile_error_count,
-                                        uint32_t corrected_persistent_error_count,
+                                        uint32_t corrected_persist_error_count,
                                         Error **errp) {}
 
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
-- 
2.39.2

[PATCH v4 4/4] hw/cxl: Line length reductions

[Jonathan Cameron]

Michael Tsirkin observed that there were some unnecessarily
long lines in the CXL code in a recent review.
This patch is intended to rectify that where it does not
hurt readability.

Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
 include/hw/cxl/cxl_component.h |  3 ++-
 include/hw/cxl/cxl_device.h    |  5 +++--
 include/hw/cxl/cxl_events.h    |  3 ++-
 hw/cxl/cxl-cdat.c              |  3 ++-
 hw/cxl/cxl-component-utils.c   | 14 ++++++++------
 hw/cxl/cxl-events.c            |  9 ++++++---
 hw/cxl/cxl-mailbox-utils.c     | 21 ++++++++++++++-------
 hw/mem/cxl_type3.c             | 31 +++++++++++++++++++------------
 hw/mem/cxl_type3_stubs.c       |  5 +++--
 9 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 3c795a6278..e52dd8d2b9 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -175,7 +175,8 @@ HDM_DECODER_INIT(3);
     (CXL_IDE_REGISTERS_OFFSET + CXL_IDE_REGISTERS_SIZE)
 #define CXL_SNOOP_REGISTERS_SIZE   0x8
 
-QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET + CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
+QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET +
+                    CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
                    "No space for registers");
 
 typedef struct component_registers {
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 51cd0d9ce3..007ddaf078 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -192,7 +192,7 @@ void cxl_device_register_init_common(CXLDeviceState *dev);
  * Documented as a 128 bit register, but 64 bit accesses and the second
  * 64 bits are currently reserved.
  */
-REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte accesses */
+REG64(CXL_DEV_CAP_ARRAY, 0)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_ID, 0, 16)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
     FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
@@ -361,7 +361,8 @@ struct CXLType3Class {
                         uint64_t offset);
     void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
                     uint64_t offset);
-    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data);
+    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset,
+                          uint8_t *data);
 };
 
 MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
index 089ba2091f..d778487b7e 100644
--- a/include/hw/cxl/cxl_events.h
+++ b/include/hw/cxl/cxl_events.h
@@ -92,7 +92,8 @@ typedef enum CXLEventIntMode {
     CXL_INT_RES      = 0x03,
 } CXLEventIntMode;
 #define CXL_EVENT_INT_MODE_MASK 0x3
-#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
+#define CXL_EVENT_INT_SETTING(vector) \
+    ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
 typedef struct CXLEventInterruptPolicy {
     uint8_t info_settings;
     uint8_t warn_settings;
diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index d246d6885b..639a2db3e1 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -60,7 +60,8 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
         return;
     }
 
-    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf, cdat->private);
+    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
+                                                 cdat->private);
 
     if (!cdat->built_buf_len) {
         /* Build later as not all data available yet */
diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
index 8ab04dbb01..80efbf6365 100644
--- a/hw/cxl/cxl-component-utils.c
+++ b/hw/cxl/cxl-component-utils.c
@@ -240,7 +240,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 1);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_256B, 1);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_4K, 1);
-    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, POISON_ON_ERR_CAP, 0);
+    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
+                     POISON_ON_ERR_CAP, 0);
     ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_GLOBAL_CONTROL,
                      HDM_DECODER_ENABLE, 0);
     write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
@@ -263,15 +264,16 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
     }
 }
 
-void cxl_component_register_init_common(uint32_t *reg_state, uint32_t *write_msk,
+void cxl_component_register_init_common(uint32_t *reg_state,
+                                        uint32_t *write_msk,
                                         enum reg_type type)
 {
     int caps = 0;
 
     /*
-     * In CXL 2.0 the capabilities required for each CXL component are such that,
-     * with the ordering chosen here, a single number can be used to define
-     * which capabilities should be provided.
+     * In CXL 2.0 the capabilities required for each CXL component are such
+     * that, with the ordering chosen here, a single number can be used to
+     * define which capabilities should be provided.
      */
     switch (type) {
     case CXL2_DOWNSTREAM_PORT:
@@ -448,7 +450,7 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
         default: /* Registers are RO for other component types */
             break;
         }
-        /* There are rw1cs bits in the status register but never set currently */
+        /* There are rw1cs bits in the status register but never set */
         break;
     }
 
diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
index 3ddd6369ad..e2172b94b9 100644
--- a/hw/cxl/cxl-events.c
+++ b/hw/cxl/cxl-events.c
@@ -170,8 +170,10 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
     if (log->overflow_err_count) {
         pl->flags |= CXL_GET_EVENT_FLAG_OVERFLOW;
         pl->overflow_err_count = cpu_to_le16(log->overflow_err_count);
-        pl->first_overflow_timestamp = cpu_to_le64(log->first_overflow_timestamp);
-        pl->last_overflow_timestamp = cpu_to_le64(log->last_overflow_timestamp);
+        pl->first_overflow_timestamp =
+            cpu_to_le64(log->first_overflow_timestamp);
+        pl->last_overflow_timestamp =
+            cpu_to_le64(log->last_overflow_timestamp);
     }
 
     pl->record_count = cpu_to_le16(nr);
@@ -180,7 +182,8 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
     return CXL_MBOX_SUCCESS;
 }
 
-CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *pl)
+CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
+                                   CXLClearEventPayload *pl)
 {
     CXLEventLog *log;
     uint8_t log_type;
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 434ccc5f6e..ab082ec9de 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -366,9 +366,12 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
 
     snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
 
-    stq_le_p(&id->total_capacity, cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
-    stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
-    stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->total_capacity,
+             cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->persistent_capacity,
+             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&id->volatile_capacity,
+             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
     stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
     /* 256 poison records */
     st24_le_p(id->poison_list_max_mer, 256);
@@ -396,13 +399,15 @@ static CXLRetCode cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
         return CXL_MBOX_INTERNAL_ERROR;
     }
 
-    stq_le_p(&part_info->active_vmem, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&part_info->active_vmem,
+             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
     /*
      * When both next_vmem and next_pmem are 0, there is no pending change to
      * partitioning.
      */
     stq_le_p(&part_info->next_vmem, 0);
-    stq_le_p(&part_info->active_pmem, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
+    stq_le_p(&part_info->active_pmem,
+             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
     stq_le_p(&part_info->next_pmem, 0);
 
     *len = sizeof(*part_info);
@@ -681,8 +686,10 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
     [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
         cmd_firmware_update_get_info, 0, 0 },
     [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
-    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE },
-    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 },
+    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
+                         8, IMMEDIATE_POLICY_CHANGE },
+    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
+                              0, 0 },
     [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
     [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
         cmd_identify_memory_device, 0, 0 },
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index c02be4ce45..18ad853f5b 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -208,10 +208,9 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
     }
 
     if (nonvolatile_mr) {
+        uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
         rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
-                                           nonvolatile_mr, true,
-                                           (volatile_mr ?
-                                            memory_region_size(volatile_mr) : 0));
+                                           nonvolatile_mr, true, base);
         if (rc < 0) {
             goto error_cleanup;
         }
@@ -514,7 +513,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
     case A_CXL_RAS_UNC_ERR_STATUS:
     {
         uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
-        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL, FIRST_ERROR_POINTER);
+        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL,
+                                 FIRST_ERROR_POINTER);
         CXLError *cxl_err;
         uint32_t unc_err;
 
@@ -533,7 +533,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
                  * closest to behavior of hardware not capable of multiple
                  * header recording.
                  */
-                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node, cxl_next) {
+                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node,
+                                    cxl_next) {
                     if ((1 << cxl_err->type) & value) {
                         QTAILQ_REMOVE(&ct3d->error_list, cxl_err, node);
                         g_free(cxl_err);
@@ -1072,7 +1073,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
         if (((start >= p->start) && (start < p->start + p->length)) ||
             ((start + length > p->start) &&
              (start + length <= p->start + p->length))) {
-            error_setg(errp, "Overlap with existing poisoned region not supported");
+            error_setg(errp,
+                       "Overlap with existing poisoned region not supported");
             return;
         }
     }
@@ -1085,7 +1087,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
     p = g_new0(CXLPoison, 1);
     p->length = length;
     p->start = start;
-    p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
+    /* Different from injected via the mbox */
+    p->type = CXL_POISON_TYPE_INTERNAL;
 
     QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
     ct3d->poison_list_cnt++;
@@ -1222,7 +1225,8 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
         return;
     }
     /* If the error is masked, nothting to do here */
-    if (!((1 << cxl_err_type) & ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
+    if (!((1 << cxl_err_type) &
+          ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
         return;
     }
 
@@ -1372,7 +1376,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
                                bool has_bank, uint8_t bank,
                                bool has_row, uint32_t row,
                                bool has_column, uint16_t column,
-                               bool has_correction_mask, uint64List *correction_mask,
+                               bool has_correction_mask,
+                               uint64List *correction_mask,
                                Error **errp)
 {
     Object *obj = object_resolve_path(path, NULL);
@@ -1473,7 +1478,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
                                         int16_t temperature,
                                         uint32_t dirty_shutdown_count,
                                         uint32_t corrected_volatile_error_count,
-                                        uint32_t corrected_persistent_error_count,
+                                        uint32_t corrected_persist_error_count,
                                         Error **errp)
 {
     Object *obj = object_resolve_path(path, NULL);
@@ -1513,8 +1518,10 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
     module.life_used = life_used;
     stw_le_p(&module.temperature, temperature);
     stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
-    stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
-    stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
+    stl_le_p(&module.corrected_volatile_error_count,
+             corrected_volatile_error_count);
+    stl_le_p(&module.corrected_persistent_error_count,
+             corrected_persist_error_count);
 
     if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
         cxl_event_irq_assert(ct3d);
diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
index 8ba5d3d1f7..3e1851e32b 100644
--- a/hw/mem/cxl_type3_stubs.c
+++ b/hw/mem/cxl_type3_stubs.c
@@ -33,7 +33,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
                                bool has_bank, uint8_t bank,
                                bool has_row, uint32_t row,
                                bool has_column, uint16_t column,
-                               bool has_correction_mask, uint64List *correction_mask,
+                               bool has_correction_mask,
+                               uint64List *correction_mask,
                                Error **errp) {}
 
 void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
@@ -45,7 +46,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
                                         int16_t temperature,
                                         uint32_t dirty_shutdown_count,
                                         uint32_t corrected_volatile_error_count,
-                                        uint32_t corrected_persistent_error_count,
+                                        uint32_t corrected_persist_error_count,
                                         Error **errp) {}
 
 void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
-- 
2.39.2

Re: [PATCH v4 4/4] hw/cxl: Line length reductions

[fan]

On Thu, Oct 12, 2023 at 03:05:14PM +0100, Jonathan Cameron wrote:
> Michael Tsirkin observed that there were some unnecessarily
> long lines in the CXL code in a recent review.
> This patch is intended to rectify that where it does not
> hurt readability.
> 
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  include/hw/cxl/cxl_component.h |  3 ++-
>  include/hw/cxl/cxl_device.h    |  5 +++--
>  include/hw/cxl/cxl_events.h    |  3 ++-
>  hw/cxl/cxl-cdat.c              |  3 ++-
>  hw/cxl/cxl-component-utils.c   | 14 ++++++++------
>  hw/cxl/cxl-events.c            |  9 ++++++---
>  hw/cxl/cxl-mailbox-utils.c     | 21 ++++++++++++++-------
>  hw/mem/cxl_type3.c             | 31 +++++++++++++++++++------------
>  hw/mem/cxl_type3_stubs.c       |  5 +++--
>  9 files changed, 59 insertions(+), 35 deletions(-)
> 
> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 3c795a6278..e52dd8d2b9 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -175,7 +175,8 @@ HDM_DECODER_INIT(3);
>      (CXL_IDE_REGISTERS_OFFSET + CXL_IDE_REGISTERS_SIZE)
>  #define CXL_SNOOP_REGISTERS_SIZE   0x8
>  
> -QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET + CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
> +QEMU_BUILD_BUG_MSG((CXL_SNOOP_REGISTERS_OFFSET +
> +                    CXL_SNOOP_REGISTERS_SIZE) >= 0x1000,
>                     "No space for registers");
>  
>  typedef struct component_registers {
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 51cd0d9ce3..007ddaf078 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -192,7 +192,7 @@ void cxl_device_register_init_common(CXLDeviceState *dev);
>   * Documented as a 128 bit register, but 64 bit accesses and the second
>   * 64 bits are currently reserved.
>   */
> -REG64(CXL_DEV_CAP_ARRAY, 0) /* Documented as 128 bit register but 64 byte accesses */
> +REG64(CXL_DEV_CAP_ARRAY, 0)
>      FIELD(CXL_DEV_CAP_ARRAY, CAP_ID, 0, 16)
>      FIELD(CXL_DEV_CAP_ARRAY, CAP_VERSION, 16, 8)
>      FIELD(CXL_DEV_CAP_ARRAY, CAP_COUNT, 32, 16)
> @@ -361,7 +361,8 @@ struct CXLType3Class {
>                          uint64_t offset);
>      void (*set_lsa)(CXLType3Dev *ct3d, const void *buf, uint64_t size,
>                      uint64_t offset);
> -    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data);
> +    bool (*set_cacheline)(CXLType3Dev *ct3d, uint64_t dpa_offset,
> +                          uint8_t *data);
>  };
>  
>  MemTxResult cxl_type3_read(PCIDevice *d, hwaddr host_addr, uint64_t *data,
> diff --git a/include/hw/cxl/cxl_events.h b/include/hw/cxl/cxl_events.h
> index 089ba2091f..d778487b7e 100644
> --- a/include/hw/cxl/cxl_events.h
> +++ b/include/hw/cxl/cxl_events.h
> @@ -92,7 +92,8 @@ typedef enum CXLEventIntMode {
>      CXL_INT_RES      = 0x03,
>  } CXLEventIntMode;
>  #define CXL_EVENT_INT_MODE_MASK 0x3
> -#define CXL_EVENT_INT_SETTING(vector) ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
> +#define CXL_EVENT_INT_SETTING(vector) \
> +    ((((uint8_t)vector & 0xf) << 4) | CXL_INT_MSI_MSIX)
>  typedef struct CXLEventInterruptPolicy {
>      uint8_t info_settings;
>      uint8_t warn_settings;
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> index d246d6885b..639a2db3e1 100644
> --- a/hw/cxl/cxl-cdat.c
> +++ b/hw/cxl/cxl-cdat.c
> @@ -60,7 +60,8 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
>          return;
>      }
>  
> -    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf, cdat->private);
> +    cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
> +                                                 cdat->private);
>  
>      if (!cdat->built_buf_len) {
>          /* Build later as not all data available yet */
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index 8ab04dbb01..80efbf6365 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -240,7 +240,8 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, TARGET_COUNT, 1);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_256B, 1);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, INTERLEAVE_4K, 1);
> -    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, POISON_ON_ERR_CAP, 0);
> +    ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> +                     POISON_ON_ERR_CAP, 0);
>      ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_GLOBAL_CONTROL,
>                       HDM_DECODER_ENABLE, 0);
>      write_msk[R_CXL_HDM_DECODER_GLOBAL_CONTROL] = 0x3;
> @@ -263,15 +264,16 @@ static void hdm_init_common(uint32_t *reg_state, uint32_t *write_msk,
>      }
>  }
>  
> -void cxl_component_register_init_common(uint32_t *reg_state, uint32_t *write_msk,
> +void cxl_component_register_init_common(uint32_t *reg_state,
> +                                        uint32_t *write_msk,
>                                          enum reg_type type)
>  {
>      int caps = 0;
>  
>      /*
> -     * In CXL 2.0 the capabilities required for each CXL component are such that,
> -     * with the ordering chosen here, a single number can be used to define
> -     * which capabilities should be provided.
> +     * In CXL 2.0 the capabilities required for each CXL component are such
> +     * that, with the ordering chosen here, a single number can be used to
> +     * define which capabilities should be provided.
>       */
>      switch (type) {
>      case CXL2_DOWNSTREAM_PORT:
> @@ -448,7 +450,7 @@ void cxl_component_create_dvsec(CXLComponentState *cxl,
>          default: /* Registers are RO for other component types */
>              break;
>          }
> -        /* There are rw1cs bits in the status register but never set currently */
> +        /* There are rw1cs bits in the status register but never set */
>          break;
>      }
>  
> diff --git a/hw/cxl/cxl-events.c b/hw/cxl/cxl-events.c
> index 3ddd6369ad..e2172b94b9 100644
> --- a/hw/cxl/cxl-events.c
> +++ b/hw/cxl/cxl-events.c
> @@ -170,8 +170,10 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
>      if (log->overflow_err_count) {
>          pl->flags |= CXL_GET_EVENT_FLAG_OVERFLOW;
>          pl->overflow_err_count = cpu_to_le16(log->overflow_err_count);
> -        pl->first_overflow_timestamp = cpu_to_le64(log->first_overflow_timestamp);
> -        pl->last_overflow_timestamp = cpu_to_le64(log->last_overflow_timestamp);
> +        pl->first_overflow_timestamp =
> +            cpu_to_le64(log->first_overflow_timestamp);
> +        pl->last_overflow_timestamp =
> +            cpu_to_le64(log->last_overflow_timestamp);
>      }
>  
>      pl->record_count = cpu_to_le16(nr);
> @@ -180,7 +182,8 @@ CXLRetCode cxl_event_get_records(CXLDeviceState *cxlds, CXLGetEventPayload *pl,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> -CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds, CXLClearEventPayload *pl)
> +CXLRetCode cxl_event_clear_records(CXLDeviceState *cxlds,
> +                                   CXLClearEventPayload *pl)
>  {
>      CXLEventLog *log;
>      uint8_t log_type;
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 434ccc5f6e..ab082ec9de 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -366,9 +366,12 @@ static CXLRetCode cmd_identify_memory_device(struct cxl_cmd *cmd,
>  
>      snprintf(id->fw_revision, 0x10, "BWFW VERSION %02d", 0);
>  
> -    stq_le_p(&id->total_capacity, cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
> -    stq_le_p(&id->persistent_capacity, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> -    stq_le_p(&id->volatile_capacity, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> +    stq_le_p(&id->total_capacity,
> +             cxl_dstate->mem_size / CXL_CAPACITY_MULTIPLIER);
> +    stq_le_p(&id->persistent_capacity,
> +             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> +    stq_le_p(&id->volatile_capacity,
> +             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
>      stl_le_p(&id->lsa_size, cvc->get_lsa_size(ct3d));
>      /* 256 poison records */
>      st24_le_p(id->poison_list_max_mer, 256);
> @@ -396,13 +399,15 @@ static CXLRetCode cmd_ccls_get_partition_info(struct cxl_cmd *cmd,
>          return CXL_MBOX_INTERNAL_ERROR;
>      }
>  
> -    stq_le_p(&part_info->active_vmem, cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
> +    stq_le_p(&part_info->active_vmem,
> +             cxl_dstate->vmem_size / CXL_CAPACITY_MULTIPLIER);
>      /*
>       * When both next_vmem and next_pmem are 0, there is no pending change to
>       * partitioning.
>       */
>      stq_le_p(&part_info->next_vmem, 0);
> -    stq_le_p(&part_info->active_pmem, cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
> +    stq_le_p(&part_info->active_pmem,
> +             cxl_dstate->pmem_size / CXL_CAPACITY_MULTIPLIER);
>      stq_le_p(&part_info->next_pmem, 0);
>  
>      *len = sizeof(*part_info);
> @@ -681,8 +686,10 @@ static struct cxl_cmd cxl_cmd_set[256][256] = {
>      [FIRMWARE_UPDATE][GET_INFO] = { "FIRMWARE_UPDATE_GET_INFO",
>          cmd_firmware_update_get_info, 0, 0 },
>      [TIMESTAMP][GET] = { "TIMESTAMP_GET", cmd_timestamp_get, 0, 0 },
> -    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set, 8, IMMEDIATE_POLICY_CHANGE },
> -    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported, 0, 0 },
> +    [TIMESTAMP][SET] = { "TIMESTAMP_SET", cmd_timestamp_set,
> +                         8, IMMEDIATE_POLICY_CHANGE },
> +    [LOGS][GET_SUPPORTED] = { "LOGS_GET_SUPPORTED", cmd_logs_get_supported,
> +                              0, 0 },
>      [LOGS][GET_LOG] = { "LOGS_GET_LOG", cmd_logs_get_log, 0x18, 0 },
>      [IDENTIFY][MEMORY_DEVICE] = { "IDENTIFY_MEMORY_DEVICE",
>          cmd_identify_memory_device, 0, 0 },
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index c02be4ce45..18ad853f5b 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -208,10 +208,9 @@ static int ct3_build_cdat_table(CDATSubHeader ***cdat_table, void *priv)
>      }
>  
>      if (nonvolatile_mr) {
> +        uint64_t base = volatile_mr ? memory_region_size(volatile_mr) : 0;
>          rc = ct3_build_cdat_entries_for_mr(&(table[cur_ent]), dsmad_handle++,
> -                                           nonvolatile_mr, true,
> -                                           (volatile_mr ?
> -                                            memory_region_size(volatile_mr) : 0));
> +                                           nonvolatile_mr, true, base);
>          if (rc < 0) {
>              goto error_cleanup;
>          }
> @@ -514,7 +513,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>      case A_CXL_RAS_UNC_ERR_STATUS:
>      {
>          uint32_t capctrl = ldl_le_p(cache_mem + R_CXL_RAS_ERR_CAP_CTRL);
> -        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL, FIRST_ERROR_POINTER);
> +        uint32_t fe = FIELD_EX32(capctrl, CXL_RAS_ERR_CAP_CTRL,
> +                                 FIRST_ERROR_POINTER);
>          CXLError *cxl_err;
>          uint32_t unc_err;
>  
> @@ -533,7 +533,8 @@ static void ct3d_reg_write(void *opaque, hwaddr offset, uint64_t value,
>                   * closest to behavior of hardware not capable of multiple
>                   * header recording.
>                   */
> -                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node, cxl_next) {
> +                QTAILQ_FOREACH_SAFE(cxl_err, &ct3d->error_list, node,
> +                                    cxl_next) {
>                      if ((1 << cxl_err->type) & value) {
>                          QTAILQ_REMOVE(&ct3d->error_list, cxl_err, node);
>                          g_free(cxl_err);
> @@ -1072,7 +1073,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>          if (((start >= p->start) && (start < p->start + p->length)) ||
>              ((start + length > p->start) &&
>               (start + length <= p->start + p->length))) {
> -            error_setg(errp, "Overlap with existing poisoned region not supported");
> +            error_setg(errp,
> +                       "Overlap with existing poisoned region not supported");
>              return;
>          }
>      }
> @@ -1085,7 +1087,8 @@ void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
>      p = g_new0(CXLPoison, 1);
>      p->length = length;
>      p->start = start;
> -    p->type = CXL_POISON_TYPE_INTERNAL; /* Different from injected via the mbox */
> +    /* Different from injected via the mbox */
> +    p->type = CXL_POISON_TYPE_INTERNAL;
>  
>      QLIST_INSERT_HEAD(&ct3d->poison_list, p, node);
>      ct3d->poison_list_cnt++;
> @@ -1222,7 +1225,8 @@ void qmp_cxl_inject_correctable_error(const char *path, CxlCorErrorType type,
>          return;
>      }
>      /* If the error is masked, nothting to do here */
> -    if (!((1 << cxl_err_type) & ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
> +    if (!((1 << cxl_err_type) &
> +          ~ldl_le_p(reg_state + R_CXL_RAS_COR_ERR_MASK))) {
>          return;
>      }
>  
> @@ -1372,7 +1376,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
>                                 bool has_bank, uint8_t bank,
>                                 bool has_row, uint32_t row,
>                                 bool has_column, uint16_t column,
> -                               bool has_correction_mask, uint64List *correction_mask,
> +                               bool has_correction_mask,
> +                               uint64List *correction_mask,
>                                 Error **errp)
>  {
>      Object *obj = object_resolve_path(path, NULL);
> @@ -1473,7 +1478,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
>                                          int16_t temperature,
>                                          uint32_t dirty_shutdown_count,
>                                          uint32_t corrected_volatile_error_count,
> -                                        uint32_t corrected_persistent_error_count,
> +                                        uint32_t corrected_persist_error_count,
>                                          Error **errp)
>  {
>      Object *obj = object_resolve_path(path, NULL);
> @@ -1513,8 +1518,10 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
>      module.life_used = life_used;
>      stw_le_p(&module.temperature, temperature);
>      stl_le_p(&module.dirty_shutdown_count, dirty_shutdown_count);
> -    stl_le_p(&module.corrected_volatile_error_count, corrected_volatile_error_count);
> -    stl_le_p(&module.corrected_persistent_error_count, corrected_persistent_error_count);
> +    stl_le_p(&module.corrected_volatile_error_count,
> +             corrected_volatile_error_count);
> +    stl_le_p(&module.corrected_persistent_error_count,
> +             corrected_persist_error_count);
>  
>      if (cxl_event_insert(cxlds, enc_log, (CXLEventRecordRaw *)&module)) {
>          cxl_event_irq_assert(ct3d);
> diff --git a/hw/mem/cxl_type3_stubs.c b/hw/mem/cxl_type3_stubs.c
> index 8ba5d3d1f7..3e1851e32b 100644
> --- a/hw/mem/cxl_type3_stubs.c
> +++ b/hw/mem/cxl_type3_stubs.c
> @@ -33,7 +33,8 @@ void qmp_cxl_inject_dram_event(const char *path, CxlEventLog log, uint8_t flags,
>                                 bool has_bank, uint8_t bank,
>                                 bool has_row, uint32_t row,
>                                 bool has_column, uint16_t column,
> -                               bool has_correction_mask, uint64List *correction_mask,
> +                               bool has_correction_mask,
> +                               uint64List *correction_mask,
>                                 Error **errp) {}
>  
>  void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
> @@ -45,7 +46,7 @@ void qmp_cxl_inject_memory_module_event(const char *path, CxlEventLog log,
>                                          int16_t temperature,
>                                          uint32_t dirty_shutdown_count,
>                                          uint32_t corrected_volatile_error_count,
> -                                        uint32_t corrected_persistent_error_count,
> +                                        uint32_t corrected_persist_error_count,
>                                          Error **errp) {}
>  
>  void qmp_cxl_inject_poison(const char *path, uint64_t start, uint64_t length,
> -- 
> 2.39.2
>