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

[Alistair Francis]

On Thu, Jun 23, 2016 at 5:55 AM, Peter Maydell <address@hidden> wrote:
> 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)

Ok, I have fixed these.

>
>> +    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.)

I have added a finalise function which can be used to free the register array.

This meant I had to change register_init_block32() to return
RegisterInfoArray instead of a MemoryRegion.

>> +
>>  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?)

The function expects everything to already be initialised, it just
wires them up.

ri should already be allocated, this function just sets up some pointers to it.

I added a comment in the description saying that ri and data must
already be allocated. Is that enough?

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

Fixed.

Thanks,

Alistair

>
>> + *          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
>