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 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?

I guess that masters that really care about accurate the error
handling would need to issue transactions without relying on
the intermediate "magic" that splits unaligned accesses...

Anyway, I think your option a sounds the most flexible...

Cheers,
Edgar