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

[Blue Swirl]

On Mon, Mar 4, 2013 at 1:37 AM, Peter Crosthwaite
<address@hidden> wrote:
> Hi Blue,
>
> Thanks for the review. Comments in-line below. Are you on the IRC much
> and what timezone? I'd like to ask a few questions about how you see
> this fitting with the memory API, that would probably go much faster
> in live discussion. I've commented on the issue below with my current
> thoughts.
>
> On Sun, Mar 3, 2013 at 7:01 PM, Blue Swirl <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.

I think Avi mentioned that that memory API should be operating at
single register level one day and your model looks like we could use
that as one step. Perhaps the best of all worlds would be to keep
things compatible with memory API so that it can be used if needed.

>
>> VMState and reset handling
>> needs some thought.
>>
>
> If we add VMState and reset then we are only one step away from these
> registers being a QOM object (TYPE_DEVICE) in their own right. Might
> not be a bad thing but does strike me as heavy-weight. It is a good
> question where do we draw the line between the responsibilities of the
> device and the API. Here im am shooting for the grey area between full
> QOM device for every register and the status quo (no API at all).
> Regarding VMState, the current implementation the devices dynamic
> state is still limited to a uint32_t array so a single
> VMSTATE_UINT32_ARRAY will do the job.

Yes, QOM and VMState probably model bigger entities than single
register entries. Specifying everything in one place would be nice
though.

>
>> There's some overlap also with PCI subsystem, it already implements
>> readonly bits.
>>
>>>
>>> Helper functions are then used to access the uint32_t which observe the
>>> semantics defined by the UInt32StateInfo struct.
>>
>> We also need uint8_t, uint16_t and uint64_t versions for some devices.
>> Perhaps it would be better to implement a uint64_t device which can be
>> used with shorter widths or even stronger connection with memory API.
>>
>
> Yes. Going lowest common denominator with uint64_t does however cost
> more memory and VMS storage which could impact devices with large
> register spaces. This wouldn't impact any of my devices in any way
> though. The performance hit is probably worth it for the simplicity.
> V2 will be 64 bit. I will drop the "64" from the function name to keep
> it generic and we can assert that no one tries to create a register
> with .width > 64.
>
>>>
>>> Some features:
>>> Bits can be marked as read_only (ro field)
>>> Bits can be marked as write-1-clear (w1c field)
>>> Bits can be marked as sticky (nw0 and nw1)
>>> Reset values can be defined (reset)
>>> Bits can be marked to throw guest errors when written certain values (ge0, 
>>> ge1)
>>
>> Other bits could be marked as unimplemented (LOG_UNIMP).
>>
>
> Agreed. Will add to V2. Another Idea id like to play with is adding
> per-bit explanations for the LOG_UNIMP and LOG_GUEST_ERROR. Just
> arrays of const char * in the definitions that cat onto the qemu_log
> msgs telling you why that was a guest error or exactly what feature is
> unimplemented.
>
>>> Bits can be marked clear on read (cor)
>>> Regsiters can be truncated in width (width)
>>
>> s/Regsiters/Registers/
>>
>
> Will fix.
>
>>>
>>> Useful for defining device register spaces in a data driven way. Cuts down 
>>> on a
>>> lot of the verbosity and repetition in the switch-case blocks in the 
>>> standard
>>> foo_mmio_read/write functions.
>>
>> For maximum flexibility, a callback could be specified but then we
>> overlap memory API.
>>
>
> I think this is a good idea, but continuing on the theme of what this
> API is trying to achieve I think there should be capability for
> per-bit function definitions. On the topic I think Gerd has played
> with the idea of per-register callbacks for some device models in the
> past.
>
>> Once we have Pin available, it could be useful to couple a register
>> bit directly with a Pin. Currently we could use qemu_irq.
>
> Ill wait on Pin to land and stabilise for this one and continue to use
> code driven GPIO setters.
>
>> This would
>> mean that the dynamic state would need to be more complex than just
>> uint32_t. Also Pin could implement some of this functionality.
>>
>>>
>>> Signed-off-by: Peter Crosthwaite <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 think this is asking that the prefix should be handled at device
level and register objects should be always part of a parent device
object so that register methods can always query parent device name.

>
>> I'd also combine state and
>> info,
>
> Similar problem. info would need to be per-device and no longer
> shared. Not such a bad thing though.
>
>> and avoid passing debug flag directly (it could be in the
>> dynamic structure or a pointer).
>
> Debug as used in its current form could actually live in the static
> structure. I'm just thinking if there are any nice ways to set that up
> such that you don't have to add ".debug = FOO_DEBUG" to every table
> entry. Although if we end up de-constifying the table to fold in the
> prefix and state (as discussed above) then a helper function can just
> iterate over the table and sort it out in one go.

