* [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
@ 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
` (3 more replies)
0 siblings, 4 replies; 13+ 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] 13+ messages in thread
* [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
2020-05-26 16:18 [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses James Morse
@ 2020-05-26 16:18 ` James Morse
2020-05-27 16:57 ` Sasha Levin
2020-05-28 8:57 ` Marc Zyngier
2020-05-26 16:18 ` [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1 James Morse
` (2 subsequent siblings)
3 siblings, 2 replies; 13+ 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] 13+ messages in thread
* [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
2020-05-26 16:18 [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses 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-28 12:36 ` Marc Zyngier
2020-05-26 16:18 ` [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2 James Morse
2020-05-31 13:37 ` [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses Marc Zyngier
3 siblings, 1 reply; 13+ 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] 13+ messages in thread
* [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
2020-05-26 16:18 [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses 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 ` [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1 James Morse
@ 2020-05-26 16:18 ` James Morse
2020-05-28 12:51 ` Marc Zyngier
2020-05-31 13:37 ` [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses Marc Zyngier
3 siblings, 1 reply; 13+ 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] 13+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
2020-05-26 16:18 ` [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR James Morse
@ 2020-05-27 16:57 ` Sasha Levin
2020-05-28 8:57 ` Marc Zyngier
1 sibling, 0 replies; 13+ 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] 13+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR
2020-05-26 16:18 ` [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR James Morse
2020-05-27 16:57 ` Sasha Levin
@ 2020-05-28 8:57 ` Marc Zyngier
2020-05-28 11:59 ` James Morse
1 sibling, 1 reply; 13+ 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] 13+ 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
2020-05-28 12:10 ` Marc Zyngier
0 siblings, 1 reply; 13+ 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] 13+ 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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1
2020-05-26 16:18 ` [PATCH 2/3] KVM: arm64: Stop save/restoring ACTLR_EL1 James Morse
@ 2020-05-28 12:36 ` Marc Zyngier
2020-05-28 12:38 ` Marc Zyngier
0 siblings, 1 reply; 13+ 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] 13+ 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
2020-05-28 12:55 ` James Morse
0 siblings, 1 reply; 13+ 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] 13+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2
2020-05-26 16:18 ` [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2 James Morse
@ 2020-05-28 12:51 ` Marc Zyngier
0 siblings, 0 replies; 13+ 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] 13+ 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
0 siblings, 0 replies; 13+ 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] 13+ messages in thread
* Re: [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses
2020-05-26 16:18 [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses James Morse
` (2 preceding siblings ...)
2020-05-26 16:18 ` [PATCH 3/3] KVM: arm64: Add emulation for 32bit guests accessing ACTLR2 James Morse
@ 2020-05-31 13:37 ` Marc Zyngier
3 siblings, 0 replies; 13+ 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] 13+ messages in thread
end of thread, other threads:[~2020-05-31 13:37 UTC | newest]
Thread overview: 13+ 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 ` [PATCH 1/3] KVM: arm64: Stop writing aarch32's CSSELR into ACTLR James Morse
2020-05-27 16:57 ` Sasha Levin
2020-05-28 8:57 ` Marc Zyngier
2020-05-28 11:59 ` James Morse
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-28 12:36 ` Marc Zyngier
2020-05-28 12:38 ` Marc Zyngier
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-28 12:51 ` Marc Zyngier
2020-05-31 13:37 ` [PATCH 0/3] KVM: arm64: aarch32 ACTLR accesses Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).