Data Driven device registers & Zynq DEVCFG
cover Data Driven device registers & Zynq DEVCFG
Hi All. This is a new scheme i've come up with handling device registers in a data driven way. My motivation for this is to factor out a lot of the access checking that seems to be replicated in every device. See P2 commit message for further discussion.
P1 is an extension of qemu_log to query what features are and are not enabled. P2 is the main patch, adds the register definition functionality P3 is an example new device (the Xilinx Zynq devcfg) that uses this scheme. P4 adds devcfg to the Zynq machine model
This devcfg device was particularly finnicky with per-bit restrictions which prompted all this. Im also looking for a higher-than-usual modelling fidelity on the register space, with semantics defined for random reserved bits in-between otherwise consistent fields.
Heres an example of the qemu_log output for the devcfg device. This is produced by now generic sharable code:
/machine/unattached/device[44]:Addr 0x000008:CFG: write of value 00000508 /machine/unattached/device[44]:Addr 0x000080:MCTRL: write of value 00800010 /machine/unattached/device[44]:Addr 0x000010:INT_MASK: write of value ffffffff /machine/unattached/device[44]:Addr 00000000:CTRL: write of value 0c00607f
And an example of a rogue guest banging on a bad bit:
/machine/unattached/device[44]:Addr 0x000014:STATUS bits 0x000001 may not be \
written to 1
Future work: Theres a lot of overlap here with what Peter did with the ARM coprocessor definitions. We could go further and generalise ARM CP to use this or some further evolution of it. That and converting existing models to this scheme. Some device models will lose a lot of weight.
Peter A. G. Crosthwaite (2):
xilinx_zynq: devcfg device model
zynq: added devcfg to machine model
Peter Crosthwaite (2):
qemu-log: Allow checking of the current mask
bitops: Add UInt32StateInfo and helper functions
hw/arm/Makefile.objs | 2 +- hw/xilinx_devcfg.c | 443 +++++++++++++++++++++++++++++++++++++++++++++++++ hw/xilinx_zynq.c | 6 + include/qemu/bitops.h | 59 +++++++ include/qemu/log.h | 2 + qemu-log.c | 5 + util/bitops.c | 71 ++++++++ 7 files changed, 587 insertions(+), 1 deletions(-) create mode 100644 hw/xilinx_devcfg.c
RFC PATCH 1/4 qemu-log: Allow checking of the current mask
Useful for heavy users of qemu_log_mask that want to avoid executing expensive logic that sets up a qemu_log_mask when that mask is disabled. E.G.
if (qemu_log_get_mask() && LOG_GUEST_ERROR) {
/* do my expensive logging data query */
}qemu_log_mask(LOG_GUEST_ERROR, ...)
---
include/qemu/log.h | 2 ++ qemu-log.c | 5 +++++ 2 files changed, 7 insertions(+), 0 deletions(-)
include/qemu/log.h +2 -0
@@ -154,6 +154,8 @@ static inline void qemu_set_log(int log_flags)#endif } +int qemu_get_log_flags (void ); +void qemu_set_log_filename (const char *filename);int qemu_str_to_log_mask (const char *str);
qemu-log.c +5 -0
@@ -82,6 +82,11 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) } } +int qemu_get_log_flags (void ) +{ +return qemu_loglevel; +} +void qemu_set_log_filename (const char *filename) {g_free (logfilename);
Signature · 1 line
1.7.0.4
Re: [Qemu-devel] [RFC PATCH v1 1/4] qemu-log: Allow checking of the current mask
Quoted 7 lines · Peter Crosthwaite
Useful for heavy users of
qemu_log_maskthat want to avoid executing expensive logic that sets up aqemu_log_maskwhen that mask is disabled. E.G.if (qemu_log_get_mask() && LOG_GUEST_ERROR) { /* do my expensive logging data query */ }
qemu_log_mask(LOG_GUEST_ERROR, ...)
Why can't you use the existing
if (qemu_loglevel_mask(LOG_GUEST_ERROR) {
/* expensive thing */ }
?
-- PMM
RFC PATCH 2/4 bitops: Add UInt32StateInfo and helper functions
In reply to Peter CrosthwaiteThis 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.
---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)
include/qemu/bitops.h +59 -0
@@ -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
util/bitops.c +71 -0
80 lines not on this page · raw message
Signature · 1 line
1.7.0.4
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.
Quoted 3 lines
Helper functions are then used to access the
uint32_twhich 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.
Quoted 7 lines
Some features:
Bits can be marked asread_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).
Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)
s/Regsiters/Registers/
Quoted 4 lines
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.
include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0
I think this is not the right place since this is very much HW specific, please introduce a new file.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 7 lines
include/qemu/bitops.h +3
-0@@ -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/
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 32 lines
include/qemu/bitops.h +32
-0+* @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); + +/**
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 10 lines
include/qemu/bitops.h +10
-0+* 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.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions
read
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */
util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
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.
Quoted 8 lines · Blue Swirl
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.
Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 11 lines
There's some overlap also with PCI subsystem, it already implements readonly bits.
Quoted 3 lines
Helper functions are then used to access the
uint32_twhich observe the semantics defined by the UInt32StateInfo struct.We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice 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.
Quoted 10 lines
Quoted 7 lines
Some features:
Bits can be marked asread_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.
Quoted 5 lines
Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)s/Regsiters/Registers/
Will fix.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 2 lines
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.
Quoted 24 lines
This would
mean that the dynamic state would need to be more complex than justuint32_t. Also Pin could implement some of this functionality.include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0I think this is not the right place since this is very much HW specific, please introduce a new file.
Quoted 4 lines
include/qemu/bitops.h +0
-0@@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,return (value & ~mask) | ((fieldval << start) & mask); }
Quoted 6 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 3 lines
include/qemu/bitops.h +3
-0+/** +* 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
Quoted 5 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 36 lines
Quoted 36 lines
include/qemu/bitops.h +36
-0+* @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
Quoted 8 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 6 lines
include/qemu/bitops.h +6
-0+* @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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Quoted 9 lines
Then it should be easy to be
compatible with memory API.include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions read
Will fix.
Regards,
Peter
Quoted 104 lines
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */ util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Hi,
Quoted 8 lines
Quoted 2 lines
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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
In reply to Peter CrosthwaiteQuoted 27 lines · Peter Crosthwaite
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.
Quoted 8 lines · Blue Swirl
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.Quoted 4 lines
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.
Quoted 1 line
How to couple this with Pins, memory API,
Quoted 11 lines
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.
Quoted 5 lines
Quoted 4 lines
VMState and reset handling
needs some thought.
Quoted 9 lines
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 auint32_tarray so a singleVMSTATE_UINT32_ARRAYwill do the job.
Yes, QOM and VMState probably model bigger entities than single register entries. Specifying everything in one place would be nice though.
Quoted 153 lines
Quoted 5 lines
There's some overlap also with PCI subsystem, it already implements readonly bits.
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
Helper functions are then used to access the
uint32_twhich observe the semantics defined by the UInt32StateInfo struct.We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.Yes. Going lowest common denominator with
uint64_tdoes 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.Quoted 10 lines
Quoted 7 lines
Some features:
Bits can be marked asread_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_UNIMPandLOG_GUEST_ERROR. Just arrays of const char * in the definitions that cat onto theqemu_logmsgs telling you why that was a guest error or exactly what feature is unimplemented.Quoted 5 lines
Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)s/Regsiters/Registers/
Will fix.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 2 lines
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.
Quoted 24 lines
This would
mean that the dynamic state would need to be more complex than justuint32_t. Also Pin could implement some of this functionality.include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0I think this is not the right place since this is very much HW specific, please introduce a new file.
Quoted 4 lines
include/qemu/bitops.h +0
-0@@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int start, int length,return (value & ~mask) | ((fieldval << start) & mask); }Quoted 6 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 3 lines
include/qemu/bitops.h +3
-0+/** +* 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
Quoted 5 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 36 lines
Quoted 36 lines
include/qemu/bitops.h +36
-0+* @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 Quoted 8 lines
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 6 lines
include/qemu/bitops.h +6
-0+* @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.
Quoted 8 lines
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.
Quoted 10 lines
Quoted 3 lines
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.
Quoted 2 lines
and avoid passing debug flag directly (it could be in the dynamic structure or a pointer).
Quoted 6 lines
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.
Quoted 126 lines
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.
Quoted 9 lines
Then it should be easy to be
compatible with memory API.include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions read
Will fix.
Regards,
PeterQuoted 104 lines
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */ util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
In reply to Blue SwirlQuoted 12 lines · Blue Swirl
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
Helper functions are then used to access the
uint32_twhich observe the semantics defined by the UInt32StateInfo struct.We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.
Why not uint8_t for everyone?
Quoted 216 lines
Quoted 7 lines
Some features:
Bits can be marked asread_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).Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)s/Regsiters/Registers/
Quoted 4 lines
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 justuint32_t. Also Pin could implement some of this functionality.include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0I think this is not the right place since this is very much HW specific, please introduce a new file.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 7 lines
include/qemu/bitops.h +3
-0@@ -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/
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 32 lines
include/qemu/bitops.h +32
-0+* @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); + +/** include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 10 lines
include/qemu/bitops.h +10
-0+* 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.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions read
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */ util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 26 lines · Michael S. Tsirkin
Quoted 12 lines · Blue Swirl
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
Helper functions are then used to access the
uint32_twhich observe the semantics defined by the UInt32StateInfo struct.We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.Why not
uint8_tfor 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.
Quoted 217 lines
Quoted 217 lines
Quoted 8 lines
Some features:
Bits can be marked asread_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).Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)s/Regsiters/Registers/
Quoted 4 lines
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 justuint32_t. Also Pin could implement some of this functionality.include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0I think this is not the right place since this is very much HW specific, please introduce a new file.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 7 lines
include/qemu/bitops.h +3
-0@@ -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/
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 32 lines
include/qemu/bitops.h +32
-0+* @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); + +/** include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 10 lines
include/qemu/bitops.h +10
-0+* 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.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions read
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */ util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 38 lines · Blue Swirl
Quoted 26 lines · Michael S. Tsirkin
Quoted 12 lines · Blue Swirl
Quoted 5 lines · Peter Crosthwaite
This struct and functions provide some encapsulation of the
uint32_ttype 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.
Quoted 5 lines
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.Quoted 3 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
Helper functions are then used to access the
uint32_twhich observe the semantics defined by the UInt32StateInfo struct.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.Why not
uint8_tfor 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...
Quoted 2 lines
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.
Quoted 217 lines
Quoted 217 lines
Quoted 217 lines
Quoted 8 lines
Some features:
Bits can be marked asread_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).Quoted 2 lines
Bits can be marked clear on read (cor)
Regsiters can be truncated in width (width)s/Regsiters/Registers/
Quoted 4 lines
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 justuint32_t. Also Pin could implement some of this functionality.include/qemu/bitops.h
Quoted 12 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
include/qemu/bitops.h | 59 ++++++++++++++++++++++++++++++++++++++++ util/bitops.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 0 deletions(-)include/qemu/bitops.h +0
-0I think this is not the right place since this is very much HW specific, please introduce a new file.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 278
Quoted 7 lines
include/qemu/bitops.h +3
-0@@ -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/
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 280
Quoted 2 lines
include/qemu/bitops.h +2
-0+* @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
Quoted 32 lines
include/qemu/bitops.h +32
-0+* @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); + +/** include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 322
Quoted 10 lines
include/qemu/bitops.h +10
-0+* 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.
include/qemu/bitops.h static inline uint64_t deposit64(uint64_t value, int start, int length, · line 325
Quoted 3 lines
include/qemu/bitops.h +3
-0+ +/** +* write a value from a uint32_t subject to its restrictions read
Quoted 25 lines
include/qemu/bitops.h +9
-0+* @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 util/bitops.c +2
-0@@ -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 */ util/bitops.c unsigned long find_last_bit(const unsigned long *addr, unsigned long size) · line 152
Quoted 10 lines
util/bitops.c +8
-0return 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.
Quoted 65 lines
util/bitops.c +61
-1+ +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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Hi,
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.Why not
uint8_tfor everyone?That would be simple, but then modeling for example 32 bit registers gets clumsy.
Quoted 6 lines
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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 20 lines · Gerd Hoffmann
Hi,
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.Why not
uint8_tfor everyone?That would be simple, but then modeling for example 32 bit registers gets clumsy.
Quoted 6 lines
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.
Quoted 7 lines
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.
Signature · 1 line
MST
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
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;Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Gerd Hoffmann
Hi,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
We also need
uint8_t,uint16_tanduint64_tversions for some devices. Perhaps it would be better to implement auint64_tdevice which can be used with shorter widths or even stronger connection with memory API.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Why not
uint8_tfor 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.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 7 lines
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.
Quoted 4 lines
Quoted 2 lines
cheers,
GerdIf 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
Quoted 7 lines
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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 6 lines · Peter Crosthwaite
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_twith 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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Quoted 8 lines · Peter Maydell
Quoted 6 lines · Peter Crosthwaite
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_twith compulsory accessors is too verbose to be used in devices.
Quoted 4 lines
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.
Quoted 2 lines
-- PMM
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
In reply to Peter CrosthwaiteQuoted 7 lines
+/**
+ * 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
Quoted 2 lines
+ * @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
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Hi Gerd,
Quoted 5 lines · Gerd Hoffmann
Quoted 4 lines
+/**
+ * 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.
Quoted 8 lines
Quoted 2 lines
+ * @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).
Quoted 6 lines
Quoted 2 lines
+ * @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
Quoted 5 lines
cheers,
Gerd
Re: [Qemu-devel] [RFC PATCH v1 2/4] bitops: Add UInt32StateInfo and helper functions
Hi,
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
+ * @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?
Quoted 7 lines
Quoted 3 lines
reset=0 + ro=1 equals nw1=1
reset=1 + ro=1 equals nw0=1Some 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
RFC PATCH 3/4 xilinx_zynq: devcfg device model
In reply to Peter CrosthwaiteFrom: Peter A. G. Crosthwaite <[email protected]>
Minimal device model for devcfg module of Zynq. DMA capabilities and interrupt generation supported. Lock mechanisms supported.
---
hw/arm/Makefile.objs | 2 +- hw/xilinx_devcfg.c | 443 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 444 insertions(+), 1 deletions(-) create mode 100644 hw/xilinx_devcfg.c
hw/arm/Makefile.objs +1 -1
@@ -1,6 +1,6 @@ obj-y = integratorcp.o versatilepb.o arm_pic.o obj-y += arm_boot.o -obj-y += xilinx_zynq.o zynq_slcr.o +obj-y += xilinx_zynq.o zynq_slcr.o xilinx_devcfg.o obj-y += xilinx_spips.o obj-y += arm_gic.o arm_gic_common.o obj-y += a9scu.o
hw/xilinx_devcfg.c new file +443 -0
443 lines not on this page · raw message
Signature · 1 line
1.7.0.4
RFC PATCH 4/4 zynq: added devcfg to machine model
In reply to Peter CrosthwaiteFrom: Peter A. G. Crosthwaite <[email protected]>
---
hw/xilinx_zynq.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
hw/xilinx_zynq.c +6 -0
@@ -196,6 +196,12 @@ static void zynq_init(QEMUMachineInitArgs *args)sysbus_mmio_map (SYS_BUS_DEVICE (dev),0 ,0xE0101000 );sysbus_connect_irq (SYS_BUS_DEVICE (dev),0 , pic[79 -IRQ_OFFSET ]); + dev =qdev_create (NULL ,"xlnx.ps7-dev-cfg" ); +qdev_init_nofail (dev); + busdev =SYS_BUS_DEVICE (dev); +sysbus_connect_irq (busdev,0 , pic[40 -IRQ_OFFSET ]); +sysbus_mmio_map (busdev,0 ,0xF8007000 ); + zynq_binfo.ram_size = ram_size; zynq_binfo.kernel_filename = kernel_filename; zynq_binfo.kernel_cmdline = kernel_cmdline;
Signature · 1 line
1.7.0.4