From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49541) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad6IG-0003XI-KA for qemu-devel@nongnu.org; Mon, 07 Mar 2016 20:18:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad6IF-0006TH-9X for qemu-devel@nongnu.org; Mon, 07 Mar 2016 20:18:36 -0500 Received: from mail-oi0-x244.google.com ([2607:f8b0:4003:c06::244]:36488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad6IF-0006TD-3I for qemu-devel@nongnu.org; Mon, 07 Mar 2016 20:18:35 -0500 Received: by mail-oi0-x244.google.com with SMTP id c129so43889oif.3 for ; Mon, 07 Mar 2016 17:18:35 -0800 (PST) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: <87fuwf759r.fsf@linaro.org> References: <87fuwf759r.fsf@linaro.org> From: Alistair Francis Date: Mon, 7 Mar 2016 17:18:04 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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: =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: Edgar Iglesias , Peter Maydell , "qemu-devel@nongnu.org Developers" , Alistair Francis , Peter Crosthwaite , Edgar Iglesias , =?UTF-8?Q?Andreas_F=C3=A4rber?= , =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= On Fri, Feb 26, 2016 at 9:25 AM, Alex Benn=C3=A9e = wrote: > > 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 =3D opaque; >> + uint64_t we =3D ~0; >> + int shift =3D 0; >> + >> + if (reg->data_size !=3D size) { >> + we =3D (size =3D=3D 8) ? ~0ull : (1ull << size * 8) - 1; >> + shift =3D 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.. > } Good point. I think I have tidied this up. > >> + >> + assert(size + addr <=3D reg->data_size); > > Why are we asserting expected input conditions after we've done stuff? I am not sure why this is here. Removed. > >> + 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 =3D opaque; >> + int shift =3D 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. I think I have fixed this up. > >> + return register_read(reg) >> shift; >> +} >> + >> +uint64_t register_read_memory_be(void *opaque, hwaddr addr, unsigned si= ze) >> +{ >> + return register_read_memory(opaque, addr, size, true); >> +} >> + >> +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned si= ze) >> +{ >> + 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? I think it should be added in this patch (although in the next version it is added in a different struct) as this is starting to add the memory infrastructure. Thanks, Alistair > >> /* */ >> 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 regi= ster. >> + * _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 regi= ster. >> + * _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 si= ze); >> +uint64_t register_read_memory_le(void *opaque, hwaddr addr, unsigned si= ze); >> + >> #endif > > > -- > Alex Benn=C3=A9e >