From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aORN4-00074u-1B for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:47:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aORMz-00077x-4K for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:46:57 -0500 Received: from greensocs.com ([193.104.36.180]:41321) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aORMy-00077c-Pr for qemu-devel@nongnu.org; Wed, 27 Jan 2016 09:46:53 -0500 References: From: KONRAD Frederic Message-ID: <56A8D855.1030702@greensocs.com> Date: Wed, 27 Jan 2016 15:46:45 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 02/16] register: Add Register API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alistair Francis , qemu-devel@nongnu.org Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, crosthwaitepeter@gmail.com, afaerber@suse.de, edgar.iglesias@gmail.com Hi, Le 19/01/2016 23:34, Alistair Francis a =E9crit : > From: Peter Crosthwaite > > 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 > --- > 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) +=3D machine.o > common-obj-$(CONFIG_SOFTMMU) +=3D null-machine.o > common-obj-$(CONFIG_SOFTMMU) +=3D loader.o > common-obj-$(CONFIG_SOFTMMU) +=3D qdev-properties-system.o > +common-obj-$(CONFIG_SOFTMMU) +=3D register.o > common-obj-$(CONFIG_PLATFORM_BUS) +=3D 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 > + * > + * This work is licensed under the terms of the GNU GPL, version 2. S= ee > + * 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, uint= 64_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 =3D val; > + break; > + case 2: > + *(uint16_t *)reg->data =3D val; > + break; > + case 4: > + *(uint32_t *)reg->data =3D val; > + break; > + case 8: > + *(uint64_t *)reg->data =3D 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 =3D reg->access; > + old_val =3D 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 =3D 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 =3D 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 =3D (old_val ^ val) & ac->rsvd; > + if (test) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: change of value in res= erved bit" > + "fields: %#" PRIx64 ")\n", reg->prefix, test= ); > + } > + for (rae =3D ac->ge1; rae && rae->mask; rae++) { > + test =3D val & rae->mask; > + if (test) { > + register_write_log(reg, 1, test, LOG_GUEST_ERROR, > + "invalid", rae->reason); > + } > + } > + for (rae =3D ac->ge0; rae && rae->mask; rae++) { > + test =3D ~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 =3D ac->ui1; rae && rae->mask; rae++) { > + test =3D val & rae->mask; > + if (test) { > + register_write_log(reg, 1, test, LOG_GUEST_ERROR, > + "unimplmented", rae->reason); > + } > + } > + for (rae =3D ac->ui0; rae && rae->mask; rae++) { > + test =3D ~val & rae->mask; > + if (test) { > + register_write_log(reg, 0, test, LOG_GUEST_ERROR, > + "unimplemented", rae->reason); > + } > + } > + } > + > + new_val =3D (val & ~no_w_mask) | (old_val & no_w_mask); > + new_val &=3D ~(val & ac->w1c); > + > + if (ac->pre_write) { > + new_val =3D 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 =3D reg->access; > + if (!ac || !ac->name) { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: read from undefined device= state\n", > + reg->prefix); > + return 0; > + } > + > + ret =3D reg->data ? register_read_val(reg) : ac->reset; > + > + if (!reg->read_lite) { > + register_write_val(reg, ret & ~ac->cor); > + } > + > + if (ac->post_read) { > + ret =3D ac->post_read(reg, ret); > + } > + > + if (!reg->read_lite) { > + if (reg->debug) { > + qemu_log("%s:%s: read of value %#" PRIx64 "\n", reg->prefi= x, > + 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 > + * > + * This work is licensed under the terms of the GNU GPL, version 2. S= ee > + * 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 fea= ture > + * @ui0: Bits that when written 0 indicate use of an unimplemented fea= ture > + * > + * @pre_write: Pre write callback. Passed the value that's to be writt= en, > + * 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 wr= ite side > + * effects should be implemented here. > + * > + * @post_read: Post read callback. Passes the value that is about to b= e returned > + * for a read. The return value from this function is what is ultimate= ly read, > + * allowing this function to modify the value before return to the cli= ent. > + */ > + > +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