From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49054) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZM9A-00078b-BH for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:25:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZM96-0000po-3K for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:25:44 -0500 Received: from mail-wm0-x230.google.com ([2a00:1450:400c:c09::230]:32828) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZM95-0000p4-Cq for qemu-devel@nongnu.org; Fri, 26 Feb 2016 12:25:39 -0500 Received: by mail-wm0-x230.google.com with SMTP id g62so81240724wme.0 for ; Fri, 26 Feb 2016 09:25:39 -0800 (PST) References: From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Fri, 26 Feb 2016 17:25:36 +0000 Message-ID: <87fuwf759r.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v4 03/16] register: Add Memory API glue 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: > From: Peter Crosthwaite > > Add memory io handlers that glue the register API to the memory API. > Just translation functions at this stage. Although it does allow for > devices to be created without all-in-one mmio r/w handlers. > > Signed-off-by: Peter Crosthwaite > Signed-off-by: Alistair Francis > --- > > hw/core/register.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/register.h | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 79 insertions(+) > > diff --git a/hw/core/register.c b/hw/core/register.c > index 7e47df5..9cd50c8 100644 > --- a/hw/core/register.c > +++ b/hw/core/register.c > @@ -150,3 +150,51 @@ void register_reset(RegisterInfo *reg) > > register_write_val(reg, reg->access->reset); > } > + > +static inline void register_write_memory(void *opaque, hwaddr addr, > + uint64_t value, unsigned size, bool be) > +{ > + RegisterInfo *reg = opaque; > + uint64_t we = ~0; > + int shift = 0; > + > + if (reg->data_size != size) { > + we = (size == 8) ? ~0ull : (1ull << size * 8) - 1; > + shift = 8 * (be ? reg->data_size - size - addr : addr); > + } What happens if the user writes too large a value at once to the register? I haven't attempted to decode the shift magic going on here but perhaps the handling would be clearer if there was a: if (size > reg->data_size) { ..deal with it.. } else if (size < data_size ) { ..do other magic.. } > + > + assert(size + addr <= reg->data_size); Why are we asserting expected input conditions after we've done stuff? > + register_write(reg, value << shift, we << shift); > +} > + > +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value, > + unsigned size) > +{ > + register_write_memory(opaque, addr, value, size, true); > +} > + > + > +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value, > + unsigned size) > +{ > + register_write_memory(opaque, addr, value, size, false); > +} > + > +static inline uint64_t register_read_memory(void *opaque, hwaddr addr, > + unsigned size, bool be) > +{ > + RegisterInfo *reg = opaque; > + int shift = 8 * (be ? reg->data_size - size - addr : addr); > + Well we never have to deal with an over/undersized read? I suspect the magic above might not correctly represent some hardware when presented with such a thing on the bus. > + return register_read(reg) >> shift; > +} > + > +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size) > +{ > + return register_read_memory(opaque, addr, size, true); > +} > + > +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size) > +{ > + return register_read_memory(opaque, addr, size, false); > +} > diff --git a/include/hw/register.h b/include/hw/register.h > index 444239c..9aa9cfc 100644 > --- a/include/hw/register.h > +++ b/include/hw/register.h > @@ -69,9 +69,14 @@ struct RegisterAccessInfo { > * @prefix: String prefix for log and debug messages > * > * @opaque: Opaque data for the register > + * > + * @mem: optional Memory region for the register > */ > > struct RegisterInfo { > + /* */ > + MemoryRegion mem; > + This seems unconnected with adding the helpers. Should it come with the original definition or when it actually gets used? > /* */ > void *data; > int data_size; > @@ -108,4 +113,30 @@ uint64_t register_read(RegisterInfo *reg); > > void register_reset(RegisterInfo *reg); > > +/** > + * Memory API MMIO write handler that will write to a Register API register. > + * _be for big endian variant and _le for little endian. > + * @opaque: RegisterInfo to write to > + * @addr: Address to write > + * @value: Value to write > + * @size: Number of bytes to write > + */ > + > +void register_write_memory_be(void *opaque, hwaddr addr, uint64_t value, > + unsigned size); > +void register_write_memory_le(void *opaque, hwaddr addr, uint64_t value, > + unsigned size); > + > +/** > + * Memory API MMIO read handler that will read from a Register API register. > + * _be for big endian variant and _le for little endian. > + * @opaque: RegisterInfo to read from > + * @addr: Address to read > + * @size: Number of bytes to read > + * returns: Value read from register > + */ > + > +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned size); > +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned size); > + > #endif -- Alex Bennée