Re: [Qemu-devel] [PATCH v3 02/16] register: Add Register API

[Alistair Francis]

On Tue, Feb 9, 2016 at 8:06 AM, Alex Bennée <address@hidden> wrote:
>
> Alistair Francis <address@hidden> writes:
>
>> This API provides some encapsulation of registers and factors our some
>> common functionality to common code. Bits of device state (usually MMIO
>> registers), often have all sorts of access restrictions and semantics
>> associated with them. This API allow you to define what those
>> restrictions are on a bit-by-bit basis.
>>
>> Helper functions are then used to access the register which observe the
>> semantics defined by the RegisterAccessInfo 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 reserved (rsvd field)
>> Reset values can be defined (reset)
>> Bits can throw guest errors when written certain values (ge0, ge1)
>> Bits can throw unimp errors when written certain values (ui0, ui1)
>> Bits can be marked clear on read (cor)
>> Pre and post action callbacks can be added to read and write ops
>> Verbose debugging info can be enabled/disabled
>>
>> 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.
>>
>> Also useful for automated generation of device models from hardware
>> design sources.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> V3:
>>  - Address some comments from Fred
>>
>>  hw/core/Makefile.objs |   1 +
>>  hw/core/register.c    | 189 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 322 insertions(+)
>>  create mode 100644 hw/core/register.c
>>  create mode 100644 include/hw/register.h
>>
>> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
>> index abb3560..bf95db5 100644
>> --- a/hw/core/Makefile.objs
>> +++ b/hw/core/Makefile.objs
>> @@ -14,4 +14,5 @@ common-obj-$(CONFIG_SOFTMMU) += machine.o
>>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>> +common-obj-$(CONFIG_SOFTMMU) += register.o
>>  common-obj-$(CONFIG_PLATFORM_BUS) += platform-bus.o
>> diff --git a/hw/core/register.c b/hw/core/register.c
>> new file mode 100644
>> index 0000000..f0fc39c
>> --- /dev/null
>> +++ b/hw/core/register.c
>> @@ -0,0 +1,189 @@
>> +/*
>> + * Register Definition API
>> + *
>> + * Copyright (c) 2013 Xilinx Inc.
>> + * Copyright (c) 2013 Peter Crosthwaite <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "hw/register.h"
>> +#include "qemu/log.h"
>> +
>> +static inline void register_write_log(RegisterInfo *reg, int dir, uint64_t 
>> val,
>> +                                      int mask, const char *msg,
>> +                                      const char *reason)
>> +{
>> +    qemu_log_mask(mask, "%s:%s bits %#" PRIx64 " %s write of %d%s%s\n",
>> +                  reg->prefix, reg->access->name, val, msg, dir,
>> +                  reason ? ": " : "", reason ? reason : "");
>
> I'm not sure mask needs to be passed down as every use of it seems to
> imply LOG_GUEST_USER. If you think this might change I would consider
> passing the mask as the first option to align with other logging functions.

Removing this function as it is no longer required.

>
>> +}
>> +
>> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
>> +{
>> +    if (!reg->data) {
>> +        return;
>> +    }
>
> Can you have a defined register without a data field? If it is invalid I
> would use a g_assert() instead.

Good point, I can't see any cases where there wouldn't be a data field.

>
>> +    switch (reg->data_size) {
>> +    case 1:
>> +        *(uint8_t *)reg->data = val;
>> +        break;
>> +    case 2:
>> +        *(uint16_t *)reg->data = val;
>> +        break;
>> +    case 4:
>> +        *(uint32_t *)reg->data = val;
>> +        break;
>> +    case 8:
>> +        *(uint64_t *)reg->data = val;
>> +        break;
>> +    default:
>> +        abort();
>
> g_assert_not_reached();

Fixed

>
>> +    }
>> +}
>> +
>> +static inline uint64_t register_read_val(RegisterInfo *reg)
>> +{
>> +    switch (reg->data_size) {
>> +    case 1:
>> +        return *(uint8_t *)reg->data;
>> +    case 2:
>> +        return *(uint16_t *)reg->data;
>> +    case 4:
>> +        return *(uint32_t *)reg->data;
>> +    case 8:
>> +        return *(uint64_t *)reg->data;
>> +    default:
>> +        abort();
>> +    }
>> +    return 0; /* unreachable */
>> +}
>> +
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we)
>> +{
>> +    uint64_t old_val, new_val, test, no_w_mask;
>> +    const RegisterAccessInfo *ac;
>> +    const RegisterAccessError *rae;
>> +
>> +    assert(reg);
>> +
>> +    ac = reg->access;
>
> Should this assert(reg->access), how would you create a register without one?

An assert seems a little harsh here. It seems reachable if the guest
writes at the end of the register list or if you didn't bother
implementing all of the registers. I think this gives more details
then an assert would.

>> +
>> +    if (!ac || !ac->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state 
>> "
>> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
>> +        return;
>> +    }
>> +
>> +    old_val = reg->data ? register_read_val(reg) : ac->reset;
>> +
>> +    if (reg->write_lite && !~we) { /* fast path!! */
>> +        new_val = val;
>> +        goto register_write_fast;
>> +    }
>
> What is the point of this fast path? It looks like it saves a few debug
> log checks and some bitops, hardly performance critical considering
> we've already left the TCG code at this point.
>
> I'd be minded to leave it out and if you can actually measure a
> difference add it in a future patch.

Peter would know more about this then me, I'm not sure if it sees a
huge speed increase.

I'll take it out now, then when we start adding more complex devices
(the ones with this patch set are pretty simple) we can add it back in
if there is a slowdown. Same with read lite

>
>> +
>> +    no_w_mask = ac->ro | ac->w1c | ~we;
>
> Push this calculation down to where the rest are done.

Fixed

>
>> +
>> +    if (reg->debug) {
>> +        qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, 
>> ac->name,
>> +                 val);
>> +    }
>
> If this is for debugging purposes I'd be tempted to move the print down
> to the bottom after you've calculated the eventual result.

