[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and help
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions |
Date: |
Mon, 4 Mar 2013 11:44:14 +0200 |
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
> >
> >
- [Qemu-devel] [RFC PATCH v1 0/4] Data Driven device registers & Zynq DEVCFG, Peter Crosthwaite, 2013/03/03
- [Qemu-devel] [RFC PATCH v1 1/4] qemu-log: Allow checking of the current mask, Peter Crosthwaite, 2013/03/03
- [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Peter Crosthwaite, 2013/03/03
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Blue Swirl, 2013/03/03
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Blue Swirl, 2013/03/04
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Michael S. Tsirkin, 2013/03/05
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Gerd Hoffmann, 2013/03/05
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Michael S. Tsirkin, 2013/03/05
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Peter Crosthwaite, 2013/03/06
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Peter Maydell, 2013/03/06
- Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Blue Swirl, 2013/03/09
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions, Gerd Hoffmann, 2013/03/04