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

[Michael S. Tsirkin]

On Mon, Mar 04, 2013 at 08:52:33PM +0000, Blue Swirl wrote:
> 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 way we do this in pci is support wrappers for word/long accesses.
This is a nice way to do endian-ness conversion anyway, I guess.
If people are interested, it shouldn't be hard to generalize the pci code...

> The register model could provide unions for accessing
> different width entities though.

At least with PCI, guest can perform a long access and host word access
to the same register, so width is not a register property.

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