All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
@ 2020-05-26 16:18 ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel; +Cc: Marc Zyngier

Hello!

Patch 1 fixes an issue where the 32bit and 64bit indexes into copro[]
and sys_regs[] are muddled.

Patch 3 adds support for aarch32 accessing the top 32bits of ACTLR_EL1
via ACTLR2. Support for this register is advertised in ID_MMFR4.AC2, which
doesn't get removed by cpufeature. The register is mandatory from v8.2, but
imp-def before then.

Patch 2 stops the sys_regs[] value we use for emulation being save/restored.
This simplifies patch 3 as the aarch32 helper can rely on the in-memory copy.

I think Patch 1 is stable material, I'm not sure about 2&3.


Bonus cans of worms!:

1. How does this copro[] thing work with a big-endian host?
The cp15_regs emulation look fine as nothing uses vcpu_cp15() to read the
register, but wouldn't prepare_fault32() read the wrong end of the register
when using vcpu_cp15()?

2. How does the 32bit fault injection code work with VHE?
vcpu_cp15() modifies the in-memory copy, surely a vcpu_put() will clobber
everything it did, or fail to restore it when entering the guest.


Thanks,

James Morse (3):
  KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  KVM: arm64: Stop save/restoring ACTLR_EL1
  KVM: arm64: Add emulation for 32bit guests accessing ACTLR2

 arch/arm64/include/asm/kvm_host.h    |  1 +
 arch/arm64/kvm/hyp/sysreg-sr.c       |  2 --
 arch/arm64/kvm/sys_regs.c            |  8 +++-----
 arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
 4 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
@ 2020-05-26 16:18 ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, James Morse, Julien Thierry, Suzuki K Poulose

Hello!

Patch 1 fixes an issue where the 32bit and 64bit indexes into copro[]
and sys_regs[] are muddled.

Patch 3 adds support for aarch32 accessing the top 32bits of ACTLR_EL1
via ACTLR2. Support for this register is advertised in ID_MMFR4.AC2, which
doesn't get removed by cpufeature. The register is mandatory from v8.2, but
imp-def before then.

Patch 2 stops the sys_regs[] value we use for emulation being save/restored.
This simplifies patch 3 as the aarch32 helper can rely on the in-memory copy.

I think Patch 1 is stable material, I'm not sure about 2&3.


Bonus cans of worms!:

1. How does this copro[] thing work with a big-endian host?
The cp15_regs emulation look fine as nothing uses vcpu_cp15() to read the
register, but wouldn't prepare_fault32() read the wrong end of the register
when using vcpu_cp15()?

2. How does the 32bit fault injection code work with VHE?
vcpu_cp15() modifies the in-memory copy, surely a vcpu_put() will clobber
everything it did, or fail to restore it when entering the guest.


Thanks,

James Morse (3):
  KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  KVM: arm64: Stop save/restoring ACTLR_EL1
  KVM: arm64: Add emulation for 32bit guests accessing ACTLR2

 arch/arm64/include/asm/kvm_host.h    |  1 +
 arch/arm64/kvm/hyp/sysreg-sr.c       |  2 --
 arch/arm64/kvm/sys_regs.c            |  8 +++-----
 arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
 4 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  2020-05-26 16:18 ` James Morse
  (?)
@ 2020-05-26 16:18   ` James Morse
  -1 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, Julien Thierry, Suzuki K Poulose, James Morse, stable

aarch32 has pairs of registers to access the high and low parts of 64bit
registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
32bit accessors read the high or low part of the 64bit sys_reg[] value
through the union.

Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the accessor
for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit sys_regs[],
and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
array.

cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
c0_CSSELR which is the same location in sys_reg[]. r->reg is 'c0_CSSELR',
index 4 in the 32bit array.

access_csselr() uses the 32bit r->reg value to access the 64bit array,
so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
is subsequently save/restored when we enter the guest.

ACTLR_EL1 is supposed to be read-only for the guest. This register
only affects execution at EL1, and the host's value is restored before
we return to host EL1.

Rename access_csselr() to access_csselr_el1(), to indicate it expects
the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].

Cc: stable@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
----
Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
loaded on the CPU while this code runs. access_csselr_cp15() would have
to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
as well do it in the table.

 arch/arm64/kvm/sys_regs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51db934702b6..2eda539f3281 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
-static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
 	if (p->is_write)
@@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
 	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
-	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
+	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, CSSELR_EL1 },
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
 
 	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
@@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
 };
 
 static const struct sys_reg_desc cp15_64_regs[] = {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-26 16:18   ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel; +Cc: Marc Zyngier, stable

aarch32 has pairs of registers to access the high and low parts of 64bit
registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
32bit accessors read the high or low part of the 64bit sys_reg[] value
through the union.

Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the accessor
for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit sys_regs[],
and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
array.

cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
c0_CSSELR which is the same location in sys_reg[]. r->reg is 'c0_CSSELR',
index 4 in the 32bit array.

access_csselr() uses the 32bit r->reg value to access the 64bit array,
so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
is subsequently save/restored when we enter the guest.

ACTLR_EL1 is supposed to be read-only for the guest. This register
only affects execution at EL1, and the host's value is restored before
we return to host EL1.

Rename access_csselr() to access_csselr_el1(), to indicate it expects
the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].

Cc: stable@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
----
Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
loaded on the CPU while this code runs. access_csselr_cp15() would have
to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
as well do it in the table.

 arch/arm64/kvm/sys_regs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51db934702b6..2eda539f3281 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
-static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
 	if (p->is_write)
@@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
 	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
-	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
+	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, CSSELR_EL1 },
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
 
 	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
@@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
 };
 
 static const struct sys_reg_desc cp15_64_regs[] = {
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-26 16:18   ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, James Morse, stable, Julien Thierry, Suzuki K Poulose

aarch32 has pairs of registers to access the high and low parts of 64bit
registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
32bit accessors read the high or low part of the 64bit sys_reg[] value
through the union.

Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the accessor
for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit sys_regs[],
and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
array.

cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
c0_CSSELR which is the same location in sys_reg[]. r->reg is 'c0_CSSELR',
index 4 in the 32bit array.

access_csselr() uses the 32bit r->reg value to access the 64bit array,
so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
is subsequently save/restored when we enter the guest.

ACTLR_EL1 is supposed to be read-only for the guest. This register
only affects execution at EL1, and the host's value is restored before
we return to host EL1.

Rename access_csselr() to access_csselr_el1(), to indicate it expects
the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].

Cc: stable@vger.kernel.org
Signed-off-by: James Morse <james.morse@arm.com>
----
Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
loaded on the CPU while this code runs. access_csselr_cp15() would have
to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
as well do it in the table.

 arch/arm64/kvm/sys_regs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 51db934702b6..2eda539f3281 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	return true;
 }
 
-static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
 	if (p->is_write)
@@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
 	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
-	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 },
+	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, CSSELR_EL1 },
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
 
 	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
