On 12/6/18 7:49 AM, Peter Maydell wrote:
>> +static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
>> +{
>> + return FIELD_EX64(id->id_aa64mmfr0, ID_AA64MMFR1, LO) != 0;
>
> You're testing the wrong ID register here...
Oops, I thought I fixed that...
>> +static CPAccessResult access_lor_other(CPUARMState *env,
>> + const ARMCPRegInfo *ri, bool isread)
>> +{
>> + int el = arm_current_el(env);
>> +
>> + if (arm_is_secure_below_el3(env)) {
>> + /* Access denied in secure mode. */
>> + return CP_ACCESS_TRAP;
>> + }
>> + if (el < 2 && arm_el_is_aa64(env, 2)) {
>
> You don't need to explicitly check if EL2 is AArch64 --
> these registers are AArch64 only so if we accessed them
> from a lower EL then that EL is AArch64 and so all the
> ELs above it must be too.
Ok.
>
>> + uint64_t hcr = arm_hcr_el2_eff(env);
>> + if (hcr & HCR_E2H) {
>> + hcr &= HCR_TLOR;
>> + } else {
>> + hcr &= HCR_TGE | HCR_TLOR;
>
> This doesn't make sense to me: I think TGE can't be 1 here.
> We know (.access flag) that we're not in EL0. We know from
> the el < 2 that we're in EL1, and we've already ruled out
> Secure EL1. So this is NS EL1. If E2H == 0 then TGE can't
> be set, because E2H,TGE={0,1} means the CPU can never be
> in NS EL1.
Ok. Perhaps I was too blindly following the access cases listed in the ARM and
not applying any extra thought.
> (these remarks apply also to the access_lorid function)
I think I will split out a shared subroutine for these.
>> + case 0x8: /* STLLR */
>> + if (!dc_isar_feature(aa64_lor, s)) {
>> + break;
>> + }
>> + /* StoreLORelease is the same as Store-Release for QEMU. */
>> + /* fallthru */
>
> We are as usual a bit inconsistent, but we seem to use the
> spelling "fall through" for this linter-hint more often than
> any of the other variations.
I was going to say something about old habits, but I do note that I've somehow
migrated from the witheringly old FALLTHRU. So perhaps I can learn
eventually...
r~
