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

[Michael S. Tsirkin]

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.

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

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