@@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
 };
 
 static const struct sys_reg_desc cp15_64_regs[] = {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
  2020-05-26 16:18 ` James Morse
@ 2020-05-26 16:18   ` James Morse
  -1 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel; +Cc: Marc Zyngier

KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
This means ACTLR* accesses from the guest are always trapped, and
always return the value in the sys_regs array.

The guest can't change the value of these registers, so we are
save restoring the reset value, which came from the host.

Stop save/restoring this register.

This also stops this register being affected by sysregs_loaded_on_cpu,
so we can provide 32 bit accessors that always use the in-memory copy.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
 arch/arm64/kvm/sys_regs.c      | 2 --
 2 files changed, 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 75b1925763f1..57116cf3a1a5 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -44,7 +44,6 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
 {
 	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
 	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
-	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
 	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
 	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
 	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
@@ -133,7 +132,6 @@ static void __hyp_text __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
 		isb();
 	}
 
-	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);
 	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
 	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
 	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2eda539f3281..aae58513025c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
 	switch (reg) {
 	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
 	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
-	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
 	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
 	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
 	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
@@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
 	switch (reg) {
 	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
 	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
-	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
 	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
 	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
 	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
@ 2020-05-26 16:18   ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, James Morse, Julien Thierry, Suzuki K Poulose

KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
This means ACTLR* accesses from the guest are always trapped, and
always return the value in the sys_regs array.

The guest can't change the value of these registers, so we are
save restoring the reset value, which came from the host.

Stop save/restoring this register.

This also stops this register being affected by sysregs_loaded_on_cpu,
so we can provide 32 bit accessors that always use the in-memory copy.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
 arch/arm64/kvm/sys_regs.c      | 2 --
 2 files changed, 4 deletions(-)

diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 75b1925763f1..57116cf3a1a5 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -44,7 +44,6 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
 {
 	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
 	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
-	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
 	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
 	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
 	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
@@ -133,7 +132,6 @@ static void __hyp_text __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
 		isb();
 	}
 
-	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);
 	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
 	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
 	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2eda539f3281..aae58513025c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg)
 	switch (reg) {
 	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
 	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
-	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
 	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
 	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
 	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
@@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
 	switch (reg) {
 	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
 	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
-	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
 	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
 	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
 	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
  2020-05-26 16:18 ` James Morse
@ 2020-05-26 16:18   ` James Morse
  -1 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel; +Cc: Marc Zyngier

ACTLR_EL1 is a 64bit register while the 32bit ACTLR is obviously 32bit.
For 32bit software, the extra bits are accessible via ACTLR2... which
KVM doesn't emulate.

Signed-off-by: James Morse <james.morse@arm.com>
---
I'm not convinced this is endian safe, but it does match what
kvm_inject_undef32() do.

The alternative would be to always read the 64bit value, and generate
the 32bit offets like access_vm_reg() does.

 arch/arm64/include/asm/kvm_host.h    |  1 +
 arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 32c8a675e5a4..5b7538663a8e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -185,6 +185,7 @@ enum vcpu_sysreg {
 #define c0_CSSELR	(CSSELR_EL1 * 2)/* Cache Size Selection Register */
 #define c1_SCTLR	(SCTLR_EL1 * 2)	/* System Control Register */
 #define c1_ACTLR	(ACTLR_EL1 * 2)	/* Auxiliary Control Register */
+#define c1_ACTLR2	(c1_ACTLR + 1)	/* ACTLR top 32 bits */
 #define c1_CPACR	(CPACR_EL1 * 2)	/* Coprocessor Access Control */
 #define c2_TTBR0	(TTBR0_EL1 * 2)	/* Translation Table Base Register 0 */
 #define c2_TTBR0_high	(c2_TTBR0 + 1)	/* TTBR0 top 32 bits */
diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
index 9cb6b4c8355a..ed77bbb48e64 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -30,6 +30,18 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_cp15_actlr(struct kvm_vcpu *vcpu,
+			      struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	p->regval = vcpu_cp15(vcpu, r->reg);
+	return true;
+
+}
+
 static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
@@ -46,7 +58,9 @@ static const struct sys_reg_desc genericv8_sys_regs[] = {
 static const struct sys_reg_desc genericv8_cp15_regs[] = {
 	/* ACTLR */
 	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
-	  access_actlr },
+	  access_cp15_actlr, NULL, c1_ACTLR },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011),
+	  access_cp15_actlr, NULL, c1_ACTLR2 },
 };
 
 static struct kvm_sys_reg_target_table genericv8_target_table = {
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
@ 2020-05-26 16:18   ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-26 16:18 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, James Morse, Julien Thierry, Suzuki K Poulose

ACTLR_EL1 is a 64bit register while the 32bit ACTLR is obviously 32bit.
For 32bit software, the extra bits are accessible via ACTLR2... which
KVM doesn't emulate.

Signed-off-by: James Morse <james.morse@arm.com>
---
I'm not convinced this is endian safe, but it does match what
kvm_inject_undef32() do.

The alternative would be to always read the 64bit value, and generate
the 32bit offets like access_vm_reg() does.

 arch/arm64/include/asm/kvm_host.h    |  1 +
 arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 32c8a675e5a4..5b7538663a8e 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -185,6 +185,7 @@ enum vcpu_sysreg {
 #define c0_CSSELR	(CSSELR_EL1 * 2)/* Cache Size Selection Register */
 #define c1_SCTLR	(SCTLR_EL1 * 2)	/* System Control Register */
 #define c1_ACTLR	(ACTLR_EL1 * 2)	/* Auxiliary Control Register */
+#define c1_ACTLR2	(c1_ACTLR + 1)	/* ACTLR top 32 bits */
 #define c1_CPACR	(CPACR_EL1 * 2)	/* Coprocessor Access Control */
 #define c2_TTBR0	(TTBR0_EL1 * 2)	/* Translation Table Base Register 0 */
 #define c2_TTBR0_high	(c2_TTBR0 + 1)	/* TTBR0 top 32 bits */
diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
index 9cb6b4c8355a..ed77bbb48e64 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -30,6 +30,18 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_cp15_actlr(struct kvm_vcpu *vcpu,
+			      struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	if (p->is_write)
+		return ignore_write(vcpu, p);
+
+	p->regval = vcpu_cp15(vcpu, r->reg);
+	return true;
+
+}
+
 static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
 	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
@@ -46,7 +58,9 @@ static const struct sys_reg_desc genericv8_sys_regs[] = {
 static const struct sys_reg_desc genericv8_cp15_regs[] = {
 	/* ACTLR */
 	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
-	  access_actlr },
+	  access_cp15_actlr, NULL, c1_ACTLR },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011),
+	  access_cp15_actlr, NULL, c1_ACTLR2 },
 };
 
 static struct kvm_sys_reg_target_table genericv8_target_table = {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  2020-05-26 16:18   ` James Morse
  (?)
@ 2020-05-27 16:57     ` Sasha Levin
  -1 siblings, 0 replies; 31+ messages in thread
From: Sasha Levin @ 2020-05-27 16:57 UTC (permalink / raw)
  To: Sasha Levin, James Morse, kvmarm, linux-arm-kernel
  Cc: Marc Zyngier, stable, stable

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224.

v5.6.14: Build OK!
v5.4.42: Build OK!
v4.19.124: Failed to apply! Possible dependencies:
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.14.181: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.9.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    016f98afd050 ("irqchip/gic-v3: Use nops macro for Cavium ThunderX erratum 23154")
    0d449541c185 ("KVM: arm64: use common invariant sysreg definitions")
    0e9884fe63c6 ("arm64: sysreg: subsume GICv3 sysreg definitions")
    14ae7518dd55 ("arm64: sysreg: add register encodings used by KVM")
    47863d41ecf8 ("arm64: sysreg: sort by encoding")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    851050a573e1 ("KVM: arm64: Use common sysreg definitions")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    bca8f17f57bd ("arm64: Get rid of asm/opcodes.h")
    c7a3c61fc606 ("arm64: sysreg: add performance monitor registers")
    c9a3c58f01fb ("KVM: arm64: Add the EL1 physical timer access handler")
    cd9e1927a525 ("arm64: Work around broken .inst when defective gas is detected")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.4.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    06282fd2c2bf ("arm64: KVM: Implement vgic-v2 save/restore")
    068a17a5805d ("arm64: mm: create new fine-grained mappings at boot")
    072f0a633838 ("arm64: Introduce raw_{d,i}cache_line_size")
    0a28714c53fd ("arm64: Use PoU cache instr for I/D coherency")
    116c81f427ff ("arm64: Work around systems with mismatched cache line sizes")
    1431af367e52 ("arm64: KVM: Implement timer save/restore")
    157962f5a8f2 ("arm64: decouple early fixmap init from linear mapping")
    1e48ef7fcc37 ("arm64: add support for building vmlinux as a relocatable PIE binary")
    2a803c4db615 ("arm64: head.S: use memset to clear BSS")
    57f4959bad0a ("arm64: kernel: Add support for User Access Override")
    6d6ec20fcf28 ("arm64: KVM: Implement system register save/restore")
    7b7293ae3dbd ("arm64: Fold proc-macros.S into assembler.h")
    82869ac57b5d ("arm64: kernel: Add support for hibernate/suspend-to-disk")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    8eb992674c9e ("arm64: KVM: Implement debug save/restore")
    910917bb7db0 ("arm64: KVM: Map the kernel RO section into HYP")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    9e8e865bbe29 ("arm64: unify idmap removal")
    a0bf9776cd0b ("arm64: kvm: deal with kernel symbols outside of linear mapping")
    a7f8de168ace ("arm64: allow kernel Image to be loaded anywhere in physical memory")
    ab893fb9f1b1 ("arm64: introduce KIMAGE_VADDR as the virtual base of the kernel region")
    adc9b2dfd009 ("arm64: kernel: Rework finisher callback out of __cpu_suspend_enter()")
    b3122023df93 ("arm64: Fix an enum typo in mm/dump.c")
    b97b66c14b96 ("arm64: KVM: Implement guest entry")
    be901e9b15cd ("arm64: KVM: Implement the core world switch")
    c1a88e9124a4 ("arm64: kasan: avoid TLB conflicts")
    c76a0a6695c6 ("arm64: KVM: Add a HYP-specific header file")
    d5370f754875 ("arm64: prefetch: add alternative pattern for CPUs without a prefetcher")
    f68d2b1b73cc ("arm64: KVM: Implement vgic-v3 save/restore")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")
    f80fb3a3d508 ("arm64: add support for kernel ASLR")
    f9040773b7bb ("arm64: move kernel image to base of vmalloc area")
    fd045f6cd98e ("arm64: add support for module PLTs")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-27 16:57     ` Sasha Levin
  0 siblings, 0 replies; 31+ messages in thread
From: Sasha Levin @ 2020-05-27 16:57 UTC (permalink / raw)
  To: Sasha Levin, James Morse, kvmarm, linux-arm-kernel; +Cc: Marc Zyngier, stable

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224.

v5.6.14: Build OK!
v5.4.42: Build OK!
v4.19.124: Failed to apply! Possible dependencies:
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.14.181: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.9.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    016f98afd050 ("irqchip/gic-v3: Use nops macro for Cavium ThunderX erratum 23154")
    0d449541c185 ("KVM: arm64: use common invariant sysreg definitions")
    0e9884fe63c6 ("arm64: sysreg: subsume GICv3 sysreg definitions")
    14ae7518dd55 ("arm64: sysreg: add register encodings used by KVM")
    47863d41ecf8 ("arm64: sysreg: sort by encoding")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    851050a573e1 ("KVM: arm64: Use common sysreg definitions")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    bca8f17f57bd ("arm64: Get rid of asm/opcodes.h")
    c7a3c61fc606 ("arm64: sysreg: add performance monitor registers")
    c9a3c58f01fb ("KVM: arm64: Add the EL1 physical timer access handler")
    cd9e1927a525 ("arm64: Work around broken .inst when defective gas is detected")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.4.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    06282fd2c2bf ("arm64: KVM: Implement vgic-v2 save/restore")
    068a17a5805d ("arm64: mm: create new fine-grained mappings at boot")
    072f0a633838 ("arm64: Introduce raw_{d,i}cache_line_size")
    0a28714c53fd ("arm64: Use PoU cache instr for I/D coherency")
    116c81f427ff ("arm64: Work around systems with mismatched cache line sizes")
    1431af367e52 ("arm64: KVM: Implement timer save/restore")
    157962f5a8f2 ("arm64: decouple early fixmap init from linear mapping")
    1e48ef7fcc37 ("arm64: add support for building vmlinux as a relocatable PIE binary")
    2a803c4db615 ("arm64: head.S: use memset to clear BSS")
    57f4959bad0a ("arm64: kernel: Add support for User Access Override")
    6d6ec20fcf28 ("arm64: KVM: Implement system register save/restore")
    7b7293ae3dbd ("arm64: Fold proc-macros.S into assembler.h")
    82869ac57b5d ("arm64: kernel: Add support for hibernate/suspend-to-disk")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    8eb992674c9e ("arm64: KVM: Implement debug save/restore")
    910917bb7db0 ("arm64: KVM: Map the kernel RO section into HYP")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    9e8e865bbe29 ("arm64: unify idmap removal")
    a0bf9776cd0b ("arm64: kvm: deal with kernel symbols outside of linear mapping")
    a7f8de168ace ("arm64: allow kernel Image to be loaded anywhere in physical memory")
    ab893fb9f1b1 ("arm64: introduce KIMAGE_VADDR as the virtual base of the kernel region")
    adc9b2dfd009 ("arm64: kernel: Rework finisher callback out of __cpu_suspend_enter()")
    b3122023df93 ("arm64: Fix an enum typo in mm/dump.c")
    b97b66c14b96 ("arm64: KVM: Implement guest entry")
    be901e9b15cd ("arm64: KVM: Implement the core world switch")
    c1a88e9124a4 ("arm64: kasan: avoid TLB conflicts")
    c76a0a6695c6 ("arm64: KVM: Add a HYP-specific header file")
    d5370f754875 ("arm64: prefetch: add alternative pattern for CPUs without a prefetcher")
    f68d2b1b73cc ("arm64: KVM: Implement vgic-v3 save/restore")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")
    f80fb3a3d508 ("arm64: add support for kernel ASLR")
    f9040773b7bb ("arm64: move kernel image to base of vmalloc area")
    fd045f6cd98e ("arm64: add support for module PLTs")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-27 16:57     ` Sasha Levin
  0 siblings, 0 replies; 31+ messages in thread
From: Sasha Levin @ 2020-05-27 16:57 UTC (permalink / raw)
  To: Sasha Levin, James Morse, kvmarm, linux-arm-kernel; +Cc: Marc Zyngier, stable

Hi

[This is an automated email]

This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all

The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224.

v5.6.14: Build OK!
v5.4.42: Build OK!
v4.19.124: Failed to apply! Possible dependencies:
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.14.181: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.9.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    016f98afd050 ("irqchip/gic-v3: Use nops macro for Cavium ThunderX erratum 23154")
    0d449541c185 ("KVM: arm64: use common invariant sysreg definitions")
    0e9884fe63c6 ("arm64: sysreg: subsume GICv3 sysreg definitions")
    14ae7518dd55 ("arm64: sysreg: add register encodings used by KVM")
    47863d41ecf8 ("arm64: sysreg: sort by encoding")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    851050a573e1 ("KVM: arm64: Use common sysreg definitions")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    bca8f17f57bd ("arm64: Get rid of asm/opcodes.h")
    c7a3c61fc606 ("arm64: sysreg: add performance monitor registers")
    c9a3c58f01fb ("KVM: arm64: Add the EL1 physical timer access handler")
    cd9e1927a525 ("arm64: Work around broken .inst when defective gas is detected")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")

v4.4.224: Failed to apply! Possible dependencies:
    005781be127f ("arm64: KVM: Move CPU ID reg trap setup off the world switch path")
    06282fd2c2bf ("arm64: KVM: Implement vgic-v2 save/restore")
    068a17a5805d ("arm64: mm: create new fine-grained mappings at boot")
    072f0a633838 ("arm64: Introduce raw_{d,i}cache_line_size")
    0a28714c53fd ("arm64: Use PoU cache instr for I/D coherency")
    116c81f427ff ("arm64: Work around systems with mismatched cache line sizes")
    1431af367e52 ("arm64: KVM: Implement timer save/restore")
    157962f5a8f2 ("arm64: decouple early fixmap init from linear mapping")
    1e48ef7fcc37 ("arm64: add support for building vmlinux as a relocatable PIE binary")
    2a803c4db615 ("arm64: head.S: use memset to clear BSS")
    57f4959bad0a ("arm64: kernel: Add support for User Access Override")
    6d6ec20fcf28 ("arm64: KVM: Implement system register save/restore")
    7b7293ae3dbd ("arm64: Fold proc-macros.S into assembler.h")
    82869ac57b5d ("arm64: kernel: Add support for hibernate/suspend-to-disk")
    82e0191a1aa1 ("arm64: Support systems without FP/ASIMD")
    8eb992674c9e ("arm64: KVM: Implement debug save/restore")
    910917bb7db0 ("arm64: KVM: Map the kernel RO section into HYP")
    93390c0a1b20 ("arm64: KVM: Hide unsupported AArch64 CPU features from guests")
    9e8e865bbe29 ("arm64: unify idmap removal")
    a0bf9776cd0b ("arm64: kvm: deal with kernel symbols outside of linear mapping")
    a7f8de168ace ("arm64: allow kernel Image to be loaded anywhere in physical memory")
    ab893fb9f1b1 ("arm64: introduce KIMAGE_VADDR as the virtual base of the kernel region")
    adc9b2dfd009 ("arm64: kernel: Rework finisher callback out of __cpu_suspend_enter()")
    b3122023df93 ("arm64: Fix an enum typo in mm/dump.c")
    b97b66c14b96 ("arm64: KVM: Implement guest entry")
    be901e9b15cd ("arm64: KVM: Implement the core world switch")
    c1a88e9124a4 ("arm64: kasan: avoid TLB conflicts")
    c76a0a6695c6 ("arm64: KVM: Add a HYP-specific header file")
    d5370f754875 ("arm64: prefetch: add alternative pattern for CPUs without a prefetcher")
    f68d2b1b73cc ("arm64: KVM: Implement vgic-v3 save/restore")
    f7f2b15c3d42 ("arm64: KVM: Expose sanitised cache type register to guest")
    f80fb3a3d508 ("arm64: add support for kernel ASLR")
    f9040773b7bb ("arm64: move kernel image to base of vmalloc area")
    fd045f6cd98e ("arm64: add support for module PLTs")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  2020-05-26 16:18   ` James Morse
  (?)
@ 2020-05-28  8:57     ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28  8:57 UTC (permalink / raw)
  To: James Morse
  Cc: kvmarm, linux-arm-kernel, Julien Thierry, Suzuki K Poulose, stable

Hi James,

On 2020-05-26 17:18, James Morse wrote:
> aarch32 has pairs of registers to access the high and low parts of 
> 64bit
> registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
> 32bit accessors read the high or low part of the 64bit sys_reg[] value
> through the union.
> 
> Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the 
> accessor
> for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit 
> sys_regs[],
> and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
> array.
> 
> cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
> c0_CSSELR which is the same location in sys_reg[]. r->reg is 
> 'c0_CSSELR',
> index 4 in the 32bit array.
> 
> access_csselr() uses the 32bit r->reg value to access the 64bit array,
> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
> is subsequently save/restored when we enter the guest.

Huhuh... Nice catch.

> 
> ACTLR_EL1 is supposed to be read-only for the guest. This register
> only affects execution at EL1, and the host's value is restored before
> we return to host EL1.
> 
> Rename access_csselr() to access_csselr_el1(), to indicate it expects
> the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: James Morse <james.morse@arm.com>
> ----
> Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
> loaded on the CPU while this code runs. access_csselr_cp15() would have
> to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
> as well do it in the table.
> 
>  arch/arm64/kvm/sys_regs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 51db934702b6..2eda539f3281 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
>  	return true;
>  }
> 
> -static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
> *p,
> +static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct 
> sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
>  	if (p->is_write)
> @@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] 
> = {
> 
>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
> -	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 
> },
> +	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, 
> CSSELR_EL1 },
>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> 
>  	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
> CSSELR_EL1 },
>  };
> 
>  static const struct sys_reg_desc cp15_64_regs[] = {

This is a departure from the way we deal with 32bit CP15 registers.
We deal with this exact issue in a very different way for other
CP15 regs, by adjusting the index in the sys_regs array (see the
way we handle the VM regs).

How about something like this (untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 12d07e7ced82..515c0c11a668 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1321,10 +1321,16 @@ static bool access_clidr(struct kvm_vcpu *vcpu, 
struct sys_reg_params *p,
  static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
*p,
  			  const struct sys_reg_desc *r)
  {
+	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, r->reg);
+		vcpu_write_sys_reg(vcpu, p->regval, reg);
  	else
-		p->regval = vcpu_read_sys_reg(vcpu, r->reg);
+		p->regval = vcpu_read_sys_reg(vcpu, reg);
  	return true;
  }

Ideally, I'd like the core sys_reg code to deal with this sort
of funnies, but I'm trying to keep the change minimal...

Thanks,

          M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28  8:57     ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28  8:57 UTC (permalink / raw)
  To: James Morse; +Cc: stable, kvmarm, linux-arm-kernel

Hi James,

On 2020-05-26 17:18, James Morse wrote:
> aarch32 has pairs of registers to access the high and low parts of 
> 64bit
> registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
> 32bit accessors read the high or low part of the 64bit sys_reg[] value
> through the union.
> 
> Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the 
> accessor
> for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit 
> sys_regs[],
> and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
> array.
> 
> cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
> c0_CSSELR which is the same location in sys_reg[]. r->reg is 
> 'c0_CSSELR',
> index 4 in the 32bit array.
> 
> access_csselr() uses the 32bit r->reg value to access the 64bit array,
> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
> is subsequently save/restored when we enter the guest.

Huhuh... Nice catch.

> 
> ACTLR_EL1 is supposed to be read-only for the guest. This register
> only affects execution at EL1, and the host's value is restored before
> we return to host EL1.
> 
> Rename access_csselr() to access_csselr_el1(), to indicate it expects
> the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: James Morse <james.morse@arm.com>
> ----
> Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
> loaded on the CPU while this code runs. access_csselr_cp15() would have
> to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
> as well do it in the table.
> 
>  arch/arm64/kvm/sys_regs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 51db934702b6..2eda539f3281 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
>  	return true;
>  }
> 
> -static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
> *p,
> +static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct 
> sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
>  	if (p->is_write)
> @@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] 
> = {
> 
>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
> -	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 
> },
> +	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, 
> CSSELR_EL1 },
>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> 
>  	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
> CSSELR_EL1 },
>  };
> 
>  static const struct sys_reg_desc cp15_64_regs[] = {

This is a departure from the way we deal with 32bit CP15 registers.
We deal with this exact issue in a very different way for other
CP15 regs, by adjusting the index in the sys_regs array (see the
way we handle the VM regs).

How about something like this (untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 12d07e7ced82..515c0c11a668 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1321,10 +1321,16 @@ static bool access_clidr(struct kvm_vcpu *vcpu, 
struct sys_reg_params *p,
  static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
*p,
  			  const struct sys_reg_desc *r)
  {
+	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, r->reg);
+		vcpu_write_sys_reg(vcpu, p->regval, reg);
  	else
-		p->regval = vcpu_read_sys_reg(vcpu, r->reg);
+		p->regval = vcpu_read_sys_reg(vcpu, reg);
  	return true;
  }

Ideally, I'd like the core sys_reg code to deal with this sort
of funnies, but I'm trying to keep the change minimal...

Thanks,

          M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28  8:57     ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28  8:57 UTC (permalink / raw)
  To: James Morse
  Cc: stable, Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

Hi James,

On 2020-05-26 17:18, James Morse wrote:
> aarch32 has pairs of registers to access the high and low parts of 
> 64bit
> registers. KVM has a union of 64bit sys_regs[] and 32bit copro[]. The
> 32bit accessors read the high or low part of the 64bit sys_reg[] value
> through the union.
> 
> Both sys_reg_descs[] and cp15_regs[] list access_csselr() as the 
> accessor
> for CSSELR{,_EL1}. access_csselr() is only aware of the 64bit 
> sys_regs[],
> and expects r->reg to be 'CSSELR_EL1' in the enum, index 2 of the 64bit
> array.
> 
> cp15_regs[] uses the 32bit copro[] alias of sys_regs[]. Here CSSELR is
> c0_CSSELR which is the same location in sys_reg[]. r->reg is 
> 'c0_CSSELR',
> index 4 in the 32bit array.
> 
> access_csselr() uses the 32bit r->reg value to access the 64bit array,
> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
> is subsequently save/restored when we enter the guest.

Huhuh... Nice catch.

> 
> ACTLR_EL1 is supposed to be read-only for the guest. This register
> only affects execution at EL1, and the host's value is restored before
> we return to host EL1.
> 
> Rename access_csselr() to access_csselr_el1(), to indicate it expects
> the 64bit register index, and pass it CSSELR_EL1 from cp15_regs[].
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: James Morse <james.morse@arm.com>
> ----
> Providing access_csselr_cp15() wouldn't work as with VHE CSSELR_EL1 is
> loaded on the CPU while this code runs. access_csselr_cp15() would have
> to map it back the 64bit resgister to use vcpu_write_sys_reg(). We may
> as well do it in the table.
> 
>  arch/arm64/kvm/sys_regs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 51db934702b6..2eda539f3281 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1302,7 +1302,7 @@ static bool access_clidr(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
>  	return true;
>  }
> 
> -static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
> *p,
> +static bool access_csselr_el1(struct kvm_vcpu *vcpu, struct 
> sys_reg_params *p,
>  			  const struct sys_reg_desc *r)
>  {
>  	if (p->is_write)
> @@ -1566,7 +1566,7 @@ static const struct sys_reg_desc sys_reg_descs[] 
> = {
> 
>  	{ SYS_DESC(SYS_CCSIDR_EL1), access_ccsidr },
>  	{ SYS_DESC(SYS_CLIDR_EL1), access_clidr },
> -	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr, reset_unknown, CSSELR_EL1 
> },
> +	{ SYS_DESC(SYS_CSSELR_EL1), access_csselr_el1, reset_unknown, 
> CSSELR_EL1 },
>  	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
> 
>  	{ SYS_DESC(SYS_PMCR_EL0), access_pmcr, reset_pmcr, PMCR_EL0 },
> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
> CSSELR_EL1 },
>  };
> 
>  static const struct sys_reg_desc cp15_64_regs[] = {

This is a departure from the way we deal with 32bit CP15 registers.
We deal with this exact issue in a very different way for other
CP15 regs, by adjusting the index in the sys_regs array (see the
way we handle the VM regs).

How about something like this (untested):

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 12d07e7ced82..515c0c11a668 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1321,10 +1321,16 @@ static bool access_clidr(struct kvm_vcpu *vcpu, 
struct sys_reg_params *p,
  static bool access_csselr(struct kvm_vcpu *vcpu, struct sys_reg_params 
*p,
  			  const struct sys_reg_desc *r)
  {
+	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, r->reg);
+		vcpu_write_sys_reg(vcpu, p->regval, reg);
  	else
-		p->regval = vcpu_read_sys_reg(vcpu, r->reg);
+		p->regval = vcpu_read_sys_reg(vcpu, reg);
  	return true;
  }

Ideally, I'd like the core sys_reg code to deal with this sort
of funnies, but I'm trying to keep the change minimal...

Thanks,

          M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  2020-05-28  8:57     ` Marc Zyngier
  (?)
@ 2020-05-28 11:59       ` James Morse
  -1 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-28 11:59 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, linux-arm-kernel, Julien Thierry, Suzuki K Poulose, stable

Hi Marc,

On 28/05/2020 09:57, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> access_csselr() uses the 32bit r->reg value to access the 64bit array,
>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>> is subsequently save/restored when we enter the guest.

>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 51db934702b6..2eda539f3281 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
>>  };

> This is a departure from the way we deal with 32bit CP15 registers.
> We deal with this exact issue in a very different way for other
> CP15 regs, by adjusting the index in the sys_regs array (see the
> way we handle the VM regs).
> 
> How about something like this (untested):

[like access_vm_reg() does]

Sure, I'll give that a test and re-post it.


> Ideally, I'd like the core sys_reg code to deal with this sort
> of funnies, but I'm trying to keep the change minimal...

Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg() that calls
vcpu_write_sys_reg()? (/me hunts out the todo list)


Thanks,

James

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28 11:59       ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-28 11:59 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: stable, kvmarm, linux-arm-kernel

Hi Marc,

On 28/05/2020 09:57, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> access_csselr() uses the 32bit r->reg value to access the 64bit array,
>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>> is subsequently save/restored when we enter the guest.

>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 51db934702b6..2eda539f3281 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
>>  };

> This is a departure from the way we deal with 32bit CP15 registers.
> We deal with this exact issue in a very different way for other
> CP15 regs, by adjusting the index in the sys_regs array (see the
> way we handle the VM regs).
> 
> How about something like this (untested):

[like access_vm_reg() does]

Sure, I'll give that a test and re-post it.


> Ideally, I'd like the core sys_reg code to deal with this sort
> of funnies, but I'm trying to keep the change minimal...

Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg() that calls
vcpu_write_sys_reg()? (/me hunts out the todo list)


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28 11:59       ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-28 11:59 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: stable, Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

Hi Marc,

On 28/05/2020 09:57, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> access_csselr() uses the 32bit r->reg value to access the 64bit array,
>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>> is subsequently save/restored when we enter the guest.

>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 51db934702b6..2eda539f3281 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, CSSELR_EL1 },
>>  };

> This is a departure from the way we deal with 32bit CP15 registers.
> We deal with this exact issue in a very different way for other
> CP15 regs, by adjusting the index in the sys_regs array (see the
> way we handle the VM regs).
> 
> How about something like this (untested):

[like access_vm_reg() does]

Sure, I'll give that a test and re-post it.


> Ideally, I'd like the core sys_reg code to deal with this sort
> of funnies, but I'm trying to keep the change minimal...

Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg() that calls
vcpu_write_sys_reg()? (/me hunts out the todo list)


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
  2020-05-28 11:59       ` James Morse
  (?)
@ 2020-05-28 12:10         ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:10 UTC (permalink / raw)
  To: James Morse
  Cc: kvmarm, linux-arm-kernel, Julien Thierry, Suzuki K Poulose, stable

On 2020-05-28 12:59, James Morse wrote:
> Hi Marc,
> 
> On 28/05/2020 09:57, Marc Zyngier wrote:
>> On 2020-05-26 17:18, James Morse wrote:
>>> access_csselr() uses the 32bit r->reg value to access the 64bit 
>>> array,
>>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>>> is subsequently save/restored when we enter the guest.
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 51db934702b6..2eda539f3281 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
>>> CSSELR_EL1 },
>>>  };
> 
>> This is a departure from the way we deal with 32bit CP15 registers.
>> We deal with this exact issue in a very different way for other
>> CP15 regs, by adjusting the index in the sys_regs array (see the
>> way we handle the VM regs).
>> 
>> How about something like this (untested):
> 
> [like access_vm_reg() does]
> 
> Sure, I'll give that a test and re-post it.

