From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42196) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZLx4-00035U-Vo for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:13:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZLx0-0001f8-5B for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:13:14 -0500 Received: from mail-wm0-x235.google.com ([2a00:1450:400c:c09::235]:35645) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZLwz-0001eQ-Pz for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:13:10 -0500 Received: by mail-wm0-x235.google.com with SMTP id c200so81278390wme.0 for ; Fri, 26 Feb 2016 09:13:09 -0800 (PST) References: <30297902aab7203c57444d3f6a3c0da6aee3ce97.1455055858.git.alistair.francis@xilinx.com> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: <30297902aab7203c57444d3f6a3c0da6aee3ce97.1455055858.git.alistair.francis@xilinx.com> Date: Fri, 26 Feb 2016 17:13:06 +0000 Message-ID: <87h9gv75ul.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 02/16] register: Add Register API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, qemu-devel@nongnu.org, crosthwaitepeter@gmail.com, edgar.iglesias@gmail.com, afaerber@suse.de, fred.konrad@greensocs.com Alistair Francis 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 > Signed-off-by: Alistair Francis > --- > V4: > - Rebase > - Remove the guest error masking > - Simplify the unimplemented masking > - Use the reserved value in the write calculations > - Remove read_lite and write_lite > - General fixes to asserts and log printing > V3: > - Address some comments from Fred > > hw/core/Makefile.objs | 1 + > hw/core/register.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/register.h | 111 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 264 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..7e47df5 > --- /dev/null > +++ b/hw/core/register.c > @@ -0,0 +1,152 @@ > +/* > + * Register Definition API > + * > + * Copyright (c) 2013 Xilinx Inc. > + * Copyright (c) 2013 Peter Crosthwaite > + * > + * 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 "hw/qdev.h" > +#include "qemu/log.h" > + > +static inline void register_write_log(int mask, RegisterInfo *reg, int dir, > + uint64_t val, 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 : ""); > +} This seems unused, I guess the compiler ignores unused inlines through :-/ > + > +static inline void register_write_val(RegisterInfo *reg, uint64_t val) > +{ > + g_assert(reg->data); > + > + 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: > + g_assert_not_reached(); > + } > +} > + > +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: > + g_assert_not_reached(); > + } > + 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; > + > + assert(reg); > + > + ac = reg->access; > + > + 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; > + > + 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); > + } > + > + test = val & ac->unimp; > + if (test) { > + qemu_log_mask(LOG_UNIMP, > + "%s:%s writing %#" PRIx64 " to unimplemented bits:" \ > + " %#" PRIx64 "", > + reg->prefix, reg->access->name, val, ac->unimp); > + } > + > + > + no_w_mask = ac->ro | ac->w1c | ac->rsvd | ~we; > + new_val = (val & ~no_w_mask) | (old_val & no_w_mask); > + new_val &= ~(val & ac->w1c); A comment wouldn't go amiss here describing the concatenation of restrictions, RAZ, and ROW bits. > + > + if (ac->pre_write) { > + new_val = ac->pre_write(reg, new_val); > + } > + > + if (reg->debug) { > + qemu_log("%s:%s: write of value %#" PRIx64 "\n", reg->prefix, ac->name, > + new_val); > + } > + > + 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; > + > + register_write_val(reg, ret & ~ac->cor); > + > + if (ac->post_read) { > + ret = ac->post_read(reg, ret); > + } > + > + if (reg->debug) { > + qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefix, > + ac->name, ret); > + } > + > + return ret; > +} > + > +void register_reset(RegisterInfo *reg) > +{ > + g_assert(reg); > + g_assert(reg->data); > + g_assert(reg->access); > + > + 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..444239c > --- /dev/null > +++ b/include/hw/register.h > @@ -0,0 +1,111 @@ > +/* > + * Register Definition API > + * > + * Copyright (c) 2013 Xilinx Inc. > + * Copyright (c) 2013 Peter Crosthwaite > + * > + * 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; > + > +/** > + * 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 > + * > + * @ui1: Bits that when written 1 indicate use of an unimplemented feature > + * @ui0: Bits that when written 0 indicate use of an unimplemented > feature Last two comments don't match actual definition. > + * > + * @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; > + uint64_t unimp; > + > + 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 { > + /* */ > + 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 -- Alex Bennée