| Message ID | 11e3c93f4c64bda4ef46ec9de8dd8b4fa88b3ec5.1362290776.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite <peter.crosthwaite@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. > > 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. > > 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.crosthwaite@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 > >
Hi Blue, Thanks for the review. Comments in-line below. Are you on the IRC much and what timezone? I'd like to ask a few questions about how you see this fitting with the memory API, that would probably go much faster in live discussion. I've commented on the issue below with my current thoughts. On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite > <peter.crosthwaite@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. > Nice idea. Or at least a template device that reduces the coding to a few /* YOUR CODE HERE */ is another possibility. > How to couple this with Pins, memory API, I was actually quite deliberate in making this de-coupled from the memory API, The idea is any device that has guest accessible state can use this, not just SysBus/PCI/MMIO. Even though the 90% use case is making arrays of these definitions for memory mapped register files, its usable for any software accessible device state. The ARM co-processor interface is a good example of this. A random SPI or I2C device may also have some registers that are accessed indirectly over their respective transport layers. It'd be nice to be able to use this API independent of the memory API in those cases. That said if you can see any way to modify the API to make it play nicer with the Memory API I'd definitely take it onboard. > VMState and reset handling > needs some thought. > If we add VMState and reset then we are only one step away from these registers being a QOM object (TYPE_DEVICE) in their own right. Might not be a bad thing but does strike me as heavy-weight. It is a good question where do we draw the line between the responsibilities of the device and the API. Here im am shooting for the grey area between full QOM device for every register and the status quo (no API at all). Regarding VMState, the current implementation the devices dynamic state is still limited to a uint32_t array so a single VMSTATE_UINT32_ARRAY will do the job. > There's some overlap also with PCI subsystem, it already implements > readonly bits. > >> >> 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. > Yes. Going lowest common denominator with uint64_t does however cost more memory and VMS storage which could impact devices with large register spaces. This wouldn't impact any of my devices in any way though. The performance hit is probably worth it for the simplicity. V2 will be 64 bit. I will drop the "64" from the function name to keep it generic and we can assert that no one tries to create a register with .width > 64. >> >> 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). > Agreed. Will add to V2. Another Idea id like to play with is adding per-bit explanations for the LOG_UNIMP and LOG_GUEST_ERROR. Just arrays of const char * in the definitions that cat onto the qemu_log msgs telling you why that was a guest error or exactly what feature is unimplemented. >> Bits can be marked clear on read (cor) >> Regsiters can be truncated in width (width) > > s/Regsiters/Registers/ > Will fix. >> >> 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. > I think this is a good idea, but continuing on the theme of what this API is trying to achieve I think there should be capability for per-bit function definitions. On the topic I think Gerd has played with the idea of per-register callbacks for some device models in the past. > Once we have Pin available, it could be useful to couple a register > bit directly with a Pin. Currently we could use qemu_irq. Ill wait on Pin to land and stabilise for this one and continue to use code driven GPIO setters. > 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.crosthwaite@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/ > Will fix >> + * @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 > Will fix >> + * @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. Prefix is an awkward one as its only common to a single device. E.G. My first UART controller could have the prefix "UART1" while the second has "UART2". Currently the property info here are created as static const. To fold prefix into info each device would have to clone a template info and then populate prefix at object initialize time. I am currently just using the object canonical path strcatted with the register offset for this as that gives a unique per-device prefix rather than per-device-type. > I'd also combine state and > info, Similar problem. info would need to be per-device and no longer shared. Not such a bad thing though. > and avoid passing debug flag directly (it could be in the > dynamic structure or a pointer). Debug as used in its current form could actually live in the static structure. I'm just thinking if there are any nice ways to set that up such that you don't have to add ".debug = FOO_DEBUG" to every table entry. Although if we end up de-constifying the table to fold in the prefix and state (as discussed above) then a helper function can just iterate over the table and sort it out in one go. Any by now you've convinced me :) Ill add some per-instance state in V2 to store this stuff. The const table example I've given in P3 will be unchanged. It will reduce this function prototype to two args (info and val). There will a few new this and that helpers that object init fns can call to set this up in a handful of lines. > Then it should be easy to be > compatible with memory API. > >> + >> +/** >> + * write a value from a uint32_t subject to its restrictions > > read > Will fix. Regards, Peter >> + * @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 >> >> >
> +/** > + * 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 > + * @w1c: bits with the common write 1 to clear semantic. > + * @nw0: bits that cant be written with a 0 by the guest (sticky 1) > + * @nw1: bits that cant be written with a 1 by the guest (sticky 0) Why these? reset=0 + ro=1 equals nw1=1 reset=1 + ro=1 equals nw0=1 > + * @width: width of the uint32t. Only the @width least significant bits are > + * valid. All others are silent Read-as-reset/WI. That's kida redundant with "ro" too. I'd do it the other way around btw: Specify the writable bits instead of the read-only ones. width=8 easily be written as wmask=0xff then. cheers, Gerd
Hi Gerd, On Mon, Mar 4, 2013 at 4:55 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: >> +/** >> + * 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 >> + * @w1c: bits with the common write 1 to clear semantic. > >> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1) >> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0) > > Why these? > > reset=0 + ro=1 equals nw1=1 > reset=1 + ro=1 equals nw0=1 > Some bits in my devices can only be written one-way they are either "stuck on" once they are written, or only the hardware is allowed to write the other way. Setting both nw0 and nw1 is equivalent to ro. In the devcfg example, bits in the LOCK bits are sticky (the idea is once the firmware locks them the booted system cant unlock them for security and we want to test the firmware as a guest). >> + * @width: width of the uint32t. Only the @width least significant bits are >> + * valid. All others are silent Read-as-reset/WI. > > That's kida redundant with "ro" too. I'd do it the other way around > btw: Specify the writable bits instead of the read-only ones. width=8 > easily be written as wmask=0xff then. In V2 ill be doing some work to make this more flexible regarding widths to address Blues comments. Ill incorporate this as well, and I think I can make it work getting rid of the width altogether. Regards, Peter > > cheers, > Gerd > >
Hi, >> For maximum flexibility, a callback could be specified but then we >> overlap memory API. > > I think this is a good idea, but continuing on the theme of what this > API is trying to achieve I think there should be capability for > per-bit function definitions. On the topic I think Gerd has played > with the idea of per-register callbacks for some device models in the > past. hw/intel-hda.c is it. Predates memory api though and thus doesn't integrated with it, although it makes sense of course. Could be as simple as a generic MemoryRegionOps. I'm not sure it is that useful to have per-bit callbacks. For "normal" devices you rarely need that, and for gpio arrays probably want more than just callbacks per bit (qemu_irqs for example). cheers, Gerd
Hi, >>> + * @nw0: bits that cant be written with a 0 by the guest (sticky 1) >>> + * @nw1: bits that cant be written with a 1 by the guest (sticky 0) >> >> Why these? >> >> reset=0 + ro=1 equals nw1=1 >> reset=1 + ro=1 equals nw0=1 >> > > Some bits in my devices can only be written one-way they are either > "stuck on" once they are written, or only the hardware is allowed to > write the other way. Ah, ok. Makes sense. Can you make the description a bit more verbose to clarify this? cheers, Gerd
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.crosthwaite@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.crosthwaite@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 > > > >
On Mon, Mar 4, 2013 at 1:37 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Hi Blue, > > Thanks for the review. Comments in-line below. Are you on the IRC much > and what timezone? I'd like to ask a few questions about how you see > this fitting with the memory API, that would probably go much faster > in live discussion. I've commented on the issue below with my current > thoughts. > > On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote: >> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite >> <peter.crosthwaite@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. >> > > Nice idea. Or at least a template device that reduces the coding to a > few /* YOUR CODE HERE */ is another possibility. > >> How to couple this with Pins, memory API, > > I was actually quite deliberate in making this de-coupled from the > memory API, The idea is any device that has guest accessible state can > use this, not just SysBus/PCI/MMIO. Even though the 90% use case is > making arrays of these definitions for memory mapped register files, > its usable for any software accessible device state. The ARM > co-processor interface is a good example of this. A random SPI or I2C > device may also have some registers that are accessed indirectly over > their respective transport layers. It'd be nice to be able to use this > API independent of the memory API in those cases. That said if you can > see any way to modify the API to make it play nicer with the Memory > API I'd definitely take it onboard. I think Avi mentioned that that memory API should be operating at single register level one day and your model looks like we could use that as one step. Perhaps the best of all worlds would be to keep things compatible with memory API so that it can be used if needed. > >> VMState and reset handling >> needs some thought. >> > > If we add VMState and reset then we are only one step away from these > registers being a QOM object (TYPE_DEVICE) in their own right. Might > not be a bad thing but does strike me as heavy-weight. It is a good > question where do we draw the line between the responsibilities of the > device and the API. Here im am shooting for the grey area between full > QOM device for every register and the status quo (no API at all). > Regarding VMState, the current implementation the devices dynamic > state is still limited to a uint32_t array so a single > VMSTATE_UINT32_ARRAY will do the job. Yes, QOM and VMState probably model bigger entities than single register entries. Specifying everything in one place would be nice though. > >> There's some overlap also with PCI subsystem, it already implements >> readonly bits. >> >>> >>> 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. >> > > Yes. Going lowest common denominator with uint64_t does however cost > more memory and VMS storage which could impact devices with large > register spaces. This wouldn't impact any of my devices in any way > though. The performance hit is probably worth it for the simplicity. > V2 will be 64 bit. I will drop the "64" from the function name to keep > it generic and we can assert that no one tries to create a register > with .width > 64. > >>> >>> 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). >> > > Agreed. Will add to V2. Another Idea id like to play with is adding > per-bit explanations for the LOG_UNIMP and LOG_GUEST_ERROR. Just > arrays of const char * in the definitions that cat onto the qemu_log > msgs telling you why that was a guest error or exactly what feature is > unimplemented. > >>> Bits can be marked clear on read (cor) >>> Regsiters can be truncated in width (width) >> >> s/Regsiters/Registers/ >> > > Will fix. > >>> >>> 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. >> > > I think this is a good idea, but continuing on the theme of what this > API is trying to achieve I think there should be capability for > per-bit function definitions. On the topic I think Gerd has played > with the idea of per-register callbacks for some device models in the > past. > >> Once we have Pin available, it could be useful to couple a register >> bit directly with a Pin. Currently we could use qemu_irq. > > Ill wait on Pin to land and stabilise for this one and continue to use > code driven GPIO setters. > >> 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.crosthwaite@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/ >> > > Will fix > >>> + * @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 >> > > Will fix > >>> + * @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. > > Prefix is an awkward one as its only common to a single device. E.G. > My first UART controller could have the prefix "UART1" while the > second has "UART2". Currently the property info here are created as > static const. To fold prefix into info each device would have to clone > a template info and then populate prefix at object initialize time. I > am currently just using the object canonical path strcatted with the > register offset for this as that gives a unique per-device prefix > rather than per-device-type. I think this is asking that the prefix should be handled at device level and register objects should be always part of a parent device object so that register methods can always query parent device name. > >> I'd also combine state and >> info, > > Similar problem. info would need to be per-device and no longer > shared. Not such a bad thing though. > >> and avoid passing debug flag directly (it could be in the >> dynamic structure or a pointer). > > Debug as used in its current form could actually live in the static > structure. I'm just thinking if there are any nice ways to set that up > such that you don't have to add ".debug = FOO_DEBUG" to every table > entry. Although if we end up de-constifying the table to fold in the > prefix and state (as discussed above) then a helper function can just > iterate over the table and sort it out in one go. The dynamic structure could have a pointer to the static const struct. > > Any by now you've convinced me :) Ill add some per-instance state in > V2 to store this stuff. The const table example I've given in P3 will > be unchanged. It will reduce this function prototype to two args (info > and val). There will a few new this and that helpers that object init > fns can call to set this up in a handful of lines. > >> Then it should be easy to be >> compatible with memory API. >> >>> + >>> +/** >>> + * write a value from a uint32_t subject to its restrictions >> >> read >> > > Will fix. > > Regards, > Peter > >>> + * @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 >>> >>> >>
On Mon, Mar 4, 2013 at 9:44 AM, Michael S. Tsirkin <mst@redhat.com> 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 >> <peter.crosthwaite@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. 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 <peter.crosthwaite@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 >> > >> >
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 <mst@redhat.com> 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 > >> <peter.crosthwaite@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. > > 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 <peter.crosthwaite@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 > >> > > >> >
Hi, >>>> 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... > 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. Thanks, but I'm not interested. Memory API handles this just fine for me, and there is zero reason to care about how the guests accesses the registers (unless the hardware you are emulating behaves strange enough that you have to care to get it right). cheers, Gerd
On Tue, Mar 05, 2013 at 03:17:13PM +0100, Gerd Hoffmann wrote: > Hi, > > >>>> 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... > > > 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. > > Thanks, but I'm not interested. > > Memory API handles this just fine for me, and there is zero reason to > care about how the guests accesses the registers (unless the hardware > you are emulating behaves strange enough that you have to care to get it > right). > > cheers, > Gerd If the intended audience uses the memory API, then it's not needed. pci config is not going through a memory API ATM though I think it might be possible to make it do this by creating a separate config address space per device.
Hi All,
I have brought myself up to speed with this PCI stuff. First of all I
am assuming you are talking about the PCI Config space only? This
(hw/pci.h):
struct PCIDevice {
DeviceState qdev;
/* PCI config space */
uint8_t *config;
[snip]
/* Used to implement RW1C(Write 1 to Clear) bytes */
uint8_t *w1cmask;
/* Used to allocate config space for capabilities. */
uint8_t *used;
On Wed, Mar 6, 2013 at 12:32 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Mar 05, 2013 at 03:17:13PM +0100, Gerd Hoffmann wrote:
>> Hi,
>>
>> >>>> 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...
So the issue I have with the uint8_t based defintion + accessor is you
need to use code driven to set all the properties (w1cmask, used and
friends). eg (hw/pci/shpc.c):
pci_set_byte(shpc->wmask + SHPC_CMD_CODE, 0xff);
pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX);
pci_set_byte(shpc->wmask + SHPC_CMD_TRGT, SHPC_CMD_TRGT_MAX);
I'm trying to break that, with table driven instantiation of this
information. Check patch 3 of this series for the guinea pig example
that does this, short except below:
+static const UInt32StateInfo xilinx_devcfg_regs_info[] = {
+ [R_CTRL] = { .name = "CTRL",
+ .reset = PCAP_PR | PCAP_MODE | 0x3 << 13,
+ .ro = 0x107f6000 | USER_MODE,
+ .ge0 = 0x3 << 13,
+ },
+ [R_LOCK] = { .name = "LOCK", .width = 5, .nw0 = ~0 },
+ [R_CFG] = { .name = "CFG",
+ .width = 12,
+ .reset = 1 << RFIFO_TH_SHIFT | 1 << WFIFO_TH_SHIFT | 0x8,
+ .ge1 = 0x7,
+ .ge0 = 0x8,
+ .ro = 0x00f,
+ },
This philosophy also breaks the parallel array approach adopted by
PCI, so to share with PCI that will require some major earthworks PCI
side.
>>
>> > 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.
>>
Guest access is the easiest part as you can use wrappers/accessors
that translate memory-API to uint8_t. But for hardware side access its
nice to be able to bang on the raw uint32_t[]. E.G. (from next patch):
+ DB_PRINT("dma operation finished\n");
+ s->regs[R_INT_STS] |= DMA_DONE_INT | DMA_P_DONE_INT;
This code raises the interrupt from the hardware side by setting a
register bit directly. It would be tedious (and an enourmous tree-wide
change pattern) to have the change this over to use a wrapping
accessor. My goal here is that only guest side access are regulated by
this API - hardware has free reign.
>> Thanks, but I'm not interested.
>>
>> Memory API handles this just fine for me, and there is zero reason to
>> care about how the guests accesses the registers (unless the hardware
>> you are emulating behaves strange enough that you have to care to get it
>> right).
>>
So in V2 I want to use something close to the memory API in interface,
so there is only trivial glue on the device level between the memory
API callback and this API.
>> cheers,
>> Gerd
>
> If the intended audience uses the memory API, then it's not needed.
Not 100% accurate. My goal here it to control (or wrap) only guest
accesses, in the first instance via the Memory API, but other forms of
guest access are perfectly valid as well, and PCI config space, would
be a good example. If we are going to share code however, we will need
to make changes PCI side, as that uint8_t with compulsory accessors is
too verbose to be used in devices.
I have a solution that may work for everyone however. Leave the
storage format (uint32_t, uint8_t etc) up to the client. In PCI config
this is the uint8_t *config as it stands today. In my device its
uint32_t *regs. The client then casts the accessed area to uint8_t and
is responsible for defining the endianness policy via the accompanying
Uint32StateInfo struct. For PCI, this is PCIs endianness. For my
device this is just the endianness of the host machine, as thats how
the hardware side accesses (eg s->regs[R_INT_STS] |= DMA_DONE_INT;)
will operate.
Might be easier to describe as patches rather than talk though.
Regards,
Peter
> pci config is not going through a memory API ATM though
> I think it might be possible to make it do this by creating a separate
> config address space per device.
>
> --
> MST
>
On 7 March 2013 10:00, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > Not 100% accurate. My goal here it to control (or wrap) only guest > accesses, in the first instance via the Memory API, but other forms of > guest access are perfectly valid as well, and PCI config space, would > be a good example. If we are going to share code however, we will need > to make changes PCI side, as that uint8_t with compulsory accessors is > too verbose to be used in devices. We should just use the memory API. If other bits of QEMU like PCI don't care to update their interfaces to be memory API then they don't get to use your new features. I don't see any point in defining a new interface at a level below the memory API for their benefit. -- PMM
On Thu, Mar 7, 2013 at 2:26 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 7 March 2013 10:00, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> Not 100% accurate. My goal here it to control (or wrap) only guest >> accesses, in the first instance via the Memory API, but other forms of >> guest access are perfectly valid as well, and PCI config space, would >> be a good example. If we are going to share code however, we will need >> to make changes PCI side, as that uint8_t with compulsory accessors is >> too verbose to be used in devices. > > We should just use the memory API. If other bits of QEMU like PCI > don't care to update their interfaces to be memory API then they > don't get to use your new features. I don't see any point in defining > a new interface at a level below the memory API for their benefit. I'm also leaning towards memory API approach, it should be better in the long term since all devices should use that sooner or later. > > -- PMM
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 @@ -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 + * @w1c: bits with the common write 1 to clear semantic. + * @nw0: bits that cant be written with a 0 by the guest (sticky 1) + * @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); + +/** + * write a value from a uint32_t subject to its restrictions + * @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; + } +} + +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; +}
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. Helper functions are then used to access the uint32_t which observe the semantics defined by the UInt32StateInfo struct. 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) Bits can be marked clear on read (cor) Regsiters can be truncated in width (width) 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. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)