Thanks!

> 
> 
>> Ideally, I'd like the core sys_reg code to deal with this sort
>> of funnies, but I'm trying to keep the change minimal...
> 
> Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg()
> that calls
> vcpu_write_sys_reg()? (/me hunts out the todo list)

I was thinking of hiding it differently: in emulate_cp, substitute the
sys_reg_desc structure for a temporary one that represents the 64bit
version, and make it completely transparent.

I'm sure there is a couple of nits around that though...

         M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28 12:10         ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:10 UTC (permalink / raw)
  To: James Morse; +Cc: stable, kvmarm, linux-arm-kernel

On 2020-05-28 12:59, James Morse wrote:
> Hi Marc,
> 
> On 28/05/2020 09:57, Marc Zyngier wrote:
>> On 2020-05-26 17:18, James Morse wrote:
>>> access_csselr() uses the 32bit r->reg value to access the 64bit 
>>> array,
>>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>>> is subsequently save/restored when we enter the guest.
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 51db934702b6..2eda539f3281 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
>>> CSSELR_EL1 },
>>>  };
> 
>> This is a departure from the way we deal with 32bit CP15 registers.
>> We deal with this exact issue in a very different way for other
>> CP15 regs, by adjusting the index in the sys_regs array (see the
>> way we handle the VM regs).
>> 
>> How about something like this (untested):
> 
> [like access_vm_reg() does]
> 
> Sure, I'll give that a test and re-post it.

