diff mbox series

[3/8] KVM: arm64: Map AArch32 cp15 register to AArch64 sysregs

Message ID 20201102191609.265711-4-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Kill the copro array | expand

Commit Message

Marc Zyngier Nov. 2, 2020, 7:16 p.m. UTC 
Move all the cp15 registers over to their AArch64 counterpart.
This requires the annotation of a few of them (such as the usual
DFAR/IFAR vs FAR_EL1), and a new helper that generates mask/shift
pairs for the various configurations.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 107 +++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 49 deletions(-)

Comments

James Morse Nov. 3, 2020, 6:29 p.m. UTC | #1 
Hi Marc,

On 02/11/2020 19:16, Marc Zyngier wrote:
> Move all the cp15 registers over to their AArch64 counterpart.
> This requires the annotation of a few of them (such as the usual
> DFAR/IFAR vs FAR_EL1), and a new helper that generates mask/shift
> pairs for the various configurations.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 26c7c25f8a6d..137818793a4a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -138,26 +156,16 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>  			  const struct sys_reg_desc *r)
>  {
>  	bool was_enabled = vcpu_has_cache_enabled(vcpu);
> -	u64 val;
> -	int reg = r->reg;
> +	u64 val, mask, shift;
>  
>  	BUG_ON(!p->is_write);
>  
> -	/* See the 32bit mapping in kvm_host.h */
> -	if (p->is_aarch32)
> -		reg = r->reg / 2;
> +	get_access_mask(r, &mask, &shift);
>  
> -	if (!p->is_aarch32 || !p->is_32bit) {
> -		val = p->regval;
> -	} else {
> -		val = vcpu_read_sys_reg(vcpu, reg);
> -		if (r->reg % 2)
> -			val = (p->regval << 32) | (u64)lower_32_bits(val);
> -		else
> -			val = ((u64)upper_32_bits(val) << 32) |
> -				lower_32_bits(p->regval);
> -	}
> -	vcpu_write_sys_reg(vcpu, val, reg);

> +	val = vcpu_read_sys_reg(vcpu, r->reg);
> +	val &= ~mask;
> +	val |= (p->regval & (mask >> shift)) << shift;
> +	vcpu_write_sys_reg(vcpu, val, r->reg);

I can't tell if the compiler has worked out ithat it can skip the sys_read most of the
time... Won't some of these trap for a nested VHE hypervisor?

| if (~mask) {
|	val = vcpu_read_sys_reg(vcpu, r->reg);
|	val &= ~mask;
| }


But, as the sys_reg arrays already have indirection for this function, why does
access_vm_reg() have to deal with this at all? Its not even needed for all the aarch32
registers...


| { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_aarch32_alias, NULL, MAIR_EL1 },

Where access_aarch32_alias() does the shift+mask and read_modify-write on the sys_reg?


>  	kvm_toggle_cache(vcpu, was_enabled);
>  	return true;

> @@ -1919,19 +1919,24 @@ static const struct sys_reg_desc cp14_64_regs[] = {
>   */
>  static const struct sys_reg_desc cp15_regs[] = {

> -	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, c2_TTBCR },

> +	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, TCR_EL1 },

Don't look now ... TTBRCR2 means this one is a LO/HI job.

(I'm guessing that should be fixed before this series to make the backport easy)


Thanks,

James
Marc Zyngier Nov. 10, 2020, 10:14 a.m. UTC | #2 
On 2020-11-03 18:29, James Morse wrote:
> Hi Marc,
> 
> On 02/11/2020 19:16, Marc Zyngier wrote:
>> Move all the cp15 registers over to their AArch64 counterpart.
>> This requires the annotation of a few of them (such as the usual
>> DFAR/IFAR vs FAR_EL1), and a new helper that generates mask/shift
>> pairs for the various configurations.
> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 26c7c25f8a6d..137818793a4a 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -138,26 +156,16 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>>  			  const struct sys_reg_desc *r)
>>  {
>>  	bool was_enabled = vcpu_has_cache_enabled(vcpu);
>> -	u64 val;
>> -	int reg = r->reg;
>> +	u64 val, mask, shift;
>> 
>>  	BUG_ON(!p->is_write);
>> 
>> -	/* See the 32bit mapping in kvm_host.h */
>> -	if (p->is_aarch32)
>> -		reg = r->reg / 2;
>> +	get_access_mask(r, &mask, &shift);
>> 
>> -	if (!p->is_aarch32 || !p->is_32bit) {
>> -		val = p->regval;
>> -	} else {
>> -		val = vcpu_read_sys_reg(vcpu, reg);
>> -		if (r->reg % 2)
>> -			val = (p->regval << 32) | (u64)lower_32_bits(val);
>> -		else
>> -			val = ((u64)upper_32_bits(val) << 32) |
>> -				lower_32_bits(p->regval);
>> -	}
>> -	vcpu_write_sys_reg(vcpu, val, reg);
> 
>> +	val = vcpu_read_sys_reg(vcpu, r->reg);
>> +	val &= ~mask;
>> +	val |= (p->regval & (mask >> shift)) << shift;
>> +	vcpu_write_sys_reg(vcpu, val, r->reg);
> 
> I can't tell if the compiler has worked out ithat it can skip the
> sys_read most of the
> time... Won't some of these trap for a nested VHE hypervisor?
> 
> | if (~mask) {
> |	val = vcpu_read_sys_reg(vcpu, r->reg);
> |	val &= ~mask;
> | }

Seems like a good call. I'm happy to fold that in.

> 
> 
> But, as the sys_reg arrays already have indirection for this function, 
> why does
> access_vm_reg() have to deal with this at all? Its not even needed for
> all the aarch32 registers...
> 
> 
> | { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0),
> access_aarch32_alias, NULL, MAIR_EL1 },
> 
> Where access_aarch32_alias() does the shift+mask and read_modify-write
> on the sys_reg?


I don't really like the idea of separate handlers. The whole point is to
unify the two view, and I feel like having yet another indirection makes
it harder to maintain.

> 
>>  	kvm_toggle_cache(vcpu, was_enabled);
>>  	return true;
> 
>> @@ -1919,19 +1919,24 @@ static const struct sys_reg_desc 
>> cp14_64_regs[] = {
>>   */
>>  static const struct sys_reg_desc cp15_regs[] = {
> 
>> -	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, c2_TTBCR 
>> },
> 
>> +	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, TCR_EL1 
>> },
> 
> Don't look now ... TTBRCR2 means this one is a LO/HI job.

That's the problem with AArch32. You stop looking for a couple of years,
and it decides to throw a new sysreg at you without any notice. WTF?

> (I'm guessing that should be fixed before this series to make the 
> backport easy)

I'll work something out as a prologue to this series.

Thanks,

         M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 26c7c25f8a6d..137818793a4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -128,6 +128,24 @@  static bool access_dcsw(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static void get_access_mask(const struct sys_reg_desc *r, u64 *mask, u64 *shift)
+{
+	switch (r->aarch32_map) {
+	case AA32_LO:
+		*mask = GENMASK_ULL(31, 0);
+		*shift = 0;
+		break;
+	case AA32_HI:
+		*mask = GENMASK_ULL(63, 32);
+		*shift = 32;
+		break;
+	default:
+		*mask = GENMASK_ULL(63, 0);
+		*shift = 0;
+		break;
+	}
+}
+
 /*
  * Generic accessor for VM registers. Only called as long as HCR_TVM
  * is set. If the guest enables the MMU, we stop trapping the VM
@@ -138,26 +156,16 @@  static bool access_vm_reg(struct kvm_vcpu *vcpu,
 			  const struct sys_reg_desc *r)
 {
 	bool was_enabled = vcpu_has_cache_enabled(vcpu);
-	u64 val;
-	int reg = r->reg;
+	u64 val, mask, shift;
 
 	BUG_ON(!p->is_write);
 
-	/* See the 32bit mapping in kvm_host.h */
-	if (p->is_aarch32)
-		reg = r->reg / 2;
+	get_access_mask(r, &mask, &shift);
 
-	if (!p->is_aarch32 || !p->is_32bit) {
-		val = p->regval;
-	} else {
-		val = vcpu_read_sys_reg(vcpu, reg);
-		if (r->reg % 2)
-			val = (p->regval << 32) | (u64)lower_32_bits(val);
-		else
-			val = ((u64)upper_32_bits(val) << 32) |
-				lower_32_bits(p->regval);
-	}
-	vcpu_write_sys_reg(vcpu, val, reg);
+	val = vcpu_read_sys_reg(vcpu, r->reg);
+	val &= ~mask;
+	val |= (p->regval & (mask >> shift)) << shift;
+	vcpu_write_sys_reg(vcpu, val, r->reg);
 
 	kvm_toggle_cache(vcpu, was_enabled);
 	return true;
@@ -167,17 +175,13 @@  static bool access_actlr(struct kvm_vcpu *vcpu,
 			 struct sys_reg_params *p,
 			 const struct sys_reg_desc *r)
 {
+	u64 mask, shift;
+
 	if (p->is_write)
 		return ignore_write(vcpu, p);
 
-	p->regval = vcpu_read_sys_reg(vcpu, ACTLR_EL1);
-
-	if (p->is_aarch32) {
-		if (r->Op2 & 2)
-			p->regval = upper_32_bits(p->regval);
-		else
-			p->regval = lower_32_bits(p->regval);
-	}
+	get_access_mask(r, &mask, &shift);
+	p->regval = (vcpu_read_sys_reg(vcpu, r->reg) & mask) >> shift;
 
 	return true;
 }
@@ -1264,10 +1268,6 @@  static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	int reg = r->reg;
 
-	/* See the 32bit mapping in kvm_host.h */
-	if (p->is_aarch32)
-		reg = r->reg / 2;
-
 	if (p->is_write)
 		vcpu_write_sys_reg(vcpu, p->regval, reg);
 	else
@@ -1919,19 +1919,24 @@  static const struct sys_reg_desc cp14_64_regs[] = {
  */
 static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 0), CRm( 0), Op2( 1), access_ctr },
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, c1_SCTLR },
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 1), access_actlr },
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 3), access_actlr },
-	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
-	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, c2_TTBR1 },
-	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, c2_TTBCR },
-	{ Op1( 0), CRn( 3), CRm( 0), Op2( 0), access_vm_reg, NULL, c3_DACR },
-	{ Op1( 0), CRn( 5), CRm( 0), Op2( 0), access_vm_reg, NULL, c5_DFSR },
-	{ Op1( 0), CRn( 5), CRm( 0), Op2( 1), access_vm_reg, NULL, c5_IFSR },
-	{ Op1( 0), CRn( 5), CRm( 1), Op2( 0), access_vm_reg, NULL, c5_ADFSR },
-	{ Op1( 0), CRn( 5), CRm( 1), Op2( 1), access_vm_reg, NULL, c5_AIFSR },
-	{ Op1( 0), CRn( 6), CRm( 0), Op2( 0), access_vm_reg, NULL, c6_DFAR },
-	{ Op1( 0), CRn( 6), CRm( 0), Op2( 2), access_vm_reg, NULL, c6_IFAR },
+	{ Op1( 0), CRn( 1), CRm( 0), Op2( 0), access_vm_reg, NULL, SCTLR_EL1 },
+	/* ACTLR */
+	{ AA32(LO), Op1( 0), CRn( 1), CRm( 0), Op2( 1), access_actlr, NULL, ACTLR_EL1 },
+	/* ACTLR2 */
+	{ AA32(HI), Op1( 0), CRn( 1), CRm( 0), Op2( 3), access_actlr, NULL, ACTLR_EL1 },
+	{ Op1( 0), CRn( 2), CRm( 0), Op2( 0), access_vm_reg, NULL, TTBR0_EL1 },
+	{ Op1( 0), CRn( 2), CRm( 0), Op2( 1), access_vm_reg, NULL, TTBR1_EL1 },
+	{ Op1( 0), CRn( 2), CRm( 0), Op2( 2), access_vm_reg, NULL, TCR_EL1 },
+	{ Op1( 0), CRn( 3), CRm( 0), Op2( 0), access_vm_reg, NULL, DACR32_EL2 },
+	/* DFSR */
+	{ Op1( 0), CRn( 5), CRm( 0), Op2( 0), access_vm_reg, NULL, ESR_EL1 },
+	{ Op1( 0), CRn( 5), CRm( 0), Op2( 1), access_vm_reg, NULL, IFSR32_EL2 },
+	{ Op1( 0), CRn( 5), CRm( 1), Op2( 0), access_vm_reg, NULL, AFSR0_EL1 },
+	{ Op1( 0), CRn( 5), CRm( 1), Op2( 1), access_vm_reg, NULL, AFSR1_EL1 },
+	/* DFAR */
+	{ AA32(LO), Op1( 0), CRn( 6), CRm( 0), Op2( 0), access_vm_reg, NULL, FAR_EL1 },
+	/* IFAR */
+	{ AA32(HI), Op1( 0), CRn( 6), CRm( 0), Op2( 2), access_vm_reg, NULL, FAR_EL1 },
 
 	/*
 	 * DC{C,I,CI}SW operations:
@@ -1957,15 +1962,19 @@  static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 2), access_pminten },
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
 
-	{ Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, c10_PRRR },
-	{ Op1( 0), CRn(10), CRm( 2), Op2( 1), access_vm_reg, NULL, c10_NMRR },
-	{ Op1( 0), CRn(10), CRm( 3), Op2( 0), access_vm_reg, NULL, c10_AMAIR0 },
-	{ Op1( 0), CRn(10), CRm( 3), Op2( 1), access_vm_reg, NULL, c10_AMAIR1 },
+	/* PRRR/MAIR0 */
+	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },
+	/* NMRR/MAIR1 */
+	{ AA32(HI), Op1( 0), CRn(10), CRm( 2), Op2( 1), access_vm_reg, NULL, MAIR_EL1 },
+	/* AMAIR0 */
+	{ AA32(LO), Op1( 0), CRn(10), CRm( 3), Op2( 0), access_vm_reg, NULL, AMAIR_EL1 },
+	/* AMAIR1 */
+	{ AA32(HI), Op1( 0), CRn(10), CRm( 3), Op2( 1), access_vm_reg, NULL, AMAIR_EL1 },
 
 	/* ICC_SRE */
 	{ Op1( 0), CRn(12), CRm(12), Op2( 5), access_gic_sre },
 
