1. Patchew
  2. QEMU
  3. [PATCH v3 0/4] hw/cxl: Line length reduction and related
  4. View patch

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

Jonathan Cameron via posted 4 patches 2 years, 3 months ago
There is a newer version of this series
[PATCH v3 2/4] hw/cxl: Use switch statements for read and write of cachemem registers
Posted by Jonathan Cameron via 2 years, 3 months ago 
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>
---
 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..7ef3ef2bd6 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 {
+            static_assert(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:
+        static_assert(sizeof(*cregs->cache_mem_regs_write_mask) == 4);
+        static_assert(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 v3 2/4] hw/cxl: Use switch statements for read and write of cachemem registers
Posted by Michael S. Tsirkin 2 years, 3 months ago 
On Tue, Sep 19, 2023 at 10:34:32AM +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>
> ---
>  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..7ef3ef2bd6 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 {
> +            static_assert(sizeof(*cregs->cache_mem_registers) == 4);

In C currently static_assert needs a message.

In fact clang warns:
../hw/cxl/cxl-component-utils.c:75:67: error: '_Static_assert' with no message is a C2x extension [-Werror,-Wc2x-extensions]
            static_assert(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:
> +        static_assert(sizeof(*cregs->cache_mem_regs_write_mask) == 4);
> +        static_assert(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 v3 2/4] hw/cxl: Use switch statements for read and write of cachemem registers
Posted by Jonathan Cameron via 2 years, 3 months ago 
On Tue, 3 Oct 2023 16:42:37 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 19, 2023 at 10:34:32AM +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>
> > ---
> >  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..7ef3ef2bd6 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 {
> > +            static_assert(sizeof(*cregs->cache_mem_registers) == 4);  
> 
> In C currently static_assert needs a message.

I switched to QEMU_BUILD_BUG_ON() which seems better
+ doesn't need a additional message.

> 
> In fact clang warns:
> ../hw/cxl/cxl-component-utils.c:75:67: error: '_Static_assert' with no message is a C2x extension [-Werror,-Wc2x-extensions]
>             static_assert(sizeof(*cregs->cache_mem_registers) == 4);
> 
Thanks,

Jonathan

> 
> > +            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:
> > +        static_assert(sizeof(*cregs->cache_mem_regs_write_mask) == 4);
> > +        static_assert(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 v3 2/4] hw/cxl: Use switch statements for read and write of cachemem registers
Posted by Michael Tokarev 2 years, 3 months ago 
19.09.2023 12:34, Jonathan Cameron via 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>

> @@ -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;
..
This hunk does not apply to qemu/master.  Is it based on some other
change missing in this area?

I thought about collecting all this and pushing trivial-patches but
stumbled upon this one.

Thanks,

/mjt
Re: [PATCH v3 2/4] hw/cxl: Use switch statements for read and write of cachemem registers
Posted by Jonathan Cameron via 2 years, 3 months ago 
On Wed, 20 Sep 2023 08:08:39 +0300
Michael Tokarev <mjt@tls.msk.ru> wrote:

> 19.09.2023 12:34, Jonathan Cameron via 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>  
> 
> > @@ -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;  
> ..
> This hunk does not apply to qemu/master.  Is it based on some other
> change missing in this area?
> 
> I thought about collecting all this and pushing trivial-patches but
> stumbled upon this one.

See the dependencies listed in the cover letter for this set.
The HDM decoder series in particular affects the same code.

There are several series that are more urgent than this one as I'm keen
to get some features upstream as well as cleanup this cycle.
I could reorder the tree, but that would have knock on impacts on those
series. Hopefully nothing that would require re-review, but still noisy.

Thanks for considering / trying these though and for picking up
the ones that were dependency free.

Jonathan


> 
> Thanks,
> 
> /mjt
>

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.