Thanks!

> 
> 
>> Ideally, I'd like the core sys_reg code to deal with this sort
>> of funnies, but I'm trying to keep the change minimal...
> 
> Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg()
> that calls
> vcpu_write_sys_reg()? (/me hunts out the todo list)

I was thinking of hiding it differently: in emulate_cp, substitute the
sys_reg_desc structure for a temporary one that represents the 64bit
version, and make it completely transparent.

I'm sure there is a couple of nits around that though...

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
@ 2020-05-28 12:10         ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:10 UTC (permalink / raw)
  To: James Morse
  Cc: stable, Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

On 2020-05-28 12:59, James Morse wrote:
> Hi Marc,
> 
> On 28/05/2020 09:57, Marc Zyngier wrote:
>> On 2020-05-26 17:18, James Morse wrote:
>>> access_csselr() uses the 32bit r->reg value to access the 64bit 
>>> array,
>>> so reads and write the wrong value. sys_regs[4], is ACTLR_EL1, which
>>> is subsequently save/restored when we enter the guest.
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 51db934702b6..2eda539f3281 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -2060,7 +2060,7 @@ 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_el1, NULL, 
>>> CSSELR_EL1 },
>>>  };
> 
>> This is a departure from the way we deal with 32bit CP15 registers.
>> We deal with this exact issue in a very different way for other
>> CP15 regs, by adjusting the index in the sys_regs array (see the
>> way we handle the VM regs).
>> 
>> How about something like this (untested):
> 
> [like access_vm_reg() does]
> 
> Sure, I'll give that a test and re-post it.

