Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

[Peter Maydell]

On 28 January 2014 14:05, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Jan 28, 2014 at 6:45 PM, Peter Maydell <address@hidden> wrote:
>> On 28 January 2014 01:46, Peter Crosthwaite
>> <address@hidden> wrote:
>>> There seem to be multiple instances in this series where you fallback
>>> to open coded R/W accessor functions for the sake of access checks. Is
>>> it better to define a bool check_access() fn hook in ARMCPRegInfo and
>>> leave the actual write/read behaviour to the data driven mechanisms?
>>> This may also minimise the need for raw_write hook usages as it serves
>>> to isolate the actual state change into its own self contained
>>> definition (whether open coded or not).
>>
>> Yes, I think it's probably going to be better to do that. We may need
>> to make it more than just bool, though since for AArch64 the
>> kind of exception can be different I think -- the specific syndrome
>> information can vary.
>>
>
> I guess then it's simplest to just return in same format as read/write
> accessors.

Well, AArch64 means we also can't just return EXCP_UDEF or 0,
but ideally return at least some syndrome information.
I haven't quite figured out the nicest way to do this yet.
My current work in progress has:
 * retain all the AArch32 EXCP_* classes as values for
   env->exception_index
 * whenever we generate an exception, also set up syndrome
   register information in the cpu state struct; the
   syndrome register high 6 bits happen to contain a category
   code, but we don't map this to an env->exception_index

So for AArch64, access functions need to be able to return one
of three things:
 (1) no exception, ie access OK
 (2) exception which in UNDEF if taken to AArch32, or
     a synchronous exception with ESR_ELx.EC = 0 ("uncategorized")
     if taken to AArch64
 (3) exception which is UNDEF if taken to AArch32, or a
     synchronous exception with ESR_ELx.EC == 0x3/4/5/6/C/18
     if taken to AArch64 (exact category depends on the
     insn used to access the register, and ESR_ELx
     includes all the opc1/2/crn/crm fields from the insn)
     -- this generally is used for "access failed because of
     a configurable trap or enable bit"

so we need to at least return a three-way "ok/disabled/undef"
code. I think actually most if not all of our current access
checks done in read/write fns should be type (2), but I don't
want to leave us stuck if we find one that needs to be type
(1) now or in the future. Failures due to the ri->access
permissions are always type (1).

We could return a complete EXCP_* value plus syndrome,
but that seems like overkill especially since the calling
code has most of the info needed for putting the syndrome
together properly. So I think:

typedef enum CPAccessResult {
    /* Access is permitted */
    CP_ACCESS_OK = 0,
    /* Access fails due to a configurable trap or enable which would
     * result in an exception syndrome other than 'uncategorized'
     */
    CP_ACCESS_TRAP = 1,
    /* Access fails and should result in an exception syndrome of
     * 'uncategorized'
     */
    CP_ACCESS_TRAP = 2,
} CPAccessResult;

typedef CPAccessResult CPAccessFn(CPUARMState *env,
                                  const ARMCPRegInfo *opaque);

and an entry in ARMCPRegInfo:
    /* Function for checking access permissions for the register
     * which should be done in addition to those specified by
     * the access field. If NULL, no extra checks required.
     */
    CPAccessFn accessfn;

and CPReadFn and CPWriteFn both go from 'int' return to
'void' return.

(Anybody got a better name than CPAccessResult for the enum type?)

thanks
-- PMM