Re: [PATCH 2/4] hw/cxl: Use available size parameter to index into register arrays.

[Michael Tokarev]

13.09.2023 18:05, Jonathan Cameron via wrote:
> Indexing has to be done into an array with the right size elements.
> As such, the size parameter always matches the array element size
> and can be used in place of the longer sizeof(*array)
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>   hw/cxl/cxl-component-utils.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index f3bbf0fd13..089e10b232 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -76,7 +76,7 @@ static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr 
> offset,
>       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)];
> +        return cregs->cache_mem_registers[offset / size];

This is a though one, and smells wrong.

Though because it is not at all obvious where this "size" value comes from,
have to find usage(s) of this function (cache_mem_ops) and think twice about
the other parameters in there.  Also having in mind the previous comparison
with 8.  In this part of the code, size should always be =4, but it takes
hard time to figure this out.

Wrong - no, because of the above - the only 2 possible values are 4 and 8,
but it's difficult to see what's going on, and you're making it worse.

Original code was at least clear you're getting a single register from
an array of registers, with new code it is not clear at all.

What I'd probably use here is to add comment that size can be either 4 or 8,
and use a switch similar to what you've used in first patch in this series.
And have a static_assert(sizeof(register) == 4) or something like that
here in this second branch.

So it is something like:

static uint64_t cxl_cache_mem_read_reg(void *opaque, hwaddr offset,
                                       unsigned size)
{
    CXLComponentState *cxl_cstate = opaque;
    ComponentRegisters *cregs = &cxl_cstate->crb;

    switch (size) {
    case 8:
        qemu_log_mask(LOG_UNIMP,
                      "CXL 8 byte cache mem registers not implemented\n");
        return 0;

    case 4:
        if (cregs->special_ops && cregs->special_ops->read) {
            return cregs->special_ops->read(cxl_cstate, offset, 4);
        } else {
            return cregs->cache_mem_registers[offset /
                                              
sizeof(*cregs->cache_mem_registers)];
        }

    default:
        /* this routine is called with size being either 4 or 8 only */
        g_assert_not_reached();
    }
}

Note: I especially left the sizeof() here, instead of using a previously
suggested static_assert() - because a register can be implemented using
larger integers on the host, it does not need to be 4 bytes, - but only
low 4 bytes can actually be used.

This does not shorten the line (it does by wrapping it up), but it keep
code correct and more understandable.  Adding size parameter there makes
it much more cryptic.

Here and in other places.

This is just an example, not a suggestion.

/mjt