Thanks!

> 
> 
>> Ideally, I'd like the core sys_reg code to deal with this sort
>> of funnies, but I'm trying to keep the change minimal...
> 
> Roll this '/2' and upper/lower bits stuff into a vcpu_write_cp15_reg()
> that calls
> vcpu_write_sys_reg()? (/me hunts out the todo list)

I was thinking of hiding it differently: in emulate_cp, substitute the
sys_reg_desc structure for a temporary one that represents the 64bit
version, and make it completely transparent.

I'm sure there is a couple of nits around that though...

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
  2020-05-26 16:18   ` James Morse
@ 2020-05-28 12:36     ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:36 UTC (permalink / raw)
  To: James Morse; +Cc: kvmarm, linux-arm-kernel

On 2020-05-26 17:18, James Morse wrote:
> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.

TAC is a leftover from 32bit.

> This means ACTLR* accesses from the guest are always trapped, and
> always return the value in the sys_regs array.
> 
> The guest can't change the value of these registers, so we are
> save restoring the reset value, which came from the host.
> 
> Stop save/restoring this register.
> 
> This also stops this register being affected by sysregs_loaded_on_cpu,
> so we can provide 32 bit accessors that always use the in-memory copy.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
>  arch/arm64/kvm/sys_regs.c      | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c 
> b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 75b1925763f1..57116cf3a1a5 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -44,7 +44,6 @@ static void __hyp_text
> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  {
>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
> @@ -133,7 +132,6 @@ static void __hyp_text
> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>  		isb();
>  	}
> 
> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);

If we don't need to save/restore it, we can also drop its presence
in the sys_regs array.

>  	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
>  	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
>  	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2eda539f3281..aae58513025c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, 
> int reg)
>  	switch (reg) {
>  	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
>  	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
> -	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
>  	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
>  	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
>  	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
> val, int reg)
>  	switch (reg) {
>  	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
>  	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
> -	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
>  	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
>  	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
>  	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;

It strikes me that we don't even have a trap handler for this sysreg,
whether it is 32 or 64bit... That's a bit unfortunate, to say the
least...

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
@ 2020-05-28 12:36     ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:36 UTC (permalink / raw)
  To: James Morse; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

On 2020-05-26 17:18, James Morse wrote:
> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.

TAC is a leftover from 32bit.

> This means ACTLR* accesses from the guest are always trapped, and
> always return the value in the sys_regs array.
> 
> The guest can't change the value of these registers, so we are
> save restoring the reset value, which came from the host.
> 
> Stop save/restoring this register.
> 
> This also stops this register being affected by sysregs_loaded_on_cpu,
> so we can provide 32 bit accessors that always use the in-memory copy.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
>  arch/arm64/kvm/sys_regs.c      | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c 
> b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 75b1925763f1..57116cf3a1a5 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -44,7 +44,6 @@ static void __hyp_text
> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  {
>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
> @@ -133,7 +132,6 @@ static void __hyp_text
> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>  		isb();
>  	}
> 
> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);

If we don't need to save/restore it, we can also drop its presence
in the sys_regs array.

>  	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
>  	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
>  	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2eda539f3281..aae58513025c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, 
> int reg)
>  	switch (reg) {
>  	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
>  	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
> -	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
>  	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
>  	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
>  	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
> val, int reg)
>  	switch (reg) {
>  	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
>  	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
> -	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
>  	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
>  	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
>  	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;

It strikes me that we don't even have a trap handler for this sysreg,
whether it is 32 or 64bit... That's a bit unfortunate, to say the
least...

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
  2020-05-28 12:36     ` Marc Zyngier
@ 2020-05-28 12:38       ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:38 UTC (permalink / raw)
  To: James Morse; +Cc: kvmarm, linux-arm-kernel