The dynamic structure could have a pointer to the static const struct.

>
> Any by now you've convinced me :) Ill add some per-instance state in
> V2 to store this stuff. The const table example I've given in P3 will
> be unchanged. It will reduce this function prototype to two args (info
> and val). There will a few new this and that helpers that object init
> fns can call to set this up in a handful of lines.
>
>> Then it should be easy to be
>> compatible with memory API.
>>
>>> +
>>> +/**
>>> + * write a value from a uint32_t subject to its restrictions
>>
>> read
>>
>
> Will fix.
>
> Regards,
> Peter
>
>>> + * @state: Pointer to location to be read
>>> + * @info: Definition of variable
>>> + * @prefix: Debug and error message prefix
>>> + * @debug: Turn on noisy debug printfery
>>> + */
>>> +
>>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>>> +                     const char *prefix, bool debug);
>>> +
>>>  #endif
>>> diff --git a/util/bitops.c b/util/bitops.c
>>> index e72237a..51db476 100644
>>> --- a/util/bitops.c
>>> +++ b/util/bitops.c
>>> @@ -11,6 +11,8 @@
>>>   * 2 of the License, or (at your option) any later version.
>>>   */
>>>
>>> +#include "qemu-common.h"
>>> +#include "qemu/log.h"
>>>  #include "qemu/bitops.h"
>>>
>>>  #define BITOP_WORD(nr)         ((nr) / BITS_PER_LONG)
>>> @@ -140,3 +142,72 @@ unsigned long find_last_bit(const unsigned long *addr, 
>>> unsigned long size)
>>>      /* Not found */
>>>      return size;
>>>  }
>>> +void uint32_array_reset(uint32_t *state, const UInt32StateInfo *info, int 
>>> num)
>>> +{
>>> +    int i = 0;
>>> +
>>> +    for (i = 0; i < num; ++i) {
>>> +        state[i] = info[i].reset;
>>> +    }
>>> +}
>>
>> Perhaps this could be automatically registered.
>>
>>> +
>>> +void uint32_write(uint32_t *state, const UInt32StateInfo *info, uint32_t 
>>> val,
>>> +                  const char *prefix, bool debug)
>>> +{
>>> +    int i;
>>> +    uint32_t new_val;
>>> +    int width = info->width ? info->width : 32;
>>> +
>>> +    uint32_t no_w0_mask = info->ro | info->w1c | info->nw0 |
>>> +                        ~((1ull << width) - 1);
>>> +    uint32_t no_w1_mask = info->ro | info->w1c | info->nw1 |
>>> +                        ~((1ull << width) - 1);
>>> +
>>> +    if (!info->name) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device 
>>> state "
>>> +                      "(written value: %#08x)\n", prefix, val);
>>> +        return;
>>> +    }
>>> +
>>> +    if (debug) {
>>> +        fprintf(stderr, "%s:%s: write of value %08x\n", prefix, info->name,
>>> +                val);
>>> +    }
>>> +
>>> +    /*FIXME: skip over if no LOG_GUEST_ERROR */
>>> +    for (i = 0; i <= 1; ++i) {
>>> +        uint32_t test = (val ^ (i ? 0 : ~0)) & (i ? info->ge1 : info->ge0);
>>> +        if (test) {
>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s:%s bits %#08x may not be 
>>> written"
>>> +                          " to %d\n", prefix, info->name, test, i);
>>> +        }
>>> +    }
>>> +
>>> +    new_val = val & ~(no_w1_mask & val);
>>> +    new_val |= no_w1_mask & *state & val;
>>> +    new_val |= no_w0_mask & *state & ~val;
>>> +    new_val &= ~(val & info->w1c);
>>> +    *state = new_val;
>>> +}
>>> +
>>> +uint32_t uint32_read(uint32_t *state, const UInt32StateInfo *info,
>>> +                     const char *prefix, bool debug)
>>> +{
>>> +    uint32_t ret = *state;
>>> +
>>> +    /* clear on read */
>>> +    ret &= ~info->cor;
>>> +    *state = ret;
>>> +
>>> +    if (!info->name) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device 
>>> state "
>>> +                      "(read value: %#08x)\n", prefix, ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (debug) {
>>> +        fprintf(stderr, "%s:%s: read of value %08x\n", prefix, info->name, 
>>> ret);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> --
>>> 1.7.0.4
>>>
>>>
>>