Fixed

>
>> +
>> +    if (qemu_loglevel_mask(LOG_GUEST_ERROR)) {
>> +        test = (old_val ^ val) & ac->rsvd;
>> +        if (test) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in reserved 
>> bit"
>> +                          "fields: %#" PRIx64 ")\n", reg->prefix, test);
>> +        }
>> +        for (rae = ac->ge1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>
> I think the walking of the RegisterAcceessError entries could be pushed
> into a helper function. See later comments on the structure:
>
>> +        for (rae = ac->ge0; rae && rae->mask; rae++) {
>> +            test = ~val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "invalid", rae->reason);
>> +            }
>> +        }
>> +    }
>> +
>> +    if (qemu_loglevel_mask(LOG_UNIMP)) {
>> +        for (rae = ac->ui1; rae && rae->mask; rae++) {
>> +            test = val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 1, test, LOG_GUEST_ERROR,
>> +                                   "unimplmented", rae->reason);
>
> LOG_UNIMP surely, these are bits we aren't modelling yet??

Fixed

>
>> +            }
>> +        }
>> +        for (rae = ac->ui0; rae && rae->mask; rae++) {
>> +            test = ~val & rae->mask;
>> +            if (test) {
>> +                register_write_log(reg, 0, test, LOG_GUEST_ERROR,
>> +                                   "unimplemented", rae->reason);
>
> LOG_UNIMP?
>
>> +            }
>> +        }
>> +    }
>> +
>> +    new_val = (val & ~no_w_mask) | (old_val & no_w_mask);
>> +    new_val &= ~(val & ac->w1c);
>> +
>> +    if (ac->pre_write) {
>> +        new_val = ac->pre_write(reg, new_val);
>> +    }
>> +
>> +register_write_fast:
>> +    register_write_val(reg, new_val);
>> +    if (ac->post_write) {
>> +        ac->post_write(reg, new_val);
>> +    }
>> +}
>> +
>> +uint64_t register_read(RegisterInfo *reg)
>> +{
>> +    uint64_t ret;
>> +    const RegisterAccessInfo *ac;
>> +
>> +    assert(reg);
>> +
>> +    ac = reg->access;
>> +    if (!ac || !ac->name) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device 
>> state\n",
>> +                      reg->prefix);
>> +        return 0;
>> +    }
>
> Again I think these are assertions if you shouldn't be able to create
> registers without access rules.

See above

>
>> +
>> +    ret = reg->data ? register_read_val(reg) : ac->reset;
>> +
>> +    if (!reg->read_lite) {
>> +        register_write_val(reg, ret & ~ac->cor);
>> +    }
>
> I'm a little fuzzy on what read_lite is meant to mean? Later we say:
>
>     /* no debug and no clear-on-read is a fast read */
>     reg->read_lite = reg->debug || ac->cor ? false : true;
>
> Why not simply test ac->cor here and act accordingly. It seems a fuzzy 
> indirection.

I have removed read_lite