On 2020-05-28 13:36, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
> 
> TAC is a leftover from 32bit.
> 
>> This means ACTLR* accesses from the guest are always trapped, and
>> always return the value in the sys_regs array.
>> 
>> The guest can't change the value of these registers, so we are
>> save restoring the reset value, which came from the host.
>> 
>> Stop save/restoring this register.
>> 
>> This also stops this register being affected by sysregs_loaded_on_cpu,
>> so we can provide 32 bit accessors that always use the in-memory copy.
>> 
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
>>  arch/arm64/kvm/sys_regs.c      | 2 --
>>  2 files changed, 4 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c 
>> b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 75b1925763f1..57116cf3a1a5 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -44,7 +44,6 @@ static void __hyp_text
>> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>>  {
>>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
>> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
>>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
>>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
>> @@ -133,7 +132,6 @@ static void __hyp_text
>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>  		isb();
>>  	}
>> 
>> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);
> 
> If we don't need to save/restore it, we can also drop its presence
> in the sys_regs array.
> 
>>  	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2eda539f3281..aae58513025c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, 
>> int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
>>  	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
>> -	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
>>  	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
>>  	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
>>  	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
>> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
>> val, int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
>>  	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
>> -	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
>>  	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
>>  	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
>>  	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;
> 
> It strikes me that we don't even have a trap handler for this sysreg,
> whether it is 32 or 64bit... That's a bit unfortunate, to say the
> least...

Ah, no. the sucker is hidden away in "generic_v8"...

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
@ 2020-05-28 12:38       ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:38 UTC (permalink / raw)
  To: James Morse; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