-	{ Op1( 0), CRn(13), CRm( 0), Op2( 1), access_vm_reg, NULL, c13_CID },
+	{ Op1( 0), CRn(13), CRm( 0), Op2( 1), access_vm_reg, NULL, CONTEXTIDR_EL1 },
 
 	/* Arch Tmers */
 	{ SYS_DESC(SYS_AARCH32_CNTP_TVAL), access_arch_timer },
@@ -2040,14 +2049,14 @@  static const struct sys_reg_desc cp15_regs[] = {
 
 	{ Op1(1), CRn( 0), CRm( 0), Op2(0), access_ccsidr },
 	{ Op1(1), CRn( 0), CRm( 0), Op2(1), access_clidr },
-	{ Op1(2), CRn( 0), CRm( 0), Op2(0), access_csselr, NULL, c0_CSSELR },
+	{ Op1(2), CRn( 0), CRm( 0), Op2(0), access_csselr, NULL, CSSELR_EL1 },
 };
 
 static const struct sys_reg_desc cp15_64_regs[] = {
-	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR0 },
+	{ Op1( 0), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR0_EL1 },
 	{ Op1( 0), CRn( 0), CRm( 9), Op2( 0), access_pmu_evcntr },
 	{ Op1( 0), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI1R */
-	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, c2_TTBR1 },
+	{ Op1( 1), CRn( 0), CRm( 2), Op2( 0), access_vm_reg, NULL, TTBR1_EL1 },
 	{ Op1( 1), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_ASGI1R */
 	{ Op1( 2), CRn( 0), CRm(12), Op2( 0), access_gic_sgi }, /* ICC_SGI0R */
 	{ SYS_DESC(SYS_AARCH32_CNTP_CVAL),    access_arch_timer },