>
>> +
>> +    if (ac->post_read) {
>> +        ret = ac->post_read(reg, ret);
>> +    }
>> +
>> +    if (!reg->read_lite) {
>> +        if (reg->debug) {
>> +            qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix,
>> +                     ac->name, ret);
>> +        }
>> +    }
>
> Same here, read_lite seems superfluous.
>
>> +
>> +    return ret;
>> +}
>> +
>> +void register_reset(RegisterInfo *reg)
>> +{
>> +    assert(reg);
>> +
>> +    if (!reg->data || !reg->access) {
>> +        return;
>> +    }
>
> The same general assert comment.

Although I agree it should be an assert here as the QEMU model calls this.

>
>> +
>> +    register_write_val(reg, reg->access->reset);
>> +}
>> diff --git a/include/hw/register.h b/include/hw/register.h
>> new file mode 100644
>> index 0000000..a80427b
>> --- /dev/null
>> +++ b/include/hw/register.h
>> @@ -0,0 +1,132 @@
>> +/*
>> + * Register Definition API
>> + *
>> + * Copyright (c) 2013 Xilinx Inc.
>> + * Copyright (c) 2013 Peter Crosthwaite <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef REGISTER_H
>> +#define REGISTER_H
>> +
>> +#include "exec/memory.h"
>> +
>> +typedef struct RegisterInfo RegisterInfo;
>> +typedef struct RegisterAccessInfo RegisterAccessInfo;
>> +
>> +/**
>> + * A register access error message
>> + * @mask: Bits in the register the error applies to
>> + * @reason: Reason why this access is an error
>> + */
>> +
>> +typedef struct RegisterAccessError {
>> +    uint64_t mask;
>> +    const char *reason;
>> +} RegisterAccessError;
>> +
>> +/**
>> + * Access description for a register that is part of guest accessible device
>> + * state.
>> + *
>> + * @name: String name of the register
>> + * @ro: whether or not the bit is read-only
>> + * @w1c: bits with the common write 1 to clear semantic.
>> + * @reset: reset value.
>> + * @cor: Bits that are clear on read
>> + * @rsvd: Bits that are reserved and should not be changed
>> + *
>> + * @ge1: Bits that when written 1 indicate a guest error
>> + * @ge0: Bits that when written 0 indicate a guest error
>> + * @ui1: Bits that when written 1 indicate use of an unimplemented feature
>> + * @ui0: Bits that when written 0 indicate use of an unimplemented feature
>> + *
>> + * @pre_write: Pre write callback. Passed the value that's to be written,
>> + * immediately before the actual write. The returned value is what is 
>> written,
>> + * giving the handler a chance to modify the written value.
>> + * @post_write: Post write callback. Passed the written value. Most write 
>> side
>> + * effects should be implemented here.
>> + *
>> + * @post_read: Post read callback. Passes the value that is about to be 
>> returned
>> + * for a read. The return value from this function is what is ultimately 
>> read,
>> + * allowing this function to modify the value before return to the client.
>> + */
>> +
>> +struct RegisterAccessInfo {
>> +    const char *name;
>> +    uint64_t ro;
>> +    uint64_t w1c;
>> +    uint64_t reset;
>> +    uint64_t cor;
>> +    uint64_t rsvd;
>> +
>> +    const RegisterAccessError *ge0;
>> +    const RegisterAccessError *ge1;
>> +    const RegisterAccessError *ui0;
>> +    const RegisterAccessError *ui1;
>
> Granted there is only one example of these being used in this patch
> series but it seems excessive to waste 4 pointers that are NULL most of
> the time. I think you could push the bit polarity and error flag into
> the RegisterAccessError structure and only walk the list of bits once.
>
> In fact I think you could make rsvd a part of this array as well for
> common handling. Wouldn't ge1/ui1 be equivalent of writing to a RESVD
> value anyway? In fact you want to model RES0 and RES1 types don't you?

Agreed! I have removed the guest error mask and updated write code accordingly.

I have also simplified the unimplemented masking. It does mean that
the reason can't be specified, but it is a lot neater and I'm not sure
how often specific reasons will be used.

Thanks,

Alistair

>
>> +
>> +    uint64_t (*pre_write)(RegisterInfo *reg, uint64_t val);
>> +    void (*post_write)(RegisterInfo *reg, uint64_t val);
>> +
>> +    uint64_t (*post_read)(RegisterInfo *reg, uint64_t val);
>> +};
>> +
>> +/**
>> + * A register that is part of guest accessible state
>> + * @data: pointer to the register data. Will be cast
>> + * to the relevant uint type depending on data_size.
>> + * @data_size: Size of the register in bytes. Must be
>> + * 1, 2, 4 or 8
>> + *
>> + * @access: Access description of this register
>> + *
>> + * @debug: Whether or not verbose debug is enabled
>> + * @prefix: String prefix for log and debug messages
>> + *
>> + * @opaque: Opaque data for the register
>> + */
>> +
>> +struct RegisterInfo {
>> +    /* <private> */
>> +    bool read_lite;
>> +    bool write_lite;
>
> As mentioned above, what do these mean?
>
>> +
>> +    /* <public> */
>> +    void *data;
>> +    int data_size;
>> +
>> +    const RegisterAccessInfo *access;
>> +
>> +    bool debug;
>> +    const char *prefix;
>> +
>> +    void *opaque;
>> +};
>> +
>> +/**
>> + * write a value to a register, subject to its restrictions
>> + * @reg: register to write to
>> + * @val: value to write
>> + * @we: write enable mask
>> + */
>> +
>> +void register_write(RegisterInfo *reg, uint64_t val, uint64_t we);
>> +
>> +/**
>> + * read a value from a register, subject to its restrictions
>> + * @reg: register to read from
>> + * returns: value read
>> + */
>> +
>> +uint64_t register_read(RegisterInfo *reg);
>> +
>> +/**
>> + * reset a register
>> + * @reg: register to reset
>> + */
>> +
>> +void register_reset(RegisterInfo *reg);
>> +
>> +#endif
>
> Thanks,
>
> --
> Alex Bennée
>