On Sun, Mar 03, 2013 at 09:01:11AM +0000, Blue Swirl wrote: > On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite > <peter.crosthwa...@xilinx.com> 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 <peter.crosthwa...@xilinx.com> > > --- > > > > 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 > > > >