Re: [Qemu-devel] [PATCH 11/21] target-arm: Update generic cpreg code for AArch64

[Peter Maydell]

On 20 December 2013 22:29, Peter Crosthwaite
<address@hidden> wrote:
> On Sat, Dec 21, 2013 at 8:07 AM, Peter Maydell <address@hidden> wrote:
>> On 20 December 2013 21:41, Peter Crosthwaite
>> <address@hidden> wrote:
>>> So to clarify, would MIDR and friends be in this bucket? And does t
>>> obsolete the old MIDR def such there is only one CPRegInfo globally?
>>
>> I think we end up with a MIDR def for old CPUs and a MIDR
>> for v8-and-up CPUs which handles both AArch64 and AArch32 views:
>>             { .name = "MIDR_EL1",
>>               .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
>>               .access = PL1_R,
>>              .type = ARM_CP_CONST|ARM_CP_AA64|ARM_CP_AA32VIEW,
>>              .resetvalue = cpu->midr },
>>
>> because (a) ARMv8 gets rid of the "unimplemented c15 0 0 0 x
>> registers read as MIDR"
>
> Ok so MIDR is an annoying special case (Some of the "friends" I listed
> early are probably better examples).

I think the friends would probably look much like the MIDR example
I give above.

>> and (b) we can ignore the annoying
>> special case handling for the TI925.
>>
>> (I wonder if it would be nicer to have a .state field (values
>
> Ideally, I think its about which encoding schemes have visibility of
> this reg. From what we discussed so far, it seems that there are three
> separate encoding schemes:
>
> 32/MRC - the current 99% case, but needs to now support access to
> widened-to-64 registers (upper RES0 policy)
> 32/MRRC - 64bit regs accessible from 32
> 64/MRS - What you are adding here
>
> One hot encode accessibility via each of these in a ".encodings"
> field, then the implementations or MRC, MRRC and MRS just check
> independently whether they are allowed or not.

Whether we split into .encodings or use flag fields or whatever
doesn't matter for that -- for "is there a register here via this
encoding scheme?" what we do is just do a hashtable lookup
and we expect to get a match only if the entire "this access
method plus all its op*/cr* fields" matches. This is what
ENCODE_CP_REG() and the new AArch64 equivalent
macro are doing.

How we lay things out in the reginfo struct is purely down
to what makes writing reginfo structs easiest. The code
which populates the hashtable just throws all the relevant
information into the correct ENCODE_CP_REG macro.

>> CP_STATE_AA32 == 0, CP_STATE_AA64, CP_STATE_BOTH)
>> rather than shoehorning it into the flags field...)
>>
>
> Agreed - new field.


>>>>  * register_cp_regs_for_features() has separate AArch32
>>>>    and AArch64 versions
>>>>
>>>
>>> Do we need this? With this scheme its still possible to enforce
>>> exclusivity in the CPRegInfo themselves.
>>
>> Mmm, I guess we don't really, though it does mean we're
>> a bit more likely to have dubious stuff lurking around in
>> the AArch32-visible sysregs. (OTOH that's pretty irrelevant
>> until we have support for either Hyp or TrustZone, since
>> otherwise EL1 is always AArch64...)
>
> Is there more to it that just enforcing MRS/MRC/MRRC perms? An MRS
> only defined CP reg should never have AArch32 visibility unless I am
> missing something.

That's correct. The issue is just that if we allow all the
existing "if v6 register these registers, if v5 register these, etc"
code to run then v8 CPUs potentially end up with (AArch32)
sysregs they don't really have. That is however pretty much
masked by the fact you can't actually get at the AArch32
sysregs mostly unless you have an Arch32 EL1, which in
turn you're not going to see in an AArch64 CPU unless
EL2 or EL3 are present and AArch64.

-- PMM