On 2020-05-28 13:36, Marc Zyngier wrote:
> On 2020-05-26 17:18, James Morse wrote:
>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
> 
> TAC is a leftover from 32bit.
> 
>> This means ACTLR* accesses from the guest are always trapped, and
>> always return the value in the sys_regs array.
>> 
>> The guest can't change the value of these registers, so we are
>> save restoring the reset value, which came from the host.
>> 
>> Stop save/restoring this register.
>> 
>> This also stops this register being affected by sysregs_loaded_on_cpu,
>> so we can provide 32 bit accessors that always use the in-memory copy.
>> 
>> Signed-off-by: James Morse <james.morse@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/sysreg-sr.c | 2 --
>>  arch/arm64/kvm/sys_regs.c      | 2 --
>>  2 files changed, 4 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c 
>> b/arch/arm64/kvm/hyp/sysreg-sr.c
>> index 75b1925763f1..57116cf3a1a5 100644
>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
>> @@ -44,7 +44,6 @@ static void __hyp_text
>> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>>  {
>>  	ctxt->sys_regs[CSSELR_EL1]	= read_sysreg(csselr_el1);
>>  	ctxt->sys_regs[SCTLR_EL1]	= read_sysreg_el1(SYS_SCTLR);
>> -	ctxt->sys_regs[ACTLR_EL1]	= read_sysreg(actlr_el1);
>>  	ctxt->sys_regs[CPACR_EL1]	= read_sysreg_el1(SYS_CPACR);
>>  	ctxt->sys_regs[TTBR0_EL1]	= read_sysreg_el1(SYS_TTBR0);
>>  	ctxt->sys_regs[TTBR1_EL1]	= read_sysreg_el1(SYS_TTBR1);
>> @@ -133,7 +132,6 @@ static void __hyp_text
>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>  		isb();
>>  	}
>> 
>> -	write_sysreg(ctxt->sys_regs[ACTLR_EL1],		actlr_el1);
> 
> If we don't need to save/restore it, we can also drop its presence
> in the sys_regs array.
> 
>>  	write_sysreg_el1(ctxt->sys_regs[CPACR_EL1],	SYS_CPACR);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1],	SYS_TTBR0);
>>  	write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1],	SYS_TTBR1);
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2eda539f3281..aae58513025c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, 
>> int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	return read_sysreg_s(SYS_CSSELR_EL1);
>>  	case SCTLR_EL1:		return read_sysreg_s(SYS_SCTLR_EL12);
>> -	case ACTLR_EL1:		return read_sysreg_s(SYS_ACTLR_EL1);
>>  	case CPACR_EL1:		return read_sysreg_s(SYS_CPACR_EL12);
>>  	case TTBR0_EL1:		return read_sysreg_s(SYS_TTBR0_EL12);
>>  	case TTBR1_EL1:		return read_sysreg_s(SYS_TTBR1_EL12);
>> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64
>> val, int reg)
>>  	switch (reg) {
>>  	case CSSELR_EL1:	write_sysreg_s(val, SYS_CSSELR_EL1);	return;
>>  	case SCTLR_EL1:		write_sysreg_s(val, SYS_SCTLR_EL12);	return;
>> -	case ACTLR_EL1:		write_sysreg_s(val, SYS_ACTLR_EL1);	return;
>>  	case CPACR_EL1:		write_sysreg_s(val, SYS_CPACR_EL12);	return;
>>  	case TTBR0_EL1:		write_sysreg_s(val, SYS_TTBR0_EL12);	return;
>>  	case TTBR1_EL1:		write_sysreg_s(val, SYS_TTBR1_EL12);	return;
> 
> It strikes me that we don't even have a trap handler for this sysreg,
> whether it is 32 or 64bit... That's a bit unfortunate, to say the
> least...

Ah, no. the sucker is hidden away in "generic_v8"...

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
  2020-05-26 16:18   ` James Morse
@ 2020-05-28 12:51     ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:51 UTC (permalink / raw)
  To: James Morse; +Cc: kvmarm, linux-arm-kernel

Hi James,

On 2020-05-26 17:18, James Morse wrote:
> ACTLR_EL1 is a 64bit register while the 32bit ACTLR is obviously 32bit.
> For 32bit software, the extra bits are accessible via ACTLR2... which
> KVM doesn't emulate.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> I'm not convinced this is endian safe, but it does match what
> kvm_inject_undef32() do.
> 
> The alternative would be to always read the 64bit value, and generate
> the 32bit offets like access_vm_reg() does.
> 
>  arch/arm64/include/asm/kvm_host.h    |  1 +
>  arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 32c8a675e5a4..5b7538663a8e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -185,6 +185,7 @@ enum vcpu_sysreg {
>  #define c0_CSSELR	(CSSELR_EL1 * 2)/* Cache Size Selection Register */
>  #define c1_SCTLR	(SCTLR_EL1 * 2)	/* System Control Register */
>  #define c1_ACTLR	(ACTLR_EL1 * 2)	/* Auxiliary Control Register */
> +#define c1_ACTLR2	(c1_ACTLR + 1)	/* ACTLR top 32 bits */
>  #define c1_CPACR	(CPACR_EL1 * 2)	/* Coprocessor Access Control */
>  #define c2_TTBR0	(TTBR0_EL1 * 2)	/* Translation Table Base Register 0 
> */
>  #define c2_TTBR0_high	(c2_TTBR0 + 1)	/* TTBR0 top 32 bits */
> diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c
> b/arch/arm64/kvm/sys_regs_generic_v8.c
> index 9cb6b4c8355a..ed77bbb48e64 100644
> --- a/arch/arm64/kvm/sys_regs_generic_v8.c
> +++ b/arch/arm64/kvm/sys_regs_generic_v8.c
> @@ -30,6 +30,18 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
>  	return true;
>  }
> 
> +static bool access_cp15_actlr(struct kvm_vcpu *vcpu,
> +			      struct sys_reg_params *p,
> +			      const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return ignore_write(vcpu, p);
> +
> +	p->regval = vcpu_cp15(vcpu, r->reg);
> +	return true;
> +
> +}
> +
>  static void reset_actlr(struct kvm_vcpu *vcpu, const struct 
> sys_reg_desc *r)
>  {
>  	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
> @@ -46,7 +58,9 @@ static const struct sys_reg_desc genericv8_sys_regs[] 
> = {
>  static const struct sys_reg_desc genericv8_cp15_regs[] = {
>  	/* ACTLR */
>  	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
> -	  access_actlr },
> +	  access_cp15_actlr, NULL, c1_ACTLR },
> +	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011),
> +	  access_cp15_actlr, NULL, c1_ACTLR2 },
>  };
> 
>  static struct kvm_sys_reg_target_table genericv8_target_table = {

I'd get rid of any form of storage, and go with something like 
(untested, again):

diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c 
b/arch/arm64/kvm/sys_regs_generic_v8.c
index 9cb6b4c8355a..1b2bf2d37612 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -26,13 +26,16 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
  	if (p->is_write)
  		return ignore_write(vcpu, p);

-	p->regval = vcpu_read_sys_reg(vcpu, ACTLR_EL1);
-	return true;
-}
+	p->regval = read_sysreg(actlr_el1);

-static void reset_actlr(struct kvm_vcpu *vcpu, const struct 
sys_reg_desc *r)
-{
-	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
+	if (p->aarch32) {
+		if (r->Op2 & 2)
+			p->regval = upper_32_bit(p->regval);
+		else
+			p->regval = lower_32_bit(p->regval);
+	}
+
+	return true;
  }

  /*
@@ -40,13 +43,13 @@ static void reset_actlr(struct kvm_vcpu *vcpu, const 
struct sys_reg_desc *r)
   * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
   */
  static const struct sys_reg_desc genericv8_sys_regs[] = {
-	{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, reset_actlr, ACTLR_EL1 },
+	{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, },
  };

  static const struct sys_reg_desc genericv8_cp15_regs[] = {
  	/* ACTLR */
-	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
-	  access_actlr },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001), access_actlr },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011), access_actlr },
  };

  static struct kvm_sys_reg_target_table genericv8_target_table = {

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
@ 2020-05-28 12:51     ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-28 12:51 UTC (permalink / raw)
  To: James Morse; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

Hi James,

On 2020-05-26 17:18, James Morse wrote:
> ACTLR_EL1 is a 64bit register while the 32bit ACTLR is obviously 32bit.
> For 32bit software, the extra bits are accessible via ACTLR2... which
> KVM doesn't emulate.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> I'm not convinced this is endian safe, but it does match what
> kvm_inject_undef32() do.
> 
> The alternative would be to always read the 64bit value, and generate
> the 32bit offets like access_vm_reg() does.
> 
>  arch/arm64/include/asm/kvm_host.h    |  1 +
>  arch/arm64/kvm/sys_regs_generic_v8.c | 16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 32c8a675e5a4..5b7538663a8e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -185,6 +185,7 @@ enum vcpu_sysreg {
>  #define c0_CSSELR	(CSSELR_EL1 * 2)/* Cache Size Selection Register */
>  #define c1_SCTLR	(SCTLR_EL1 * 2)	/* System Control Register */
>  #define c1_ACTLR	(ACTLR_EL1 * 2)	/* Auxiliary Control Register */
> +#define c1_ACTLR2	(c1_ACTLR + 1)	/* ACTLR top 32 bits */
>  #define c1_CPACR	(CPACR_EL1 * 2)	/* Coprocessor Access Control */
>  #define c2_TTBR0	(TTBR0_EL1 * 2)	/* Translation Table Base Register 0 
> */
>  #define c2_TTBR0_high	(c2_TTBR0 + 1)	/* TTBR0 top 32 bits */
> diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c
> b/arch/arm64/kvm/sys_regs_generic_v8.c
> index 9cb6b4c8355a..ed77bbb48e64 100644
> --- a/arch/arm64/kvm/sys_regs_generic_v8.c
> +++ b/arch/arm64/kvm/sys_regs_generic_v8.c
> @@ -30,6 +30,18 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
>  	return true;
>  }
> 
> +static bool access_cp15_actlr(struct kvm_vcpu *vcpu,
> +			      struct sys_reg_params *p,
> +			      const struct sys_reg_desc *r)
> +{
> +	if (p->is_write)
> +		return ignore_write(vcpu, p);
> +
> +	p->regval = vcpu_cp15(vcpu, r->reg);
> +	return true;
> +
> +}
> +
>  static void reset_actlr(struct kvm_vcpu *vcpu, const struct 
> sys_reg_desc *r)
>  {
>  	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
> @@ -46,7 +58,9 @@ static const struct sys_reg_desc genericv8_sys_regs[] 
> = {
>  static const struct sys_reg_desc genericv8_cp15_regs[] = {
>  	/* ACTLR */
>  	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
> -	  access_actlr },
> +	  access_cp15_actlr, NULL, c1_ACTLR },
> +	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011),
> +	  access_cp15_actlr, NULL, c1_ACTLR2 },
>  };
> 
>  static struct kvm_sys_reg_target_table genericv8_target_table = {

I'd get rid of any form of storage, and go with something like 
(untested, again):

diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c 
b/arch/arm64/kvm/sys_regs_generic_v8.c
index 9cb6b4c8355a..1b2bf2d37612 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -26,13 +26,16 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
  	if (p->is_write)
  		return ignore_write(vcpu, p);

-	p->regval = vcpu_read_sys_reg(vcpu, ACTLR_EL1);
-	return true;
-}
+	p->regval = read_sysreg(actlr_el1);

-static void reset_actlr(struct kvm_vcpu *vcpu, const struct 
sys_reg_desc *r)
-{
-	__vcpu_sys_reg(vcpu, ACTLR_EL1) = read_sysreg(actlr_el1);
+	if (p->aarch32) {
+		if (r->Op2 & 2)
+			p->regval = upper_32_bit(p->regval);
+		else
+			p->regval = lower_32_bit(p->regval);
+	}
+
+	return true;
  }

  /*
@@ -40,13 +43,13 @@ static void reset_actlr(struct kvm_vcpu *vcpu, const 
struct sys_reg_desc *r)
   * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
   */
  static const struct sys_reg_desc genericv8_sys_regs[] = {
-	{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, reset_actlr, ACTLR_EL1 },
+	{ SYS_DESC(SYS_ACTLR_EL1), access_actlr, },
  };

  static const struct sys_reg_desc genericv8_cp15_regs[] = {
  	/* ACTLR */
-	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001),
-	  access_actlr },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b001), access_actlr },
+	{ Op1(0b000), CRn(0b0001), CRm(0b0000), Op2(0b011), access_actlr },
  };

  static struct kvm_sys_reg_target_table genericv8_target_table = {

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
  2020-05-28 12:38       ` Marc Zyngier
@ 2020-05-28 12:55         ` James Morse
  -1 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-28 12:55 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvmarm, linux-arm-kernel

Hi Marc,

On 28/05/2020 13:38, Marc Zyngier wrote:
> On 2020-05-28 13:36, Marc Zyngier wrote:
>> On 2020-05-26 17:18, James Morse wrote:
>>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
>>> This means ACTLR* accesses from the guest are always trapped, and
>>> always return the value in the sys_regs array.
>>>
>>> The guest can't change the value of these registers, so we are
>>> save restoring the reset value, which came from the host.
>>>
>>> Stop save/restoring this register.
>>>
>>> This also stops this register being affected by sysregs_loaded_on_cpu,
>>> so we can provide 32 bit accessors that always use the in-memory copy.

>>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
>>> index 75b1925763f1..57116cf3a1a5 100644
>>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c

>>> @@ -133,7 +132,6 @@ static void __hyp_text
>>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>>          isb();
>>>      }
>>>
>>> -    write_sysreg(ctxt->sys_regs[ACTLR_EL1],        actlr_el1);
>>
>> If we don't need to save/restore it, we can also drop its presence
>> in the sys_regs array.

So even user-space accesses read from the hardware register? Fine by me.


>> It strikes me that we don't even have a trap handler for this sysreg,
>> whether it is 32 or 64bit... That's a bit unfortunate, to say the
>> least...
> 
> Ah, no. the sucker is hidden away in "generic_v8"...

That thing is A7/A15 (and then user-ABI) legacy right?

I was looking at ripping all that out when I ran over these. RFC grade, known not to bisect:
http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/kvm_kill_target_table/v0


Thanks,

James
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
@ 2020-05-28 12:55         ` James Morse
  0 siblings, 0 replies; 31+ messages in thread
From: James Morse @ 2020-05-28 12:55 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

Hi Marc,

On 28/05/2020 13:38, Marc Zyngier wrote:
> On 2020-05-28 13:36, Marc Zyngier wrote:
>> On 2020-05-26 17:18, James Morse wrote:
>>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS.
>>> This means ACTLR* accesses from the guest are always trapped, and
>>> always return the value in the sys_regs array.
>>>
>>> The guest can't change the value of these registers, so we are
>>> save restoring the reset value, which came from the host.
>>>
>>> Stop save/restoring this register.
>>>
>>> This also stops this register being affected by sysregs_loaded_on_cpu,
>>> so we can provide 32 bit accessors that always use the in-memory copy.

>>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
>>> index 75b1925763f1..57116cf3a1a5 100644
>>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
>>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c

>>> @@ -133,7 +132,6 @@ static void __hyp_text
>>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>>          isb();
>>>      }
>>>
>>> -    write_sysreg(ctxt->sys_regs[ACTLR_EL1],        actlr_el1);
>>
>> If we don't need to save/restore it, we can also drop its presence
>> in the sys_regs array.

So even user-space accesses read from the hardware register? Fine by me.


>> It strikes me that we don't even have a trap handler for this sysreg,
>> whether it is 32 or 64bit... That's a bit unfortunate, to say the
>> least...
> 
> Ah, no. the sucker is hidden away in "generic_v8"...

That thing is A7/A15 (and then user-ABI) legacy right?

I was looking at ripping all that out when I ran over these. RFC grade, known not to bisect:
http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/kvm_kill_target_table/v0


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
  2020-05-26 16:18 ` James Morse
@ 2020-05-31 13:37   ` Marc Zyngier
  -1 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-31 13:37 UTC (permalink / raw)
  To: James Morse; +Cc: kvmarm, linux-arm-kernel

Hi James,

On 2020-05-26 17:18, James Morse wrote:

[...]

> 1. How does this copro[] thing work with a big-endian host?
> The cp15_regs emulation look fine as nothing uses vcpu_cp15() to read 
> the
> register, but wouldn't prepare_fault32() read the wrong end of the 
> register
> when using vcpu_cp15()?

This seems pretty broken indeed. How about something like this:

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 
val, int reg);
   * CP14 and CP15 live in the same array, as they are backed by the
   * same system registers.
   */
