Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode

[Peter Maydell]

On 8 December 2014 at 20:14, Chengyu Song <address@hidden> wrote:
> Hello Peter,
>
>> Thanks for sending this patch. I've provided some detailed
>> review below -- there are quite a lot of comments but the
>> fixes needed are all very minor.
>
> I'm not very familiar with the CP emulation, especially the NO_MIGRATE
> flag, so thank you very much for the comment.
>
>> In this patchset you add 64-bit accessors to several registers
>> which were previously 32-bit only. For this to work you need
>> to change the fields in CPUARMState from uint32_t to uint64_t for:
>>
>>   c9_pmovsr
>>   c9_pmxevtyper
>>   c9_pmuserenr
>>   c9_pminten
>
> According to the manual, all these registers are remaining as 32-bit
> registers, so there is no need to extend them to unit64_t.

No, they must be extended. Where the ARM ARM describes an AArch64
register as "32 bit" this is a shorthand for "64 bit register where
the top 32 bits are RES0". In QEMU we always implement all AArch64
registers by doing 64 bit accesses to their CPUARMState fields,
and so the fields must be 64 bits wide or we will read/write to
a neighbouring field as well.

>>> +static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> +                           uint64_t value)
>>> +{
>>> +    env->cp15.c9_pmovsr |= value;
>>
>> You need "value &= (1 << 31);" first, because the bits
>> [30:0] are RAZ/WI. (We can get away without this in the clear-bits
>> accessor because the lower bits in the register are already zero.)
>
> Agree. Because QEMU does not support events now, the rest bit should
> be RAZ/WI. But in the pmovsr_write function, there is no such masking,
> so I'm not sure if this is necessary.

As I say in the bracketed comment above, we don't need to mask
in pmovsr_write: the lower bits in the register are already
zero and so even if the "value" written by the guest is non-zero
in those bits the "&" operation will not be able to set them.

>>> +}
>>> +
>>> static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>                              uint64_t value)
>>> {
>>> @@ -824,6 +830,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>>     { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
>>>       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 1,
>>>       .access = PL0_RW, .accessfn = pmreg_access,
>>> +      .type = ARM_CP_NO_MIGRATE,
>>
>> I don't think this is correct -- this is the struct that migrates
>> the PMCNTEN state. The other registers (32-bit accessors and the
>> 64-bit clear accessor) are all accessing the same underlying state
>> as this one, so they are marked NO_MIGRATE, but one register in
>> the group has to be migrated.
>
> So in general, if one underlying state is accessed through multiple
> registers, then one register should be able to migrate and all other
> should not, is this understand correct?

Yes, that's correct. The general approach we use is to have the
one register that migrates be the 64-bit version, with the 32
bit version marked NO_MIGRATE.

>> Please keep to the order cp/opc1/crn/crm/opc2.
>>
>> The PMOVSSET register doesn't exist in ARMv7, so this stanza
>> needs to go into the v8_cp_reginfo[] array.
>
> Let me check the v7 manual and move v8 registers to the correct array.
>
>>> +    { .name = "PMCCFILTR", .cp = 15, .crn = 14, .crm = 15, .opc1 = 3, 
>>> .opc2 = 7,
>>
>> The ARM ARM says this is opc1 = 0. Also could you order these
>> fields cp/opc1/crn/crm/opc2, please?
>
> Yes, I can. This is the AA32 mapping of the register, so what
> I was thinking is to make AA32 registers in the old form
> cp/crn/crm/opc1/opc2, and use the new cp(opc0)/opc1/crn/crm/opc2
> form for AA64 registers.

We've settled on using the same order for both. If you look at
the v8 ARM ARM you'll see that the documentation also uses this
same order for AArch32 registers. (Some older documentation had
a different order, which is partly why QEMU is inconsistent.)

>> As with PMOVSSET, this register isn't present in ARMv7,
>> so this stanza needs to be in v8_cp_reginfo[]. I suggest you
>> move the existing PMCCFILTR_EL0 along with it, so we keep all
>> the PMCCFILTR handling in one place.
>
> I see. So the old code already has the problem, and it's better
> to fix it as well.

Yes, sort of -- but it's not an actual bug, since PMCCFILTR_EL0
is an AArch64 register and there are no CPUs before v8 which can
have those registers. It is conceptually in the wrong place, though.

>>>     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 
>>> = 1,
>>> -      .access = PL1_RW,
>>> +      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>>> +      .resetvalue = 0,
>>> +      .writefn = pmintenset_write, .raw_writefn = raw_write },
>>> +    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
>>> +      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
>>> +      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>>>       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>>>       .resetvalue = 0,
>>>       .writefn = pmintenset_write, .raw_writefn = raw_write },
>>
>> You can do these two with a single struct, because the
>> encodings line up nicely:
>>
>>    { .name = "PMINTENSET", .state = ARM_CP_STATE_BOTH,
>>      .cp = 15, .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 1,
>>      .access = PL1_RW,
>>      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>>      .resetvalue = 0,
>>      .writefn = pmintenset_write, .raw_writefn = raw_write },
>
> I'm not quite sure about this, because each register should only
> be accessible in its corresponding mode: PMINTENSET in AA32,
> and PMINTENSET_EL1 in AA64. Wouldn't setting STATE_BOTH allow
> accessing PMINTENSET_EL1 in AA32 mode?

The point is that both these registers are identical: they have
the same access rights, field offset, reset value and write
access functions. Using STATE_BOTH is exactly the same as
having two separate structs. (The .name field will end up being
the same in both cases, but we use that only for debugging QEMU,
so it doesn't matter that it's not strictly the architectural
name.)

thanks
-- PMM