Re: [PATCH v4 26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE
On Tue, 3 Dec 2019 at 02:30, Richard Henderson wrote: > > For ARMv8.1, op1 == 5 is reserved for EL2 aliases of > EL1 and EL0 registers. > > Signed-off-by: Richard Henderson > --- > target/arm/helper.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 023b8963cf..1812588fa1 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, > mask = PL0_RW; > break; > case 4: > +case 5: > /* min_EL EL2 */ > mask = PL2_RW; > break; > -case 5: > -/* unallocated encoding, so not possible */ > -assert(false); > -break; > case 6: > /* min_EL EL3 */ > mask = PL3_RW; > -- Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v4 26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE
On 12/4/19 2:38 PM, Alex Bennée wrote: > > Richard Henderson writes: > >> On 12/4/19 10:58 AM, Alex Bennée wrote: @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, mask = PL0_RW; break; case 4: +case 5: /* min_EL EL2 */ mask = PL2_RW; break; -case 5: -/* unallocated encoding, so not possible */ -assert(false); -break; >>> >>> This change is fine - I don't think we should have asserted here anyway. >>> But don't we generate an unallocated exception if the CPU is v8.0? >> >> This change is only for validation of the system registers themselves. It >> has >> nothing to do with the usage of system registers from the actual guest. > > So what is the mechanism that feeds back to the translator? The existence of a structure in the hash table. > access_check_cp_reg only seems to care about XSCALE. I guess > cp_access_ok would trip if you weren't at EL2 but what if you are a v8.0 > at EL2? This is sanity-checking the structure as it goes into the hash table. The version check happens when creating the structure -- we don't create registers that exist only for v8+ if the cpu is a v7. r~
Re: [PATCH v4 26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE
Richard Henderson writes: > On 12/4/19 10:58 AM, Alex Bennée wrote: >>> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, >>> mask = PL0_RW; >>> break; >>> case 4: >>> +case 5: >>> /* min_EL EL2 */ >>> mask = PL2_RW; >>> break; >>> -case 5: >>> -/* unallocated encoding, so not possible */ >>> -assert(false); >>> -break; >> >> This change is fine - I don't think we should have asserted here anyway. >> But don't we generate an unallocated exception if the CPU is v8.0? > > This change is only for validation of the system registers themselves. It has > nothing to do with the usage of system registers from the actual guest. So what is the mechanism that feeds back to the translator? access_check_cp_reg only seems to care about XSCALE. I guess cp_access_ok would trip if you weren't at EL2 but what if you are a v8.0 at EL2? -- Alex Bennée
Re: [PATCH v4 26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE
On 12/4/19 10:58 AM, Alex Bennée wrote: >> @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, >> mask = PL0_RW; >> break; >> case 4: >> +case 5: >> /* min_EL EL2 */ >> mask = PL2_RW; >> break; >> -case 5: >> -/* unallocated encoding, so not possible */ >> -assert(false); >> -break; > > This change is fine - I don't think we should have asserted here anyway. > But don't we generate an unallocated exception if the CPU is v8.0? This change is only for validation of the system registers themselves. It has nothing to do with the usage of system registers from the actual guest. r~
Re: [PATCH v4 26/40] target/arm: Update define_one_arm_cp_reg_with_opaque for VHE
Richard Henderson writes: > For ARMv8.1, op1 == 5 is reserved for EL2 aliases of > EL1 and EL0 registers. > > Signed-off-by: Richard Henderson > --- > target/arm/helper.c | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 023b8963cf..1812588fa1 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -7437,13 +7437,10 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu, > mask = PL0_RW; > break; > case 4: > +case 5: > /* min_EL EL2 */ > mask = PL2_RW; > break; > -case 5: > -/* unallocated encoding, so not possible */ > -assert(false); > -break; This change is fine - I don't think we should have asserted here anyway. But don't we generate an unallocated exception if the CPU is v8.0? > case 6: > /* min_EL EL3 */ > mask = PL3_RW; -- Alex Bennée
