Re: [Qemu-devel] [PATCH v2 08/10] target/arm: Implement the ARMv8.1-LOR extension

[Richard Henderson]

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~