On Fri, Sep 15, 2023 at 06:04:16PM +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>
> v2: Use switch statements. Note we coudl have renamed registers to make
> the sizes visible as is true of some other cases, but that felt odd
> as this is the only size these registers can be, so why call it out.
> The asserts do the same job.
> 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
>
