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

[Paolo Bonzini]

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.

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

ACK

Paolo

> Cheers,
> Edgar
>