Re: [Qemu-devel] [RFC 1/5] memory: Define API for MemoryRegionOps to take attrs and return status

[Edgar E. Iglesias]

On Fri, Mar 27, 2015 at 01:10:07PM +0100, Paolo Bonzini wrote:
> 
> 
> 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 <address@hidden> 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.

Right, this is another case of "magic" memory handling that allows masters
to issue unnatural transactions and rely on the memory framework to
split things up.
In these cases aren't the masters trading accuracy (including error
handling accuracy) for performance or model simplicity?

It could maybe be useful to have a flag so masters can say one of the
following (could be encoded in the memattrs):
1. Stop at first error and return.
2. Keep going after errors and give me the OR result of all errors.

For 1 to be useful, I think it would have to be combined with some
kind of return info that can point out where in the magic splitting
of large or unaligned transactions that things went wrong.

Probably overkill at the moment...

Cheers,
Edgar