On Tue, 3 Oct 2023 16:42:37 -0400
"Michael S. Tsirkin" <m...@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 <m...@tls.msk.ru>
> > Signed-off-by: Jonathan Cameron <jonathan.came...@huawei.com>
> > Reviewed-by: Fan Ni <fan...@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
>
