Re: [Qemu-devel] [PATCH v2 11/35] target-arm: Split cpreg access checks out from read/write functions

[Peter Crosthwaite]

On Sun, Feb 9, 2014 at 10:02 PM, Peter Maydell <address@hidden> wrote:
> On 9 February 2014 02:50, Peter Crosthwaite
> <address@hidden> wrote:
>> On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> wrote:
>>> +typedef enum CPAccessResult {
>>> +    /* Access is permitted */
>>> +    CP_ACCESS_OK = 0,
>>> +    /* Access fails due to a configurable trap or enable which would
>>> +     * result in a categorized exception syndrome giving information about
>>> +     * the failing instruction (ie syndrome category 0x3, 0x4, 0x5, 0x6,
>>> +     * 0xc or 0x18).
>>> +     */
>>> +    CP_ACCESS_TRAP = 1,
>>> +    /* Access fails and results in an exception syndrome 0x0 
>>> ("uncategorized").
>>> +     * Note that this is not a catch-all case -- the set of cases which may
>>> +     * result in this failure is specifically defined by the architecture.
>>> +     */
>>> +    CP_ACCESS_TRAP_UNCATEGORIZED = 2,
>>
>> Hi Peter,
>>
>> So its not obvious to me just yet why these two types or traps needs
>> separate encoding on this level. From your commentary, the two
>> different types of traps are mutually exclusive and identifyable by
>> the syndrome information. I.E _TRAP is syndrome != 0 and _TRAP_UNCAT
>> is syndrome == 0. Cant the access checkers return boolean if is_trap
>> then the syndrome can be used to make this distinction?
>
> No, because the access checker is the thing that tells us what the
> syndrome register is supposed to be. That is, the caller of the access
> functions will do something like:
>
>      ret = ri->accessfn(env, ri);
>      switch (ret) {
>           case CP_ACCESS_OK:
>                  return;
>           case CP_ACCESS_TRAP:
>                  env->exception.syndrome = syn_cp_trap(stuff);
>                  break;
>           case CP_ACCESS_TRAP_UNCATEGORIZED:
>                  env->exception_syndrome = syn_uncategorized();
>                  break;
>      }
>      raise_exception(EXCP_UDEF);
>
> (This code is in the syndrome work I have which sits on top of this
> patchset; it's pretty nearly ready to post but I have a few odds and
> ends to tie off first.)
>
> The actual value of the syndrome register depends on a pile
> of stuff that includes various fields from the instruction, so we
> don't want to burden the access checkers with actually setting
> or returning us a syndrome value. (Also the syndrome encoding
> covers the whole uint32_t space so it's not possible to return
> "syndrome or some value meaning no exception", which makes
> that an awkward API choice anyway.) So we just have the checker
> tell us "OK, or this set of syndromes, or that set?" and we can calculate
> the syndrome from just that three-way bit of information.
>
> The phrasing of the comments above is supposed to help somebody
> implementing a cpreg know what they should be returning : the
> ARM ARM docs will let them know which kind of syndrome category
> the access failure is, which in turn indicates which CP_ value to
> return to cause QEMU to generate the syndrome information that
> is required.
>

Thanks for the clarification. I was assuming that checkers were going
to do syndromes.

Regards,
Peter

> thanks
> -- PMM
>