[v7,03/12] register: Add Memory API glue

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>
---
V7:
 - Remove endianess handling
 - Remove assert() and log missing registers
V6:
 - Add the memory region later
V5:
 - Convert to using only one memory region

 hw/core/register.c    | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

On 22 June 2016 at 21:23, 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>
> ---
> V7:
>  - Remove endianess handling
>  - Remove assert() and log missing registers
> V6:
>  - Add the memory region later
> V5:
>  - Convert to using only one memory region
>
>  hw/core/register.c    | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index cc067f1..149aebb 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg)
>
>      register_write_val(reg, reg->access->reset);
>  }
> +
> +void register_write_memory(void *opaque, hwaddr addr,
> +                           uint64_t value, unsigned size)
> +{
> +    RegisterInfoArray *reg_array = opaque;
> +    RegisterInfo *reg = NULL;
> +    uint64_t we = ~0;

Really ~0 and not ~0ULL ? In any case this shouldn't need
initializing as we will always set it later.

> +    int i;
> +
> +    for (i = 0; i < reg_array->num_elements; i++) {
> +        if (reg_array->r[i]->access->addr == addr) {
> +            reg = reg_array->r[i];
> +            break;
> +        }
> +    }
> +
> +    if (!reg) {
> +        qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \
> +                      "address: %#" PRIx64 "\n", addr);

Shouldn't this be a LOG_GUEST_ERROR ?

> +        return;
> +    }
> +
> +    /* Generate appropriate write enable mask */
> +    if (reg->data_size < size) {
> +        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
> +    } else if (reg->data_size >= size) {

Why not just "else {" ?

> +        we = MAKE_64BIT_MASK(0, size * 8);
> +    }
> +
> +    register_write(reg, value, we, reg_array->prefix,
> +                   reg_array->debug);
> +}
> +
> +uint64_t register_read_memory(void *opaque, hwaddr addr,
> +                              unsigned size)
> +{
> +    RegisterInfoArray *reg_array = opaque;
> +    RegisterInfo *reg = NULL;
> +    uint64_t read_val;
> +    int i;
> +
> +    for (i = 0; i < reg_array->num_elements; i++) {
> +        if (reg_array->r[i]->access->addr == addr) {
> +            reg = reg_array->r[i];
> +            break;
> +        }
> +    }
> +
> +    if (!reg) {
> +        qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \
> +                      "address: %#" PRIx64 "\n", addr);
> +        return 0;
> +    }
> +
> +    read_val = register_read(reg, reg_array->prefix, reg_array->debug);

This will silently affect bits of the register outside the
specified size with clear-on-read or other "reads have
side effects" behaviour, which isn't consistent with how
we handle writes.

>  /**
> + * This structure is used to group all of the individual registers which are
> + * modeled using the RegisterInfo strucutre.

"structure"

> + *
> + * @r is an aray containing of all the relevent RegisterInfo structures.
> + *
> + * @num_elements is the number of elements in the array r
> + *
> + * @mem: optional Memory region for the register
> + */
> +
> +struct RegisterInfoArray {
> +    int num_elements;
> +    RegisterInfo **r;
> +
> +    bool debug;
> +    const char *prefix;
> +};
> +

thanks
-- PMM

On Thu, Jun 23, 2016 at 5:21 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 June 2016 at 21:23, 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>
>> ---
>> V7:
>>  - Remove endianess handling
>>  - Remove assert() and log missing registers
>> V6:
>>  - Add the memory region later
>> V5:
>>  - Convert to using only one memory region
>>
>>  hw/core/register.c    | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/register.h | 43 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 101 insertions(+)
>>
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> index cc067f1..149aebb 100644
>> --- a/hw/core/register.c
>> +++ b/hw/core/register.c
>> @@ -154,3 +154,61 @@ void register_reset(RegisterInfo *reg)
>>
>>      register_write_val(reg, reg->access->reset);
>>  }
>> +
>> +void register_write_memory(void *opaque, hwaddr addr,
>> +                           uint64_t value, unsigned size)
>> +{
>> +    RegisterInfoArray *reg_array = opaque;
>> +    RegisterInfo *reg = NULL;
>> +    uint64_t we = ~0;
>
> Really ~0 and not ~0ULL ? In any case this shouldn't need
> initializing as we will always set it later.
>
>> +    int i;
>> +
>> +    for (i = 0; i < reg_array->num_elements; i++) {
>> +        if (reg_array->r[i]->access->addr == addr) {
>> +            reg = reg_array->r[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!reg) {
>> +        qemu_log_mask(LOG_UNIMP, "Write to unimplemented register at " \
>> +                      "address: %#" PRIx64 "\n", addr);
>
> Shouldn't this be a LOG_GUEST_ERROR ?
>
>> +        return;
>> +    }
>> +
>> +    /* Generate appropriate write enable mask */
>> +    if (reg->data_size < size) {
>> +        we = MAKE_64BIT_MASK(0, reg->data_size * 8);
>> +    } else if (reg->data_size >= size) {
>
> Why not just "else {" ?
>
>> +        we = MAKE_64BIT_MASK(0, size * 8);
>> +    }
>> +
>> +    register_write(reg, value, we, reg_array->prefix,
>> +                   reg_array->debug);
>> +}
>> +
>> +uint64_t register_read_memory(void *opaque, hwaddr addr,
>> +                              unsigned size)
>> +{
>> +    RegisterInfoArray *reg_array = opaque;
>> +    RegisterInfo *reg = NULL;
>> +    uint64_t read_val;
>> +    int i;
>> +
>> +    for (i = 0; i < reg_array->num_elements; i++) {
>> +        if (reg_array->r[i]->access->addr == addr) {
>> +            reg = reg_array->r[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!reg) {
>> +        qemu_log_mask(LOG_UNIMP, "Read to unimplemented register at " \
>> +                      "address: %#" PRIx64 "\n", addr);
>> +        return 0;
>> +    }
>> +
>> +    read_val = register_read(reg, reg_array->prefix, reg_array->debug);
>
> This will silently affect bits of the register outside the
> specified size with clear-on-read or other "reads have
> side effects" behaviour, which isn't consistent with how
> we handle writes.
>
>>  /**
>> + * This structure is used to group all of the individual registers which are
>> + * modeled using the RegisterInfo strucutre.
>
> "structure"

Fixed all.

Thanks,

Alistair

>
>> + *
>> + * @r is an aray containing of all the relevent RegisterInfo structures.
>> + *
>> + * @num_elements is the number of elements in the array r
>> + *
>> + * @mem: optional Memory region for the register
>> + */
>> +
>> +struct RegisterInfoArray {
>> +    int num_elements;
>> +    RegisterInfo **r;
>> +
>> +    bool debug;
>> +    const char *prefix;
>> +};
>> +
>
> thanks
> -- PMM
>