Re: [Qemu-devel] [PATCH v2 15/35] target-arm: Drop success/fail return from cpreg read and write functions

[Peter Maydell]

On 9 February 2014 03:27, Peter Crosthwaite
<address@hidden> wrote:
> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
>> -/* Access functions for coprocessor registers. These should always succeed. 
>> */
>> -typedef int CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque,
>> -                     uint64_t *value);
>> -typedef int CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
>> -                      uint64_t value);
>> +/* Access functions for coprocessor registers. These cannot fail and
>> + * may not raise exceptions.
>
> Is this based on an assumption that the only possible reason for an
> exception is access violation? This series quite validly obsoletes the
> need for exception-via-return-path, but my understanding is that CP
> accessor functions are able to implement any form of side affects. If
> an exception is thus generated from the side effects of a CP access
> then thats fair game - it just happens via the already existing
> mechanisms for generating an exception from helper context. Long story
> short, I think you can just drop second sentence from this comment.

It's based on the fact that the translate.c code no longer supports
calling raise_exception() from within a read/write function -- if you
try it the PC will be wrong because we haven't synchronised it
before calling the helper function. You're right that in theory a
coprocessor access could raise an arbitrary exception, but if
anybody needs that they'll need to add the support (probably by
adding an extra ARM_CP_ flag for "can raise exceptions" so we
don't take the hit of synchronising PC except in the odd case
where it's necessary.) In practice I think it is unlikely that there
will be any such situation which couldn't be handled by a suitable
accessfn.

>>  uint32_t HELPER(get_cp_reg)(CPUARMState *env, void *rip)
>>  {
>>      const ARMCPRegInfo *ri = rip;
>> -    uint64_t value;
>> -    int excp = ri->readfn(env, ri, &value);
>> -    if (excp) {
>> -        raise_exception(env, excp);
>> -    }
>> -    return value;
>
> Should ideally be a blank line here, but ..
>
>> +    return ri->readfn(env, ri);
>
> ... you could just drop the single use variable completely with:
>
> return ri->readfn(env, (const ARMCPRegInfo *)rip);

I dislike casts like that. I'll put in the blank line if you like.

thanks
-- PMM