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

[KONRAD Frederic <fred.konrad@greensocs.com>]

Hi,

Le 19/01/2016 23:34, Alistair Francis a écrit :
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> 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 <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> changed from v2:
> Simplified! Removed pre-read, nwx, wo
> Removed byte loops (Gerd Review)
> Made data pointer optional
> Added fast paths for simple registers
> Moved into hw/core and include/hw (Paolo Review)
> changed from v1:
> Rebranded as the "Register API" - I think thats probably what it is.
> Near total rewrite of implementation.
> De-arrayified reset (this is client/Memory APIs job).
> Moved out of bitops into its own file (Blue review)
> Added debug, the register pointer, and prefix to a struct (Blue Review)
> Made 64-bit to play friendlier with memory API (Blue review)
> Made backend storage uint8_t (MST review)
> Added read/write callbacks (Blue review)
> Added ui0, ui1 (Blue review)
> Moved re-purposed width (now byte width defining actual storage size)
> Arrayified ge0, ge1 (ui0, ui1 too) and added .reason
> Added wo field (not an April fools joke - this has genuine meaning here)
> Added we mask to write accessor
>
>   hw/core/Makefile.objs |   1 +
>   hw/core/register.c    | 186 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/register.h | 132 +++++++++++++++++++++++++++++++++++
>   3 files changed, 319 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..02a4376
> --- /dev/null
> +++ b/hw/core/register.c
> @@ -0,0 +1,186 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * 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 : "");
> +}
> +
> +static inline void register_write_val(RegisterInfo *reg, uint64_t val)
> +{
> +    if (!reg->data) {
> +        return;
> +    }
> +    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();
> +    }
> +}
> +
> +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;
> +    old_val = reg->data ? register_read_val(reg) : ac->reset;
Don't you need to check ac before doing that?

> +    if (reg->write_lite && !~we) { /* fast path!! */
> +        new_val = val;
> +        goto register_write_fast;
> +    }
> +
> +    if (!ac || !ac->name) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to undefined device state "
> +                      "(written value: %#" PRIx64 ")\n", reg->prefix, val);
> +        return;
> +    }
> +
> +    no_w_mask = ac->ro | ac->w1c | ~we;
> +
> +    if (reg->debug) {
> +        qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name,
> +                 val);
> +    }
> +
> +    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);
> +            }
> +        }
> +        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);
> +            }
> +        }
> +        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);
> +            }
> +        }
> +    }
> +
> +    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;
> +    }
> +
> +    ret = reg->data ? register_read_val(reg) : ac->reset;
> +
> +    if (!reg->read_lite) {
> +        register_write_val(reg, ret & ~ac->cor);
> +    }
> +
> +    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);
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +void register_reset(RegisterInfo *reg)
> +{
> +    assert(reg);
> +
> +    if (!reg->data || !reg->access) {
> +        return;
> +    }
> +
> +    register_write_val(reg, reg->access->reset);
> +}
Will that be called systematically at the reset?
If it is the case what about the register we don't want to reset?

> diff --git a/include/hw/register.h b/include/hw/register.h
> new file mode 100644
> index 0000000..249f458
> --- /dev/null
> +++ b/include/hw/register.h
> @@ -0,0 +1,132 @@
> +/*
> + * Register Definition API
> + *
> + * Copyright (c) 2013 Xilinx Inc.
> + * Copyright (c) 2013 Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> + *
> + * 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;
> +
> +    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 desciption of this register
description

Fred
> + *
> + * @debug: Whether or not verbose debug is enabled
> + * @prefix: String prefix for log and debug messages
> + *
> + * @opaque: Opaque data for the register
> + */
> +
> +struct RegisterInfo {
> +    void *data;
> +    int data_size;
> +
> +    const RegisterAccessInfo *access;
> +
> +    bool debug;
> +    const char *prefix;
> +
> +    void *opaque;
> +
> +    /*< private >*/
> +
> +    bool read_lite;
> +    bool write_lite;
> +};
> +
> +/**
> + * 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