From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbT5i-0003Yl-NG for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:10:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbT5b-0003Nf-Un for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:10:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36448) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbT5b-0003NL-GJ for qemu-devel@nongnu.org; Fri, 27 Mar 2015 08:10:15 -0400 Message-ID: <5515489F.40102@redhat.com> Date: Fri, 27 Mar 2015 13:10:07 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1426526422-28338-1-git-send-email-peter.maydell@linaro.org> <1426526422-28338-2-git-send-email-peter.maydell@linaro.org> <20150327120228.GA19483@toto> In-Reply-To: <20150327120228.GA19483@toto> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" , Peter Maydell Cc: Peter Crosthwaite , Patch Tracking , QEMU Developers , Greg Bellows , =?windows-1252?Q?Alex_Benn=E9e?= , Richard Henderson On 27/03/2015 13:02, Edgar E. Iglesias wrote: > On Fri, Mar 27, 2015 at 10:58:01AM +0000, Peter Maydell wrote: >> On 16 March 2015 at 17:20, Peter Maydell wrote: >>> Define an API so that devices can register MemoryRegionOps whose read >>> and write callback functions are passed an arbitrary pointer to some >>> transaction attributes and can return a success-or-failure status code. >>> This will allow us to model devices which: >>> * behave differently for ARM Secure/NonSecure memory accesses >>> * behave differently for privileged/unprivileged accesses >>> * may return a transaction failure (causing a guest exception) >>> for erroneous accesses >> >>> The success/failure response indication is currently ignored; it is >>> provided so we can implement it later without having to change the >>> callback function API yet again in future. >> >>> +/* New-style MMIO accessors can indicate that the transaction failed. >>> + * This is currently ignored, but provided in the API to allow us to add >>> + * support later without changing the MemoryRegionOps functions yet again. >>> + */ >>> +typedef enum { >>> + MemTx_OK = 0, >>> + MemTx_DecodeError = 1, /* "nothing at that address" */ >>> + MemTx_SlaveError = 2, /* "device unhappy with request" (eg misalignment) */ >>> +} MemTxResult; >> >> So I was looking at how this would actually get plumbed through >> the memory subsystem code, and there are some awkwardnesses >> with this simple enum approach. In particular, functions like >> address_space_rw want to combine the error returns from >> several io_mem_read/write calls into a single response to >> return to the caller. With an enum we'd need some pretty >> ugly code to prioritise particular failure types, or to >> do something arbitrary like "return first failure code". >> Alternatively we could: >> (a) make MemTxResult a uint32_t, where all-bits zero indicates >> "OK" and any bit set indicates some kind of error, eg >> bit 0 set for "device returned an error", and bit 1 for >> "decode error", and higher bits available for other kinds >> of extra info about errors in future. Then address_space_rw >> just ORs together all the bits in all the return codes it >> receives. >> (b) give up and say "just use a bool" >> >> Opinions? > > Hi Peter, > > Is this related to masters relying on the memory frameworks magic > handling of unaliged accesses? Not necessarily, you can get the same just by doing a large write that spans multiple MemoryRegions. See the loop in address_space_rw. > Anyway, I think your option a sounds the most flexible... ACK Paolo > Cheers, > Edgar >