Re: [Qemu-devel] [PATCH v7 06/12] register: Add block initialise helper

[Peter Maydell]

On 22 June 2016 at 21:23, Alistair Francis <address@hidden> wrote:
> From: Peter Crosthwaite <address@hidden>
>
> Add a helper that will scan a static RegisterAccessInfo Array
> and populate a container MemoryRegion with registers as defined.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> The reason that I'm not using GArray is because the array needs to store
> the memory region that covers all of the registers.
>
> V7:
>  - Return the memory region instead of adding it as a subregion
> V6:
>  - Fixup the loop logic
> V5:
>  - Convert to only using one memory region
> V3:
>  - Fix typo
> V2:
>  - Use memory_region_add_subregion_no_print()
>
>  hw/core/register.c    | 36 ++++++++++++++++++++++++++++++++++++
>  include/hw/register.h | 24 ++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index 41c11c8..f32faac 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -224,6 +224,42 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      return extract64(read_val, 0, size * 8);
>  }
>
> +MemoryRegion *register_init_block32(DeviceState *owner,
> +                                    const RegisterAccessInfo *rae,
> +                                    int num, RegisterInfo *ri, uint32_t 
> *data,
> +                                    const MemoryRegionOps *ops,
> +                                    bool debug_enabled, uint64_t memory_size)
> +{
> +    const char *device_prefix = object_get_typename(OBJECT(owner));
> +    RegisterInfoArray *r_array = g_malloc(sizeof(RegisterInfoArray));

Prefer g_new0(RegisterInfoArray, 1)

> +    int i;
> +
> +    r_array->r = g_malloc_n(num, sizeof(RegisterInfo *));

Prefer g_new0(RegisterInfo *, num)

> +    r_array->num_elements = num;
> +    r_array->debug = debug_enabled;
> +    r_array->prefix = device_prefix;
> +
> +    for (i = 0; i < num; i++) {
> +        int index = rae[i].addr / 4;
> +        RegisterInfo *r = &ri[index];
> +
> +        *r = (RegisterInfo) {
> +            .data = &data[index],
> +            .data_size = sizeof(uint32_t),
> +            .access = &rae[i],
> +            .opaque = owner,
> +        };
> +        register_init(r);
> +
> +        r_array->r[i] = r;
> +    }
> +
> +    memory_region_init_io(&r_array->mem, OBJECT(owner), ops, r_array,
> +                          device_prefix, memory_size);
> +
> +    return &r_array->mem;

This API doesn't seem to have any provision for being able to
free the RegisterInfoArray later ? (Consider hotpluggable devices
which need to be able to free everything on hot-unplug.)
> +
>  static const TypeInfo register_info = {
>      .name  = TYPE_REGISTER,
>      .parent = TYPE_DEVICE,
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 18a63df..104d381 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -100,6 +100,8 @@ struct RegisterInfo {
>   */
>
>  struct RegisterInfoArray {
> +    MemoryRegion mem;
> +
>      int num_elements;
>      RegisterInfo **r;
>
> @@ -166,6 +168,28 @@ void register_write_memory(void *opaque, hwaddr addr, 
> uint64_t value,
>
>  uint64_t register_read_memory(void *opaque, hwaddr addr, unsigned size);
>
> +/**
> + * Init a block of registers into a container MemoryRegion. A
> + * number of constant register definitions are parsed to create a 
> corresponding
> + * array of RegisterInfo's.
> + *
> + * @owner: device owning the registers
> + * @rae: Register definitions to init
> + * @num: number of registers to init (length of @rae)
> + * @ri: Register array to init
> + * @data: Array to use for register data
> + * @ops: Memory region ops to access registers.
> + * @debug enabled: turn on/off verbose debug information

It's not clear to me which of these are input arguments to the
function, and which are things that the function initializes
(I think 'ri' is purely output?)

> + * returns: A pointer to an initalised memory region that the caller should

"initialized"

> + *          add to a container.
> + */
> +
> +MemoryRegion *register_init_block32(DeviceState *owner,
> +                                    const RegisterAccessInfo *rae,
> +                                    int num, RegisterInfo *ri, uint32_t 
> *data,
> +                                    const MemoryRegionOps *ops,
> +                                    bool debug_enabled, uint64_t 
> memory_size);
> +
>  /* Define constants for a 32 bit register */
>
>  /* This macro will define A_FOO, for the byte address of a register
> --
> 2.7.4

thanks
-- PMM