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

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

On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <address@hidden> wrote:
> On Sun, Mar 3, 2013 at 6:13 AM, Peter Crosthwaite
> <address@hidden> wrote:
>> This struct and functions provide some encapsulation of the uint32_t type to
>> make it more friendly for use as guest accessible device state. Bits of 
>> device
>> state (usually MMIO registers), often have all sorts of access restrictions
>> and semantics associated with them. This struct allow you to define what 
>> whose
>> restrictions are on a bit-by-bit basis.
>
> I like the approach, it could simplify devices and make them more self
> documenting. Maybe devices could be also generated directly from HW
> synthesis tool outputs.
>

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 <address@hidden>
>> ---
>>
>>  include/qemu/bitops.h |   59 ++++++++++++++++++++++++++++++++++++++++
>>  util/bitops.c         |   71 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 130 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/qemu/bitops.h b/include/qemu/bitops.h
>> index affcc96..8ad0290 100644
>> --- a/include/qemu/bitops.h
>> +++ b/include/qemu/bitops.h
>
> I think this is not the right place since this is very much HW
> specific, please introduce a new file.
>
>> @@ -273,4 +273,63 @@ static inline uint64_t deposit64(uint64_t value, int 
>> start, int length,
>>      return (value & ~mask) | ((fieldval << start) & mask);
>>  }
>>
>> +/**
>> + * A descriptor for a Uint32 that is part of guest accessible device state
>> + * @ro: whether or not the bit is read-only state comming out of reset
>
> s/comming/coming/
>

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