-#define vcpu_cp14(v,r)		((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r)		((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN
+#define CPx_OFFSET	1
+#else
+#define CPx_OFFSET	0
+#endif
+
+#define vcpu_cp14(v,r)		((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
+#define vcpu_cp15(v,r)		((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])

  struct kvm_vm_stat {
  	ulong remote_tlb_flush;

Yes, it's ugly.

> 2. How does the 32bit fault injection code work with VHE?
> vcpu_cp15() modifies the in-memory copy, surely a vcpu_put() will 
> clobber
> everything it did, or fail to restore it when entering the guest.

Wow, you're really wadding into dangerous territory! ;-)

Again, you are absolutely right. I guess nobody really ever ran
32bit guests on VHE systems, as they are both rare and mostly
with 64-bit-only EL1. This code is also mostly never used
(we run well behaved guests at all times!).

Here's a hack that should do the right thing at all times:

diff --git a/arch/arm64/kvm/aarch32.c b/arch/arm64/kvm/aarch32.c
index 0a356aa91aa1..40a62a99fbf8 100644
--- a/arch/arm64/kvm/aarch32.c
+++ b/arch/arm64/kvm/aarch32.c
@@ -33,6 +33,26 @@ static const u8 return_offsets[8][2] = {
  	[7] = { 4, 4 },		/* FIQ, unused */
  };

+static bool pre_fault_synchronize(struct kvm_vcpu *vcpu)
+{
+	preempt_disable();
+	if (vcpu->arch.sysregs_loaded_on_cpu) {
+		kvm_arch_vcpu_put(vcpu);
+		return true;
+	}
+
+	preempt_enable();
+	return false;
+}
+
+static void post_fault_synchronize(struct kvm_vcpu *vcpu, bool loaded)
+{
+	if (loaded) {
+		kvm_arch_vcpu_load(vcpu, smp_processor_id());
+		preempt_enable();
+	}
+}
+
  /*
   * When an exception is taken, most CPSR fields are left unchanged in 
the
   * handler. However, some are explicitly overridden (e.g. M[4:0]).
@@ -155,7 +175,10 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, 
u32 mode, u32 vect_offset)

  void kvm_inject_undef32(struct kvm_vcpu *vcpu)
  {
+	bool loaded = pre_fault_synchronize(vcpu);
+
  	prepare_fault32(vcpu, PSR_AA32_MODE_UND, 4);
+	post_fault_synchronize(vcpu, loaded);
  }

  /*
@@ -168,6 +191,9 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool 
is_pabt,
  	u32 vect_offset;
  	u32 *far, *fsr;
  	bool is_lpae;
+	bool loaded;
+
+	loaded = pre_fault_synchronize(vcpu);

  	if (is_pabt) {
  		vect_offset = 12;
@@ -191,6 +217,8 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool 
is_pabt,
  		/* no need to shuffle FS[4] into DFSR[10] as its 0 */
  		*fsr = DFSR_FSC_EXTABT_nLPAE;
  	}
+
+	post_fault_synchronize(vcpu, loaded);
  }

  void kvm_inject_dabt32(struct kvm_vcpu *vcpu, unsigned long addr)

Of course, none of this is tested. We really should find ways to
trigger these corner cases... :-/

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
@ 2020-05-31 13:37   ` Marc Zyngier
  0 siblings, 0 replies; 31+ messages in thread
From: Marc Zyngier @ 2020-05-31 13:37 UTC (permalink / raw)
  To: James Morse; +Cc: Julien Thierry, kvmarm, linux-arm-kernel, Suzuki K Poulose

Hi James,

On 2020-05-26 17:18, James Morse wrote:

[...]

> 1. How does this copro[] thing work with a big-endian host?
> The cp15_regs emulation look fine as nothing uses vcpu_cp15() to read 
> the
> register, but wouldn't prepare_fault32() read the wrong end of the 
> register
> when using vcpu_cp15()?

This seems pretty broken indeed. How about something like this:

diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..e80c0e06f235 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -404,8 +404,14 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 
val, int reg);
   * CP14 and CP15 live in the same array, as they are backed by the
   * same system registers.
   */
-#define vcpu_cp14(v,r)		((v)->arch.ctxt.copro[(r)])
-#define vcpu_cp15(v,r)		((v)->arch.ctxt.copro[(r)])
+#ifdef CPU_BIG_ENDIAN
+#define CPx_OFFSET	1
+#else
+#define CPx_OFFSET	0
+#endif
+
+#define vcpu_cp14(v,r)		((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])
+#define vcpu_cp15(v,r)		((v)->arch.ctxt.copro[(r) ^ CPx_OFFSET])

  struct kvm_vm_stat {
  	ulong remote_tlb_flush;

Yes, it's ugly.

> 2. How does the 32bit fault injection code work with VHE?
> vcpu_cp15() modifies the in-memory copy, surely a vcpu_put() will 
> clobber
> everything it did, or fail to restore it when entering the guest.

Wow, you're really wadding into dangerous territory! ;-)

Again, you are absolutely right. I guess nobody really ever ran
32bit guests on VHE systems, as they are both rare and mostly
with 64-bit-only EL1. This code is also mostly never used
(we run well behaved guests at all times!).

Here's a hack that should do the right thing at all times:

diff --git a/arch/arm64/kvm/aarch32.c b/arch/arm64/kvm/aarch32.c
index 0a356aa91aa1..40a62a99fbf8 100644
--- a/arch/arm64/kvm/aarch32.c
+++ b/arch/arm64/kvm/aarch32.c
@@ -33,6 +33,26 @@ static const u8 return_offsets[8][2] = {
  	[7] = { 4, 4 },		/* FIQ, unused */
  };

+static bool pre_fault_synchronize(struct kvm_vcpu *vcpu)
+{
+	preempt_disable();
+	if (vcpu->arch.sysregs_loaded_on_cpu) {
+		kvm_arch_vcpu_put(vcpu);
+		return true;
+	}
+
+	preempt_enable();
+	return false;
+}
+
+static void post_fault_synchronize(struct kvm_vcpu *vcpu, bool loaded)
+{
+	if (loaded) {
+		kvm_arch_vcpu_load(vcpu, smp_processor_id());
+		preempt_enable();
+	}
+}
+
  /*
   * When an exception is taken, most CPSR fields are left unchanged in 
the
   * handler. However, some are explicitly overridden (e.g. M[4:0]).
@@ -155,7 +175,10 @@ static void prepare_fault32(struct kvm_vcpu *vcpu, 
u32 mode, u32 vect_offset)

  void kvm_inject_undef32(struct kvm_vcpu *vcpu)
  {
+	bool loaded = pre_fault_synchronize(vcpu);
+
  	prepare_fault32(vcpu, PSR_AA32_MODE_UND, 4);
+	post_fault_synchronize(vcpu, loaded);
  }

  /*
@@ -168,6 +191,9 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool 
is_pabt,
  	u32 vect_offset;
  	u32 *far, *fsr;
  	bool is_lpae;
+	bool loaded;
+
+	loaded = pre_fault_synchronize(vcpu);

  	if (is_pabt) {
  		vect_offset = 12;
@@ -191,6 +217,8 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool 
is_pabt,
  		/* no need to shuffle FS[4] into DFSR[10] as its 0 */
  		*fsr = DFSR_FSC_EXTABT_nLPAE;
  	}
+
+	post_fault_synchronize(vcpu, loaded);
  }

  void kvm_inject_dabt32(struct kvm_vcpu *vcpu, unsigned long addr)

Of course, none of this is tested. We really should find ways to
trigger these corner cases... :-/

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-05-31 13:37 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 16:18 [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses James Morse
2020-05-26 16:18 ` James Morse
2020-05-26 16:18 ` [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR James Morse
2020-05-26 16:18   ` James Morse
2020-05-26 16:18   ` James Morse
2020-05-27 16:57   ` Sasha Levin
2020-05-27 16:57     ` Sasha Levin
2020-05-27 16:57     ` Sasha Levin
2020-05-28  8:57   ` Marc Zyngier
2020-05-28  8:57     ` Marc Zyngier
2020-05-28  8:57     ` Marc Zyngier
2020-05-28 11:59     ` James Morse
2020-05-28 11:59       ` James Morse
2020-05-28 11:59       ` James Morse
2020-05-28 12:10       ` Marc Zyngier
2020-05-28 12:10         ` Marc Zyngier
2020-05-28 12:10         ` Marc Zyngier
2020-05-26 16:18 ` [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1 James Morse
2020-05-26 16:18   ` James Morse
2020-05-28 12:36   ` Marc Zyngier
2020-05-28 12:36     ` Marc Zyngier
2020-05-28 12:38     ` Marc Zyngier
2020-05-28 12:38       ` Marc Zyngier
2020-05-28 12:55       ` James Morse
2020-05-28 12:55         ` James Morse
2020-05-26 16:18 ` [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2 James Morse
2020-05-26 16:18   ` James Morse
2020-05-28 12:51   ` Marc Zyngier
2020-05-28 12:51     ` Marc Zyngier
2020-05-31 13:37 ` [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses Marc Zyngier
2020-05-31 13:37   ` Marc Zyngier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.