Re: [Qemu-devel] [PATCH v6 03/13] register: Add Memory API glue

[Peter Maydell <peter.maydell@linaro.org>]

On 12 May 2016 at 23:45, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add memory io handlers that glue the register API to the memory API.
> Just translation functions at this stage. Although it does allow for
> devices to be created without all-in-one mmio r/w handlers.
>
> This patch also adds the RegisterInfoArray struct, which allows all of
> the individual RegisterInfo structs to be grouped into a single memory
> region.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V6:
>  - Add the memory region later
> V5:
>  - Convert to using only one memory region
>
>  hw/core/register.c    | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/register.h | 50 +++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 5e6f621..25196e6 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -147,3 +147,75 @@ void register_reset(RegisterInfo *reg)
>
>      register_write_val(reg, reg->access->reset);
>  }
> +
> +static inline void register_write_memory(void *opaque, hwaddr addr,
> +                                         uint64_t value, unsigned size, bool be)
> +{
> +    RegisterInfoArray *reg_array = opaque;
> +    RegisterInfo *reg = NULL;
> +    uint64_t we = ~0;
> +    int i, shift = 0;
> +
> +    for (i = 0; i < reg_array->num_elements; i++) {
> +        if (reg_array->r[i]->access->decode.addr == addr) {
> +            reg = reg_array->r[i];
> +            break;
> +        }
> +    }
> +    assert(reg);

I'm surprised we don't support having the register array have
gaps for unimplemented/undefined registers. Presumably users
have to specify a lot of unimplemented entries ?

If you're going to assert() on undecoded addresses it would be
better to do a scan through at device init to sanity check
the register array, so missing elements are an obvious failure
rather than only showing up if the guest happens to access them.

> +
> +    /* Generate appropriate write enable mask and shift values */
> +    if (reg->data_size < size) {
> +        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
> +        shift = 8 * (be ? reg->data_size - size : 0);
> +    } else if (reg->data_size >= size) {
> +        we = MAKE_64BIT_MASK(0, size * 8);
> +    }
> +
> +    register_write(reg, value << shift, we << shift, reg_array->prefix,
> +                   reg_array->debug);
> +}
> +
> +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
> +                              unsigned size)
> +{
> +    register_write_memory(opaque, addr, value, size, true);
> +}
> +
> +
> +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
> +                              unsigned size)
> +{
> +    register_write_memory(opaque, addr, value, size, false);
> +}
> +
> +static inline uint64_t register_read_memory(void *opaque, hwaddr addr,
> +                                            unsigned size, bool be)
> +{
> +    RegisterInfoArray *reg_array = opaque;
> +    RegisterInfo *reg = NULL;
> +    int i, shift;
> +
> +    for (i = 0; i < reg_array->num_elements; i++) {
> +        if (reg_array->r[i]->access->decode.addr == addr) {
> +            reg = reg_array->r[i];
> +            break;
> +        }
> +    }
> +    assert(reg);
> +
> +    shift = 8 * (be ? reg->data_size - size : 0);
> +
> +    return (register_read(reg, reg_array->prefix, reg_array->debug) >> shift) &
> +           MAKE_64BIT_MASK(0, size * 8);

This kind of thing is reimplementing extract64().

> +}
> +
> +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return register_read_memory(opaque, addr, size, true);
> +}
> +
> +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return register_read_memory(opaque, addr, size, false);
> +}

Why do we need to handle big vs little endian separately rather
than just having the memory region say which it is and letting
the core memory system handle things appropriately ?

thanks
-- PMM