[v6,03/13] 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>
---
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(+)

Hi Alistair,

Le 13/05/2016 à 00:45, Alistair Francis a écrit :
> 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);
> +
> +    /* 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);
> +}
> +
> +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);
> +}
> diff --git a/include/hw/register.h b/include/hw/register.h
> index 07d0616..786707b 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -15,6 +15,7 @@
>
>  typedef struct RegisterInfo RegisterInfo;
>  typedef struct RegisterAccessInfo RegisterAccessInfo;
> +typedef struct RegisterInfoArray RegisterInfoArray;
>
>  /**
>   * Access description for a register that is part of guest accessible device
> @@ -51,6 +52,10 @@ struct RegisterAccessInfo {
>      void (*post_write)(RegisterInfo *reg, uint64_t val);
>
>      uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
> +
> +    struct {
> +        hwaddr addr;
> +    } decode;

Is there any reason why there is a struct here?

Fred

>  };
>
>  /**
> @@ -79,6 +84,25 @@ struct RegisterInfo {
>  };
>
>  /**
> + * This structure is used to group all of the individual registers which are
> + * modeled using the RegisterInfo strucutre.
> + *
> + * @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;
> +};
> +
> +/**
>   * write a value to a register, subject to its restrictions
>   * @reg: register to write to
>   * @val: value to write
> @@ -107,4 +131,30 @@ uint64_t register_read(RegisterInfo *reg, const char* prefix, bool debug);
>
>  void register_reset(RegisterInfo *reg);
>
> +/**
> + * Memory API MMIO write handler that will write to a Register API register.
> + *  _be for big endian variant and _le for little endian.
> + * @opaque: RegisterInfo to write to
> + * @addr: Address to write
> + * @value: Value to write
> + * @size: Number of bytes to write
> + */
> +
> +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value,
> +                              unsigned size);
> +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value,
> +                              unsigned size);
> +
> +/**
> + * Memory API MMIO read handler that will read from a Register API register.
> + *  _be for big endian variant and _le for little endian.
> + * @opaque: RegisterInfo to read from
> + * @addr: Address to read
> + * @size: Number of bytes to read
> + * returns: Value read from register
> + */
> +
> +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size);
> +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size);
> +
>  #endif
>

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

On Thu, Jun 9, 2016 at 12:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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.

You're right, this is a little harsh. I'm thinking I'll change it to a
qemu_log() (unimplemented, although it also could be a guest error)
and remove the assert().

>
>> +
>> +    /* 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().

Ok, I'll update it to use 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 ?

I didn't realise that is an option. So I can remove all the endianess
handling from here and the core will handle it?

Thanks,

Alistair

>
> thanks
> -- PMM
>

On 21 June 2016 at 01:46, Alistair Francis <alistair.francis@xilinx.com> wrote:
> On Thu, Jun 9, 2016 at 12:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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 ?
>
> I didn't realise that is an option. So I can remove all the endianess
> handling from here and the core will handle it?

It should do, though you should test that it behaves the way you
expect.

thanks
-- PMM

On Thu, Jun 9, 2016 at 6:08 AM, KONRAD Frederic
<fred.konrad@greensocs.com> wrote:
> Hi Alistair,
>
>
> Le 13/05/2016 à 00:45, Alistair Francis a écrit :
>>
>> 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);
>> +
>> +    /* 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);
>> +}
>> +
>> +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);
>> +}
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> index 07d0616..786707b 100644
>> --- a/include/hw/register.h
>> +++ b/include/hw/register.h
>> @@ -15,6 +15,7 @@
>>
>>  typedef struct RegisterInfo RegisterInfo;
>>  typedef struct RegisterAccessInfo RegisterAccessInfo;
>> +typedef struct RegisterInfoArray RegisterInfoArray;
>>
>>  /**
>>   * Access description for a register that is part of guest accessible
>> device
>> @@ -51,6 +52,10 @@ struct RegisterAccessInfo {
>>      void (*post_write)(RegisterInfo *reg, uint64_t val);
>>
>>      uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
>> +
>> +    struct {
>> +        hwaddr addr;
>> +    } decode;
>
>
> Is there any reason why there is a struct here?

I think it is just left over, I have removed it.

Thanks,

Alistair

>
> Fred