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

[Blue Swirl]

On Mon, Mar 4, 2013 at 9:44 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Sun, Mar 03, 2013 at 09:01:11AM +0000, Blue Swirl wrote:
>> 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.
>
> One other interesting feature that pci has is migration
> sanity checks: a bit can be marked as immutable
> which means that its value must be consistent on
> migration source and destination.
> This is often the case for read-only bits - but not always,
> as bits could be set externally.
>
> Further, pci also supports a bit based allocator, so
> if you need a range of bits for a capability you
> can allocate them cleanly.

Maybe it would be useful to make these features available for other devices.

>
>> >
>> > 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.
>
> Why not uint8_t for everyone?

That would be simple, but then modeling for example 32 bit registers
gets clumsy. The register model could provide unions for accessing
different width entities though.

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