qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Chengyu Song
Subject: Re: [Qemu-devel] [PATCH V2] support access to more performance registers in AA64 mode
Date: Mon, 8 Dec 2014 15:14:04 -0500

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.

> 
>> +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. Could you confirm this? If so, I will fix that function as 
well.

>> +}
>> +
>> 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? In my V1 patch, I didn't add the NO_MIGRATE flag and 
Christopher suggested I should add this flag, so I added.

> 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.

> 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.

>>     { .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?

Thanks,
Chengyu


reply via email to

[Prev in Thread] Current Thread [Next in Thread]