Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions

[Blue Swirl]

On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
<address@hidden> wrote:
> This struct and functions provide some encapsulation of the uint32_t type to
> make it more friendly for use as guest accessible device state. Bits of device
> state (usually MMIO registers), often have all sorts of access restrictions
> and semantics associated with them. This struct allow you to define what whose
> restrictions are on a bit-by-bit basis.

I like the approach, it could simplify devices and make them more self
documenting. Maybe devices could be also generated directly from HW
synthesis tool outputs.

How to couple this with Pins, memory API, VMState and reset handling
needs some thought.

There's some overlap also with PCI subsystem, it already implements
readonly bits.

>
> Helper functions are then used to access the uint32_t which observe the
> semantics defined by the UInt32StateInfo struct.

We also need uint8_t, uint16_t and uint64_t versions for some devices.
Perhaps it would be better to implement a uint64_t device which can be
used with shorter widths or even stronger connection with memory API.

>
> Some features:
> Bits can be marked as read_only (ro field)
> Bits can be marked as write-1-clear (w1c field)
> Bits can be marked as sticky (nw0 and nw1)
> Reset values can be defined (reset)
> Bits can be marked to throw guest errors when written certain values (ge0, 
> ge1)

Other bits could be marked as unimplemented (LOG_UNIMP).

> Bits can be marked clear on read (cor)
> Regsiters can be truncated in width (width)

s/Regsiters/Registers/

>
> Useful for defining device register spaces in a data driven way. Cuts down on 
> a
> lot of the verbosity and repetition in the switch-case blocks in the standard
> foo_mmio_read/write functions.

For maximum flexibility, a callback could be specified but then we
overlap memory API.

Once we have Pin available, it could be useful to couple a register
bit directly with a Pin. Currently we could use qemu_irq. This would
mean that the dynamic state would need to be more complex than just
uint32_t. Also Pin could implement some of this functionality.

>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>
>  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>  util/bitops.c         |   71 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 0 deletions(-)
>
> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
> index affcc96..8ad0290 100644
> --- a/include/qemu/bitops.h
> +++ b/include/qemu/bitops.h

I think this is not the right place since this is very much HW
specific, please introduce a new file.

> @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int 
> start, int length,
>      return (value & ~mask) | ((fieldval << start) & mask);
>  }
>
> +/**
> + * A descriptor for a Uint32 that is part of guest accessible device state
> + * @ro: whether or not the bit is read-only state comming out of reset

s/comming/coming/

> + * @w1c: bits with the common write 1 to clear semantic.
> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1)

s/cant/can't/, also below

> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0)
> + * @reset: reset value.
> + * @ge1: Bits that when written 1 indicate a guest error
> + * @ge0: Bits that when written 0 indicate a guest error
> + * @cor: Bits that are clear on read
> + * @width: width of the uint32t. Only the @width least significant bits are
> + * valid. All others are silent Read-as-reset/WI.
> + */
> +
> +typedef struct UInt32StateInfo {
> +    const char *name;
> +    uint32_t ro;
> +    uint32_t w1c;
> +    uint32_t nw0;
> +    uint32_t nw1;
> +    uint32_t reset;
> +    uint32_t ge1;
> +    uint32_t ge0;
> +    uint32_t cor;
> +    int width;
> +} UInt32StateInfo;
> +
> +/**
> + * reset an array of u32s
> + * @array: array of u32s to reset
> + * @info: corresponding array of UInt32StateInfos to get reset values from
> + * @num: number of values to reset
> + */
> +
> +void uint32_array_reset(uint32_t *array, const UInt32StateInfo *info, int 
> num);
> +
> +/**
> + * write a value to a uint32_t subject to its restrictions
> + * @state: Pointer to location to be written
> + * @info: Definition of variable
> + * @val: value to write
> + * @prefix: Debug and error message prefix
> + * @debug: Turn on noisy debug printfery
> + */
> +
> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> +                  const char *prefix, bool debug);

Prefix could be part of the structure. I'd also combine state and
info, and avoid passing debug flag directly (it could be in the
dynamic structure or a pointer). Then it should be easy to be
compatible with memory API.

> +
> +/**
> + * write a value from a uint32_t subject to its restrictions

read

> + * @state: Pointer to location to be read
> + * @info: Definition of variable
> + * @prefix: Debug and error message prefix
> + * @debug: Turn on noisy debug printfery
> + */
> +
> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> +                     const char *prefix, bool debug);
> +
>  #endif
> diff --git a/util/bitops.c b/util/bitops.c
> index e72237a..51db476 100644
> --- a/util/bitops.c
> +++ b/util/bitops.c
> @@ -11,6 +11,8 @@
>   * 2 of the License, or (at your option) any later version.
>   */
>
> +#include "qemu-common.h"
> +#include "qemu/log.h"
>  #include "qemu/bitops.h"
>
>  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
> @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, 
> unsigned long size)
>      /* Not found */
>      return size;
>  }
> +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int 
> num)
> +{
> +    int i = 0;
> +
> +    for (i = 0; i < num; ++i) {
> +        state[i] = info[i].reset;
> +    }
> +}

Perhaps this could be automatically registered.

> +
> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t val,
> +                  const char *prefix, bool debug)
> +{
> +    int i;
> +    uint32_t new_val;
> +    int width = info->width ? info->width : 32;
> +
> +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
> +                        ~((1ull << width) - 1);
> +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
> +                        ~((1ull << width) - 1);
> +
> +    if (!info->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> +                      "(written value: %#08x)\n", prefix, val);
> +        return;
> +    }
> +
> +    if (debug) {
> +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
> +                val);
> +    }
> +
> +    /*FIXME: skip over if no LOG_GUEST_ERROR */
> +    for (i = 0; i <= 1; ++i) {
> +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
> +        if (test) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be 
> written"
> +                          " to %d\n", prefix, info->name, test, i);
> +        }
> +    }
> +
> +    new_val = val & ~(no_w1_mask & val);
> +    new_val |= no_w1_mask & *state & val;
> +    new_val |= no_w0_mask & *state & ~val;
> +    new_val &= ~(val & info->w1c);
> +    *state = new_val;
> +}
> +
> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
> +                     const char *prefix, bool debug)
> +{
> +    uint32_t ret = *state;
> +
> +    /* clear on read */
> +    ret &= ~info->cor;
> +    *state = ret;
> +
> +    if (!info->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device state 
> "
> +                      "(read value: %#08x)\n", prefix, ret);
> +        return ret;
> +    }
> +
> +    if (debug) {
> +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, 
> ret);
> +    }
> +
> +    return ret;
> +}
> --
> 1.7.0.4
>
>