[PATCH v2 0/6] KVM: arm64: Emulate the OS lock

[PATCH v2 0/6] KVM: arm64: Emulate the OS lock

[Oliver Upton]

KVM does not implement the debug architecture to the letter of the
specification. One such issue is the fact that KVM treats the OS Lock as
RAZ/WI, rather than emulating its behavior on hardware. This series adds
emulation support for the OS Lock to KVM. Emulation is warranted as the
OS Lock affects debug exceptions taken from all ELs, and is not limited
to only the context of the guest.

The 1st patch is a correctness fix for the OSLSR register, ensuring
the trap handler actually is written to suggest WO behavior. Note that
the changed code should never be reached on a correct implementation, as
hardware should generate the undef, not KVM.

The 2nd patch adds the necessary context to track guest values of the
OS Lock bit and exposes the value to userspace for the sake of
migration.

The 3rd patch makes the OSLK bit writable in OSLAR_EL1 (from the guest)
and OSLSR_EL1 (from userspace), but does nothing with its value.

The 4th patch actually implements the OS Lock behavior, disabling all
debug exceptions from the perspective of the guest. This is done by
disabling MDE and SS in MDSCR_EL1. Since software breakpoint
instructions cannot be masked by anything but the OS Lock, we emulate by
trapping debug exceptions to EL2 and skipping the breakpoint. Skip this
whole song and dance altogether if userspace is debugging the guest.

The 5th patch asserts that OSLSR_EL1 is exposed by KVM to userspace
through the KVM_GET_REG_LIST ioctl. Lastly, the 6th patch asserts that
no debug exceptions are routed to the guest when the OSLK bit is set.

This series applies cleanly to 5.15. Tested on an Ampere Altra machine
with the included selftests patches. Additionally, I single-stepped a
guest using kvmtool to make sure userspace debugging is still working
correctly.

[v1]: http://lore.kernel.org/r/20211029003202.158161-1-oupton@google.com

v1 -> v2:
 - Added OSLSR_EL1 to get-reg-list test
 - Added test cases to debug-exceptions test
 - Scrapped the context switching of OSLSR_EL1
 - Dropped DFR0 changes, to be addressed in a later series

Oliver Upton (6):
  KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined
  KVM: arm64: Stash OSLSR_EL1 in the cpu context
  KVM: arm64: Allow guest to set the OSLK bit
  KVM: arm64: Emulate the OS Lock
  selftests: KVM: Add OSLSR_EL1 to the list of blessed regs
  selftests: KVM: Test OS lock behavior

 arch/arm64/include/asm/kvm_host.h             |  5 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 20 ++++--
 arch/arm64/kvm/handle_exit.c                  |  8 +++
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 7 files changed, 144 insertions(+), 24 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 0/6] KVM: arm64: Emulate the OS lock

[Oliver Upton]

KVM does not implement the debug architecture to the letter of the
specification. One such issue is the fact that KVM treats the OS Lock as
RAZ/WI, rather than emulating its behavior on hardware. This series adds
emulation support for the OS Lock to KVM. Emulation is warranted as the
OS Lock affects debug exceptions taken from all ELs, and is not limited
to only the context of the guest.

The 1st patch is a correctness fix for the OSLSR register, ensuring
the trap handler actually is written to suggest WO behavior. Note that
the changed code should never be reached on a correct implementation, as
hardware should generate the undef, not KVM.

The 2nd patch adds the necessary context to track guest values of the
OS Lock bit and exposes the value to userspace for the sake of
migration.

The 3rd patch makes the OSLK bit writable in OSLAR_EL1 (from the guest)
and OSLSR_EL1 (from userspace), but does nothing with its value.

The 4th patch actually implements the OS Lock behavior, disabling all
debug exceptions from the perspective of the guest. This is done by
disabling MDE and SS in MDSCR_EL1. Since software breakpoint
instructions cannot be masked by anything but the OS Lock, we emulate by
trapping debug exceptions to EL2 and skipping the breakpoint. Skip this
whole song and dance altogether if userspace is debugging the guest.

The 5th patch asserts that OSLSR_EL1 is exposed by KVM to userspace
through the KVM_GET_REG_LIST ioctl. Lastly, the 6th patch asserts that
no debug exceptions are routed to the guest when the OSLK bit is set.

This series applies cleanly to 5.15. Tested on an Ampere Altra machine
with the included selftests patches. Additionally, I single-stepped a
guest using kvmtool to make sure userspace debugging is still working
correctly.

[v1]: http://lore.kernel.org/r/20211029003202.158161-1-oupton@google.com

v1 -> v2:
 - Added OSLSR_EL1 to get-reg-list test
 - Added test cases to debug-exceptions test
 - Scrapped the context switching of OSLSR_EL1
 - Dropped DFR0 changes, to be addressed in a later series

Oliver Upton (6):
  KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined
  KVM: arm64: Stash OSLSR_EL1 in the cpu context
  KVM: arm64: Allow guest to set the OSLK bit
  KVM: arm64: Emulate the OS Lock
  selftests: KVM: Add OSLSR_EL1 to the list of blessed regs
  selftests: KVM: Test OS lock behavior

 arch/arm64/include/asm/kvm_host.h             |  5 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 20 ++++--
 arch/arm64/kvm/handle_exit.c                  |  8 +++
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 7 files changed, 144 insertions(+), 24 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 0/6] KVM: arm64: Emulate the OS lock

[Oliver Upton]

KVM does not implement the debug architecture to the letter of the
specification. One such issue is the fact that KVM treats the OS Lock as
RAZ/WI, rather than emulating its behavior on hardware. This series adds
emulation support for the OS Lock to KVM. Emulation is warranted as the
OS Lock affects debug exceptions taken from all ELs, and is not limited
to only the context of the guest.

The 1st patch is a correctness fix for the OSLSR register, ensuring
the trap handler actually is written to suggest WO behavior. Note that
the changed code should never be reached on a correct implementation, as
hardware should generate the undef, not KVM.

The 2nd patch adds the necessary context to track guest values of the
OS Lock bit and exposes the value to userspace for the sake of
migration.

The 3rd patch makes the OSLK bit writable in OSLAR_EL1 (from the guest)
and OSLSR_EL1 (from userspace), but does nothing with its value.

The 4th patch actually implements the OS Lock behavior, disabling all
debug exceptions from the perspective of the guest. This is done by
disabling MDE and SS in MDSCR_EL1. Since software breakpoint
instructions cannot be masked by anything but the OS Lock, we emulate by
trapping debug exceptions to EL2 and skipping the breakpoint. Skip this
whole song and dance altogether if userspace is debugging the guest.

The 5th patch asserts that OSLSR_EL1 is exposed by KVM to userspace
through the KVM_GET_REG_LIST ioctl. Lastly, the 6th patch asserts that
no debug exceptions are routed to the guest when the OSLK bit is set.

This series applies cleanly to 5.15. Tested on an Ampere Altra machine
with the included selftests patches. Additionally, I single-stepped a
guest using kvmtool to make sure userspace debugging is still working
correctly.

[v1]: http://lore.kernel.org/r/20211029003202.158161-1-oupton@google.com

v1 -> v2:
 - Added OSLSR_EL1 to get-reg-list test
 - Added test cases to debug-exceptions test
 - Scrapped the context switching of OSLSR_EL1
 - Dropped DFR0 changes, to be addressed in a later series

Oliver Upton (6):
  KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined
  KVM: arm64: Stash OSLSR_EL1 in the cpu context
  KVM: arm64: Allow guest to set the OSLK bit
  KVM: arm64: Emulate the OS Lock
  selftests: KVM: Add OSLSR_EL1 to the list of blessed regs
  selftests: KVM: Test OS lock behavior

 arch/arm64/include/asm/kvm_host.h             |  5 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 20 ++++--
 arch/arm64/kvm/handle_exit.c                  |  8 +++
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 7 files changed, 144 insertions(+), 24 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Oliver Upton]

Any valid implementation of the architecture should generate an
undefined exception for writes to a read-only register, such as
OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
behavior.

Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
write ever traps to EL2, inject an undef into the guest and print a
warning.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..17fa6ddf5405 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -292,7 +292,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		return ignore_write(vcpu, p);
+		return write_to_read_only(vcpu, p, r);
 	} else {
 		p->regval = (1 << 3);
 		return true;
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Oliver Upton]

Any valid implementation of the architecture should generate an
undefined exception for writes to a read-only register, such as
OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
behavior.

Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
write ever traps to EL2, inject an undef into the guest and print a
warning.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..17fa6ddf5405 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -292,7 +292,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		return ignore_write(vcpu, p);
+		return write_to_read_only(vcpu, p, r);
 	} else {
 		p->regval = (1 << 3);
 		return true;
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Oliver Upton]

Any valid implementation of the architecture should generate an
undefined exception for writes to a read-only register, such as
OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
behavior.

Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
write ever traps to EL2, inject an undef into the guest and print a
warning.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..17fa6ddf5405 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -292,7 +292,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		return ignore_write(vcpu, p);
+		return write_to_read_only(vcpu, p, r);
 	} else {
 		p->regval = (1 << 3);
 		return true;
-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

An upcoming change to KVM will context switch the OS Lock status between
guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
using the stored value.

Wire up a custom handler for writes from userspace and prevent any of
the invariant bits from changing.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8be56d5342b..c98f65c4a1f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,6 +172,7 @@ enum vcpu_sysreg {
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
+	OSLSR_EL1,	/* OS Lock Status Register */
 
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 17fa6ddf5405..0326b3df0736 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -291,12 +291,28 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	if (p->is_write) {
+	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
-	} else {
-		p->regval = (1 << 3);
-		return true;
-	}
+
+	p->regval = __vcpu_sys_reg(vcpu, r->reg);
+	return true;
+}
+
+static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			 const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	u64 id = sys_reg_to_index(rd);
+	u64 val;
+	int err;
+
+	err = reg_from_user(&val, uaddr, id);
+	if (err)
+		return err;
+
+	if (val != rd->val)
+		return -EINVAL;
+
+	return 0;
 }
 
 static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
@@ -1441,7 +1457,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
@@ -1916,7 +1933,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
 	DBGBXVR(1),
 	/* DBGOSLSR */
-	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1 },
+	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
 	DBGBXVR(2),
 	DBGBXVR(3),
 	/* DBGOSDLR */
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

An upcoming change to KVM will context switch the OS Lock status between
guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
using the stored value.

Wire up a custom handler for writes from userspace and prevent any of
the invariant bits from changing.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8be56d5342b..c98f65c4a1f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,6 +172,7 @@ enum vcpu_sysreg {
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
+	OSLSR_EL1,	/* OS Lock Status Register */
 
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 17fa6ddf5405..0326b3df0736 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -291,12 +291,28 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	if (p->is_write) {
+	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
-	} else {
-		p->regval = (1 << 3);
-		return true;
-	}
+
+	p->regval = __vcpu_sys_reg(vcpu, r->reg);
+	return true;
+}
+
+static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			 const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	u64 id = sys_reg_to_index(rd);
+	u64 val;
+	int err;
+
+	err = reg_from_user(&val, uaddr, id);
+	if (err)
+		return err;
+
+	if (val != rd->val)
+		return -EINVAL;
+
+	return 0;
 }
 
 static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
@@ -1441,7 +1457,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
@@ -1916,7 +1933,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
 	DBGBXVR(1),
 	/* DBGOSLSR */
-	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1 },
+	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
 	DBGBXVR(2),
 	DBGBXVR(3),
 	/* DBGOSDLR */
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

An upcoming change to KVM will context switch the OS Lock status between
guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
using the stored value.

Wire up a custom handler for writes from userspace and prevent any of
the invariant bits from changing.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f8be56d5342b..c98f65c4a1f7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,6 +172,7 @@ enum vcpu_sysreg {
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
+	OSLSR_EL1,	/* OS Lock Status Register */
 
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 17fa6ddf5405..0326b3df0736 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -291,12 +291,28 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
 {
-	if (p->is_write) {
+	if (p->is_write)
 		return write_to_read_only(vcpu, p, r);
-	} else {
-		p->regval = (1 << 3);
-		return true;
-	}
+
+	p->regval = __vcpu_sys_reg(vcpu, r->reg);
+	return true;
+}
+
+static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
+			 const struct kvm_one_reg *reg, void __user *uaddr)
+{
+	u64 id = sys_reg_to_index(rd);
+	u64 val;
+	int err;
+
+	err = reg_from_user(&val, uaddr, id);
+	if (err)
+		return err;
+
+	if (val != rd->val)
+		return -EINVAL;
+
+	return 0;
 }
 
 static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
@@ -1441,7 +1457,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGCLAIMSET_EL1), trap_raz_wi },
@@ -1916,7 +1933,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
 	DBGBXVR(1),
 	/* DBGOSLSR */
-	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1 },
+	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
 	DBGBXVR(2),
 	DBGBXVR(3),
 	/* DBGOSDLR */
-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
reset value of the OSLK bit to 1. Allow the value to be migrated by
making OSLSR_EL1.OSLK writable from userspace.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/sysreg.h |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b268082d67ed..6ba4dc97b69d 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -127,7 +127,13 @@
 #define SYS_DBGWCRn_EL1(n)		sys_reg(2, 0, 0, n, 7)
 #define SYS_MDRAR_EL1			sys_reg(2, 0, 1, 0, 0)
 #define SYS_OSLAR_EL1			sys_reg(2, 0, 1, 0, 4)
+
+#define SYS_OSLAR_OSLK			BIT(0)
+
 #define SYS_OSLSR_EL1			sys_reg(2, 0, 1, 1, 4)
+
+#define SYS_OSLSR_OSLK			BIT(1)
+
 #define SYS_OSDLR_EL1			sys_reg(2, 0, 1, 3, 4)
 #define SYS_DBGPRCR_EL1			sys_reg(2, 0, 1, 4, 4)
 #define SYS_DBGCLAIMSET_EL1		sys_reg(2, 0, 7, 8, 6)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0326b3df0736..acd8aa2e5a44 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -44,6 +44,10 @@
  * 64bit interface.
  */
 
+static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
+static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
+static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
+
 static bool read_from_write_only(struct kvm_vcpu *vcpu,
 				 struct sys_reg_params *params,
 				 const struct sys_reg_desc *r)
@@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 	return trap_raz_wi(vcpu, p, r);
 }
 
+static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 oslsr;
+
+	if (!p->is_write)
+		return read_from_write_only(vcpu, p, r);
+
+	/* Forward the OSLK bit to OSLSR */
+	oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
+	if (p->regval & SYS_OSLAR_OSLK)
+		oslsr |= SYS_OSLSR_OSLK;
+
+	__vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
+	return true;
+}
+
 static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
@@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	if (err)
 		return err;
 
-	if (val != rd->val)
+	if ((val | SYS_OSLSR_OSLK) != rd->val)
 		return -EINVAL;
 
+	__vcpu_sys_reg(vcpu, rd->reg) = val;
 	return 0;
 }
 
@@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
 	return __access_id_reg(vcpu, p, r, true);
 }
 
-static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
-static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
-static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
-
 /* Visibility overrides for SVE-specific control registers */
 static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *rd)
@@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	DBG_BCR_BVR_WCR_WVR_EL1(15),
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
 		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
@@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 
 	DBGBXVR(0),
 	/* DBGOSLAR */
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
+	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
 	DBGBXVR(1),
 	/* DBGOSLSR */
 	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
reset value of the OSLK bit to 1. Allow the value to be migrated by
making OSLSR_EL1.OSLK writable from userspace.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/sysreg.h |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b268082d67ed..6ba4dc97b69d 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -127,7 +127,13 @@
 #define SYS_DBGWCRn_EL1(n)		sys_reg(2, 0, 0, n, 7)
 #define SYS_MDRAR_EL1			sys_reg(2, 0, 1, 0, 0)
 #define SYS_OSLAR_EL1			sys_reg(2, 0, 1, 0, 4)
+
+#define SYS_OSLAR_OSLK			BIT(0)
+
 #define SYS_OSLSR_EL1			sys_reg(2, 0, 1, 1, 4)
+
+#define SYS_OSLSR_OSLK			BIT(1)
+
 #define SYS_OSDLR_EL1			sys_reg(2, 0, 1, 3, 4)
 #define SYS_DBGPRCR_EL1			sys_reg(2, 0, 1, 4, 4)
 #define SYS_DBGCLAIMSET_EL1		sys_reg(2, 0, 7, 8, 6)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0326b3df0736..acd8aa2e5a44 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -44,6 +44,10 @@
  * 64bit interface.
  */
 
+static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
+static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
+static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
+
 static bool read_from_write_only(struct kvm_vcpu *vcpu,
 				 struct sys_reg_params *params,
 				 const struct sys_reg_desc *r)
@@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 	return trap_raz_wi(vcpu, p, r);
 }
 
+static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 oslsr;
+
+	if (!p->is_write)
+		return read_from_write_only(vcpu, p, r);
+
+	/* Forward the OSLK bit to OSLSR */
+	oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
+	if (p->regval & SYS_OSLAR_OSLK)
+		oslsr |= SYS_OSLSR_OSLK;
+
+	__vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
+	return true;
+}
+
 static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
@@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	if (err)
 		return err;
 
-	if (val != rd->val)
+	if ((val | SYS_OSLSR_OSLK) != rd->val)
 		return -EINVAL;
 
+	__vcpu_sys_reg(vcpu, rd->reg) = val;
 	return 0;
 }
 
@@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
 	return __access_id_reg(vcpu, p, r, true);
 }
 
-static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
-static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
-static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
-
 /* Visibility overrides for SVE-specific control registers */
 static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *rd)
@@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	DBG_BCR_BVR_WCR_WVR_EL1(15),
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
 		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
@@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 
 	DBGBXVR(0),
 	/* DBGOSLAR */
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
+	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
 	DBGBXVR(1),
 	/* DBGOSLSR */
 	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
reset value of the OSLK bit to 1. Allow the value to be migrated by
making OSLSR_EL1.OSLK writable from userspace.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/sysreg.h |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b268082d67ed..6ba4dc97b69d 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -127,7 +127,13 @@
 #define SYS_DBGWCRn_EL1(n)		sys_reg(2, 0, 0, n, 7)
 #define SYS_MDRAR_EL1			sys_reg(2, 0, 1, 0, 0)
 #define SYS_OSLAR_EL1			sys_reg(2, 0, 1, 0, 4)
+
+#define SYS_OSLAR_OSLK			BIT(0)
+
 #define SYS_OSLSR_EL1			sys_reg(2, 0, 1, 1, 4)
+
+#define SYS_OSLSR_OSLK			BIT(1)
+
 #define SYS_OSDLR_EL1			sys_reg(2, 0, 1, 3, 4)
 #define SYS_DBGPRCR_EL1			sys_reg(2, 0, 1, 4, 4)
 #define SYS_DBGCLAIMSET_EL1		sys_reg(2, 0, 7, 8, 6)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0326b3df0736..acd8aa2e5a44 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -44,6 +44,10 @@
  * 64bit interface.
  */
 
+static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
+static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
+static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
+
 static bool read_from_write_only(struct kvm_vcpu *vcpu,
 				 struct sys_reg_params *params,
 				 const struct sys_reg_desc *r)
@@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 	return trap_raz_wi(vcpu, p, r);
 }
 
+static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
+			   struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 oslsr;
+
+	if (!p->is_write)
+		return read_from_write_only(vcpu, p, r);
+
+	/* Forward the OSLK bit to OSLSR */
+	oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
+	if (p->regval & SYS_OSLAR_OSLK)
+		oslsr |= SYS_OSLSR_OSLK;
+
+	__vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
+	return true;
+}
+
 static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
 			   struct sys_reg_params *p,
 			   const struct sys_reg_desc *r)
@@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
 	if (err)
 		return err;
 
-	if (val != rd->val)
+	if ((val | SYS_OSLSR_OSLK) != rd->val)
 		return -EINVAL;
 
+	__vcpu_sys_reg(vcpu, rd->reg) = val;
 	return 0;
 }
 
@@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
 	return __access_id_reg(vcpu, p, r, true);
 }
 
-static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
-static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
-static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
-
 /* Visibility overrides for SVE-specific control registers */
 static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *rd)
@@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	DBG_BCR_BVR_WCR_WVR_EL1(15),
 
 	{ SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
-	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
+	{ SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
+	{ SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
 		.set_user = set_oslsr_el1, },
 	{ SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
@@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
 
 	DBGBXVR(0),
 	/* DBGOSLAR */
-	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
+	{ Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
 	DBGBXVR(1),
 	/* DBGOSLSR */
 	{ Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

The OS lock blocks all debug exceptions at every EL. To date, KVM has
not implemented the OS lock for its guests, despite the fact that it is
mandatory per the architecture. Simple context switching between the
guest and host is not appropriate, as its effects are not constrained to
the guest context.

Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
blocking all but software breakpoint instructions. To handle breakpoint
instructions, trap debug exceptions to EL2 and skip the instruction.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  4 ++++
 arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
 arch/arm64/kvm/handle_exit.c      |  8 ++++++++
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c98f65c4a1f7..f13b8b79b06d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+
+#define kvm_vcpu_os_lock_enabled(vcpu)		\
+	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
+
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index db9361338b2a..5690a9c99c89 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 				MDCR_EL2_TDRA |
 				MDCR_EL2_TDOSA);
 
-	/* Is the VM being debugged by userspace? */
-	if (vcpu->guest_debug)
+	/*
+	 * Check if the VM is being debugged by userspace or the guest has
+	 * enabled the OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
 		/* Route all software debug exceptions to EL2 */
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
 
@@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 
 	kvm_arm_setup_mdcr_el2(vcpu);
 
-	/* Is Guest debugging in effect? */
-	if (vcpu->guest_debug) {
+	/*
+	 * Check if the guest is being debugged or if the guest has enabled the
+	 * OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
 						&vcpu->arch.debug_ptr->dbg_wcr[0],
 						&vcpu->arch.debug_ptr->dbg_wvr[0]);
+		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
+			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
+			mdscr &= ~DBG_MDSCR_MDE;
+			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 	}
 
@@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 275a27368a04..a7136888434d 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
 	u32 esr = kvm_vcpu_get_esr(vcpu);
+	u8 esr_ec = ESR_ELx_EC(esr);
+
+	if (!vcpu->guest_debug) {
+		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
+			  "Unexpected debug exception\n");
+		kvm_incr_pc(vcpu);
+		return 1;
+	}
 
 	run->exit_reason = KVM_EXIT_DEBUG;
 	run->debug.arch.hsr = esr;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index acd8aa2e5a44..d336e4c66870 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
  * Debug handling: We do trap most, if not all debug related system
  * registers. The implementation is good enough to ensure that a guest
  * can use these with minimal performance degradation. The drawback is
- * that we don't implement any of the external debug, none of the
- * OSlock protocol. This should be revisited if we ever encounter a
- * more demanding guest...
+ * that we don't implement any of the external debug architecture.
+ * This should be revisited if we ever encounter a more demanding
+ * guest...
  */
 static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

The OS lock blocks all debug exceptions at every EL. To date, KVM has
not implemented the OS lock for its guests, despite the fact that it is
mandatory per the architecture. Simple context switching between the
guest and host is not appropriate, as its effects are not constrained to
the guest context.

Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
blocking all but software breakpoint instructions. To handle breakpoint
instructions, trap debug exceptions to EL2 and skip the instruction.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  4 ++++
 arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
 arch/arm64/kvm/handle_exit.c      |  8 ++++++++
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c98f65c4a1f7..f13b8b79b06d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+
+#define kvm_vcpu_os_lock_enabled(vcpu)		\
+	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
+
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index db9361338b2a..5690a9c99c89 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 				MDCR_EL2_TDRA |
 				MDCR_EL2_TDOSA);
 
-	/* Is the VM being debugged by userspace? */
-	if (vcpu->guest_debug)
+	/*
+	 * Check if the VM is being debugged by userspace or the guest has
+	 * enabled the OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
 		/* Route all software debug exceptions to EL2 */
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
 
@@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 
 	kvm_arm_setup_mdcr_el2(vcpu);
 
-	/* Is Guest debugging in effect? */
-	if (vcpu->guest_debug) {
+	/*
+	 * Check if the guest is being debugged or if the guest has enabled the
+	 * OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
 						&vcpu->arch.debug_ptr->dbg_wcr[0],
 						&vcpu->arch.debug_ptr->dbg_wvr[0]);
+		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
+			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
+			mdscr &= ~DBG_MDSCR_MDE;
+			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 	}
 
@@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 275a27368a04..a7136888434d 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
 	u32 esr = kvm_vcpu_get_esr(vcpu);
+	u8 esr_ec = ESR_ELx_EC(esr);
+
+	if (!vcpu->guest_debug) {
+		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
+			  "Unexpected debug exception\n");
+		kvm_incr_pc(vcpu);
+		return 1;
+	}
 
 	run->exit_reason = KVM_EXIT_DEBUG;
 	run->debug.arch.hsr = esr;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index acd8aa2e5a44..d336e4c66870 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
  * Debug handling: We do trap most, if not all debug related system
  * registers. The implementation is good enough to ensure that a guest
  * can use these with minimal performance degradation. The drawback is
- * that we don't implement any of the external debug, none of the
- * OSlock protocol. This should be revisited if we ever encounter a
- * more demanding guest...
+ * that we don't implement any of the external debug architecture.
+ * This should be revisited if we ever encounter a more demanding
+ * guest...
  */
 static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

The OS lock blocks all debug exceptions at every EL. To date, KVM has
not implemented the OS lock for its guests, despite the fact that it is
mandatory per the architecture. Simple context switching between the
guest and host is not appropriate, as its effects are not constrained to
the guest context.

Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
blocking all but software breakpoint instructions. To handle breakpoint
instructions, trap debug exceptions to EL2 and skip the instruction.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  4 ++++
 arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
 arch/arm64/kvm/handle_exit.c      |  8 ++++++++
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 4 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index c98f65c4a1f7..f13b8b79b06d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
 void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
+
+#define kvm_vcpu_os_lock_enabled(vcpu)		\
+	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
+
 int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 			       struct kvm_device_attr *attr);
 int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index db9361338b2a..5690a9c99c89 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 				MDCR_EL2_TDRA |
 				MDCR_EL2_TDOSA);
 
-	/* Is the VM being debugged by userspace? */
-	if (vcpu->guest_debug)
+	/*
+	 * Check if the VM is being debugged by userspace or the guest has
+	 * enabled the OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
 		/* Route all software debug exceptions to EL2 */
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
 
@@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 
 	kvm_arm_setup_mdcr_el2(vcpu);
 
-	/* Is Guest debugging in effect? */
-	if (vcpu->guest_debug) {
+	/*
+	 * Check if the guest is being debugged or if the guest has enabled the
+	 * OS lock.
+	 */
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
 			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
 						&vcpu->arch.debug_ptr->dbg_wcr[0],
 						&vcpu->arch.debug_ptr->dbg_wvr[0]);
+		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
+			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
+			mdscr &= ~DBG_MDSCR_MDE;
+			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
 		}
 	}
 
@@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 275a27368a04..a7136888434d 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
 {
 	struct kvm_run *run = vcpu->run;
 	u32 esr = kvm_vcpu_get_esr(vcpu);
+	u8 esr_ec = ESR_ELx_EC(esr);
+
+	if (!vcpu->guest_debug) {
+		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
+			  "Unexpected debug exception\n");
+		kvm_incr_pc(vcpu);
+		return 1;
+	}
 
 	run->exit_reason = KVM_EXIT_DEBUG;
 	run->debug.arch.hsr = esr;
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index acd8aa2e5a44..d336e4c66870 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
  * Debug handling: We do trap most, if not all debug related system
  * registers. The implementation is good enough to ensure that a guest
  * can use these with minimal performance degradation. The drawback is
- * that we don't implement any of the external debug, none of the
- * OSlock protocol. This should be revisited if we ever encounter a
- * more demanding guest...
+ * that we don't implement any of the external debug architecture.
+ * This should be revisited if we ever encounter a more demanding
+ * guest...
  */
 static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 5/6] selftests: KVM: Add OSLSR_EL1 to the list of blessed regs

[Oliver Upton]

OSLSR_EL1 is now part of the visible system register state. Add it to
the get-reg-list selftest to ensure we keep it that way.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/aarch64/get-reg-list.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index cc898181faab..0c7c39a16b3f 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -761,6 +761,7 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(2, 0, 0, 15, 6),
 	ARM64_SYS_REG(2, 0, 0, 15, 7),
 	ARM64_SYS_REG(2, 4, 0, 7, 0),	/* DBGVCR32_EL2 */
+	ARM64_SYS_REG(2, 0, 1, 1, 4),	/* OSLSR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 0, 5),	/* MPIDR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 0),	/* ID_PFR0_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 1),	/* ID_PFR1_EL1 */
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 5/6] selftests: KVM: Add OSLSR_EL1 to the list of blessed regs

[Oliver Upton]

OSLSR_EL1 is now part of the visible system register state. Add it to
the get-reg-list selftest to ensure we keep it that way.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/aarch64/get-reg-list.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index cc898181faab..0c7c39a16b3f 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -761,6 +761,7 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(2, 0, 0, 15, 6),
 	ARM64_SYS_REG(2, 0, 0, 15, 7),
 	ARM64_SYS_REG(2, 4, 0, 7, 0),	/* DBGVCR32_EL2 */
+	ARM64_SYS_REG(2, 0, 1, 1, 4),	/* OSLSR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 0, 5),	/* MPIDR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 0),	/* ID_PFR0_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 1),	/* ID_PFR1_EL1 */
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 5/6] selftests: KVM: Add OSLSR_EL1 to the list of blessed regs

[Oliver Upton]

OSLSR_EL1 is now part of the visible system register state. Add it to
the get-reg-list selftest to ensure we keep it that way.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/aarch64/get-reg-list.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
index cc898181faab..0c7c39a16b3f 100644
--- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c
+++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c
@@ -761,6 +761,7 @@ static __u64 base_regs[] = {
 	ARM64_SYS_REG(2, 0, 0, 15, 6),
 	ARM64_SYS_REG(2, 0, 0, 15, 7),
 	ARM64_SYS_REG(2, 4, 0, 7, 0),	/* DBGVCR32_EL2 */
+	ARM64_SYS_REG(2, 0, 1, 1, 4),	/* OSLSR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 0, 5),	/* MPIDR_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 0),	/* ID_PFR0_EL1 */
 	ARM64_SYS_REG(3, 0, 0, 1, 1),	/* ID_PFR1_EL1 */
-- 
2.33.1.1089.g2158813163f-goog


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

[PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

KVM now correctly handles the OS Lock for its guests. When set, KVM
blocks all debug exceptions originating from the guest. Add test cases
to the debug-exceptions test to assert that software breakpoint,
hardware breakpoint, watchpoint, and single-step exceptions are in fact
blocked.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index e5e6c92b60da..6b6ff81cdd23 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -23,7 +23,7 @@
 #define SPSR_D		(1 << 9)
 #define SPSR_SS		(1 << 21)
 
-extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
+extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
 static volatile uint64_t sw_bp_addr, hw_bp_addr;
 static volatile uint64_t wp_addr, wp_data_addr;
 static volatile uint64_t svc_addr;
@@ -47,6 +47,14 @@ static void reset_debug_state(void)
 	isb();
 }
 
+static void enable_os_lock(void)
+{
+	write_sysreg(oslar_el1, 1);
+	isb();
+
+	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
+}
+
 static void install_wp(uint64_t addr)
 {
 	uint32_t wcr;
@@ -99,6 +107,7 @@ static void guest_code(void)
 	GUEST_SYNC(0);
 
 	/* Software-breakpoint */
+	reset_debug_state();
 	asm volatile("sw_bp: brk #0");
 	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
 
@@ -152,6 +161,51 @@ static void guest_code(void)
 	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
 	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
 
+	GUEST_SYNC(6);
+
+	/* OS Lock blocking software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, 0);
+
+	GUEST_SYNC(7);
+
+	/* OS Lock blocking hardware-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	install_hw_bp(PC(hw_bp2));
+	hw_bp_addr = 0;
+	asm volatile("hw_bp2: nop");
+	GUEST_ASSERT_EQ(hw_bp_addr, 0);
+
+	GUEST_SYNC(8);
+
+	/* OS Lock blocking watchpoint */
+	reset_debug_state();
+	enable_os_lock();
+	write_data = '\0';
+	wp_data_addr = 0;
+	install_wp(PC(write_data));
+	write_data = 'x';
+	GUEST_ASSERT_EQ(write_data, 'x');
+	GUEST_ASSERT_EQ(wp_data_addr, 0);
+
+	GUEST_SYNC(9);
+
+	/* OS Lock blocking single-step */
+	reset_debug_state();
+	enable_os_lock();
+	ss_addr[0] = 0;
+	install_ss();
+	ss_idx = 0;
+	asm volatile("mrs x0, esr_el1\n\t"
+		     "add x0, x0, #1\n\t"
+		     "msr daifset, #8\n\t"
+		     : : : "x0");
+	GUEST_ASSERT_EQ(ss_addr[0], 0);
+
 	GUEST_DONE();
 }
 
@@ -223,7 +277,7 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
-	for (stage = 0; stage < 7; stage++) {
+	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vm, VCPU_ID);
 
 		switch (get_ucall(vm, VCPU_ID, &uc)) {
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

KVM now correctly handles the OS Lock for its guests. When set, KVM
blocks all debug exceptions originating from the guest. Add test cases
to the debug-exceptions test to assert that software breakpoint,
hardware breakpoint, watchpoint, and single-step exceptions are in fact
blocked.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index e5e6c92b60da..6b6ff81cdd23 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -23,7 +23,7 @@
 #define SPSR_D		(1 << 9)
 #define SPSR_SS		(1 << 21)
 
-extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
+extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
 static volatile uint64_t sw_bp_addr, hw_bp_addr;
 static volatile uint64_t wp_addr, wp_data_addr;
 static volatile uint64_t svc_addr;
@@ -47,6 +47,14 @@ static void reset_debug_state(void)
 	isb();
 }
 
+static void enable_os_lock(void)
+{
+	write_sysreg(oslar_el1, 1);
+	isb();
+
+	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
+}
+
 static void install_wp(uint64_t addr)
 {
 	uint32_t wcr;
@@ -99,6 +107,7 @@ static void guest_code(void)
 	GUEST_SYNC(0);
 
 	/* Software-breakpoint */
+	reset_debug_state();
 	asm volatile("sw_bp: brk #0");
 	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
 
@@ -152,6 +161,51 @@ static void guest_code(void)
 	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
 	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
 
+	GUEST_SYNC(6);
+
+	/* OS Lock blocking software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, 0);
+
+	GUEST_SYNC(7);
+
+	/* OS Lock blocking hardware-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	install_hw_bp(PC(hw_bp2));
+	hw_bp_addr = 0;
+	asm volatile("hw_bp2: nop");
+	GUEST_ASSERT_EQ(hw_bp_addr, 0);
+
+	GUEST_SYNC(8);
+
+	/* OS Lock blocking watchpoint */
+	reset_debug_state();
+	enable_os_lock();
+	write_data = '\0';
+	wp_data_addr = 0;
+	install_wp(PC(write_data));
+	write_data = 'x';
+	GUEST_ASSERT_EQ(write_data, 'x');
+	GUEST_ASSERT_EQ(wp_data_addr, 0);
+
+	GUEST_SYNC(9);
+
+	/* OS Lock blocking single-step */
+	reset_debug_state();
+	enable_os_lock();
+	ss_addr[0] = 0;
+	install_ss();
+	ss_idx = 0;
+	asm volatile("mrs x0, esr_el1\n\t"
+		     "add x0, x0, #1\n\t"
+		     "msr daifset, #8\n\t"
+		     : : : "x0");
+	GUEST_ASSERT_EQ(ss_addr[0], 0);
+
 	GUEST_DONE();
 }
 
@@ -223,7 +277,7 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
-	for (stage = 0; stage < 7; stage++) {
+	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vm, VCPU_ID);
 
 		switch (get_ucall(vm, VCPU_ID, &uc)) {
-- 
2.33.1.1089.g2158813163f-goog

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

[PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

KVM now correctly handles the OS Lock for its guests. When set, KVM
blocks all debug exceptions originating from the guest. Add test cases
to the debug-exceptions test to assert that software breakpoint,
hardware breakpoint, watchpoint, and single-step exceptions are in fact
blocked.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
index e5e6c92b60da..6b6ff81cdd23 100644
--- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
+++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
@@ -23,7 +23,7 @@
 #define SPSR_D		(1 << 9)
 #define SPSR_SS		(1 << 21)
 
-extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
+extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
 static volatile uint64_t sw_bp_addr, hw_bp_addr;
 static volatile uint64_t wp_addr, wp_data_addr;
 static volatile uint64_t svc_addr;
@@ -47,6 +47,14 @@ static void reset_debug_state(void)
 	isb();
 }
 
+static void enable_os_lock(void)
+{
+	write_sysreg(oslar_el1, 1);
+	isb();
+
+	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
+}
+
 static void install_wp(uint64_t addr)
 {
 	uint32_t wcr;
@@ -99,6 +107,7 @@ static void guest_code(void)
 	GUEST_SYNC(0);
 
 	/* Software-breakpoint */
+	reset_debug_state();
 	asm volatile("sw_bp: brk #0");
 	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
 
@@ -152,6 +161,51 @@ static void guest_code(void)
 	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
 	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
 
+	GUEST_SYNC(6);
+
+	/* OS Lock blocking software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, 0);
+
+	GUEST_SYNC(7);
+
+	/* OS Lock blocking hardware-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	install_hw_bp(PC(hw_bp2));
+	hw_bp_addr = 0;
+	asm volatile("hw_bp2: nop");
+	GUEST_ASSERT_EQ(hw_bp_addr, 0);
+
+	GUEST_SYNC(8);
+
+	/* OS Lock blocking watchpoint */
+	reset_debug_state();
+	enable_os_lock();
+	write_data = '\0';
+	wp_data_addr = 0;
+	install_wp(PC(write_data));
+	write_data = 'x';
+	GUEST_ASSERT_EQ(write_data, 'x');
+	GUEST_ASSERT_EQ(wp_data_addr, 0);
+
+	GUEST_SYNC(9);
+
+	/* OS Lock blocking single-step */
+	reset_debug_state();
+	enable_os_lock();
+	ss_addr[0] = 0;
+	install_ss();
+	ss_idx = 0;
+	asm volatile("mrs x0, esr_el1\n\t"
+		     "add x0, x0, #1\n\t"
+		     "msr daifset, #8\n\t"
+		     : : : "x0");
+	GUEST_ASSERT_EQ(ss_addr[0], 0);
+
 	GUEST_DONE();
 }
 
@@ -223,7 +277,7 @@ int main(int argc, char *argv[])
 	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
 				ESR_EC_SVC64, guest_svc_handler);
 
-	for (stage = 0; stage < 7; stage++) {
+	for (stage = 0; stage < 11; stage++) {
 		vcpu_run(vm, VCPU_ID);
 
 		switch (get_ucall(vm, VCPU_ID, &uc)) {
-- 
2.33.1.1089.g2158813163f-goog


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

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> An upcoming change to KVM will context switch the OS Lock status between
> guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> using the stored value.
>
> Wire up a custom handler for writes from userspace and prevent any of
> the invariant bits from changing.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8be56d5342b..c98f65c4a1f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -172,6 +172,7 @@ enum vcpu_sysreg {
>         MDSCR_EL1,      /* Monitor Debug System Control Register */
>         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
>         DISR_EL1,       /* Deferred Interrupt Status Register */
> +       OSLSR_EL1,      /* OS Lock Status Register */

Sorry Marc, forgot to move this up per your suggestion on the last
series. Only caught it once the patch went out the door.

--
Oliver

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> An upcoming change to KVM will context switch the OS Lock status between
> guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> using the stored value.
>
> Wire up a custom handler for writes from userspace and prevent any of
> the invariant bits from changing.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8be56d5342b..c98f65c4a1f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -172,6 +172,7 @@ enum vcpu_sysreg {
>         MDSCR_EL1,      /* Monitor Debug System Control Register */
>         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
>         DISR_EL1,       /* Deferred Interrupt Status Register */
> +       OSLSR_EL1,      /* OS Lock Status Register */

Sorry Marc, forgot to move this up per your suggestion on the last
series. Only caught it once the patch went out the door.

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

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Oliver Upton]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> An upcoming change to KVM will context switch the OS Lock status between
> guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> using the stored value.
>
> Wire up a custom handler for writes from userspace and prevent any of
> the invariant bits from changing.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8be56d5342b..c98f65c4a1f7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -172,6 +172,7 @@ enum vcpu_sysreg {
>         MDSCR_EL1,      /* Monitor Debug System Control Register */
>         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
>         DISR_EL1,       /* Deferred Interrupt Status Register */
> +       OSLSR_EL1,      /* OS Lock Status Register */

Sorry Marc, forgot to move this up per your suggestion on the last
series. Only caught it once the patch went out the door.

--
Oliver

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Marc Zyngier]

Hi Oliver,

On Tue, 02 Nov 2021 09:46:51 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);
> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);

I haven't had a change to properly review the series, but this one
definitely caught my eye. My expectations are that BRK is *not*
affected by the OS Lock. The ARMv8 ARM goes as far as saying:

<quote>
Breakpoint Instruction exceptions are enabled regardless of the state
of the OS Lock and the OS Double Lock.
</quote>

as well as:

<quote>
There is no enable control for Breakpoint Instruction exceptions. They
are always enabled, and cannot be masked.
</quote>

I wonder how your test succeeds, though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Marc Zyngier]

Hi Oliver,

On Tue, 02 Nov 2021 09:46:51 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);
> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);

I haven't had a change to properly review the series, but this one
definitely caught my eye. My expectations are that BRK is *not*
affected by the OS Lock. The ARMv8 ARM goes as far as saying:

<quote>
Breakpoint Instruction exceptions are enabled regardless of the state
of the OS Lock and the OS Double Lock.
</quote>

as well as:

<quote>
There is no enable control for Breakpoint Instruction exceptions. They
are always enabled, and cannot be masked.
</quote>

I wonder how your test succeeds, though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Marc Zyngier]

Hi Oliver,

On Tue, 02 Nov 2021 09:46:51 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);
> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);

I haven't had a change to properly review the series, but this one
definitely caught my eye. My expectations are that BRK is *not*
affected by the OS Lock. The ARMv8 ARM goes as far as saying:

<quote>
Breakpoint Instruction exceptions are enabled regardless of the state
of the OS Lock and the OS Double Lock.
</quote>

as well as:

<quote>
There is no enable control for Breakpoint Instruction exceptions. They
are always enabled, and cannot be masked.
</quote>

I wonder how your test succeeds, though.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hey Marc,

On Tue, Nov 2, 2021 at 4:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> On Tue, 02 Nov 2021 09:46:51 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > KVM now correctly handles the OS Lock for its guests. When set, KVM
> > blocks all debug exceptions originating from the guest. Add test cases
> > to the debug-exceptions test to assert that software breakpoint,
> > hardware breakpoint, watchpoint, and single-step exceptions are in fact
> > blocked.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index e5e6c92b60da..6b6ff81cdd23 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -23,7 +23,7 @@
> >  #define SPSR_D               (1 << 9)
> >  #define SPSR_SS              (1 << 21)
> >
> > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> >  static volatile uint64_t sw_bp_addr, hw_bp_addr;
> >  static volatile uint64_t wp_addr, wp_data_addr;
> >  static volatile uint64_t svc_addr;
> > @@ -47,6 +47,14 @@ static void reset_debug_state(void)
> >       isb();
> >  }
> >
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
> > +     isb();
> > +
> > +     GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> > +}
> > +
> >  static void install_wp(uint64_t addr)
> >  {
> >       uint32_t wcr;
> > @@ -99,6 +107,7 @@ static void guest_code(void)
> >       GUEST_SYNC(0);
> >
> >       /* Software-breakpoint */
> > +     reset_debug_state();
> >       asm volatile("sw_bp: brk #0");
> >       GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
> >
> > @@ -152,6 +161,51 @@ static void guest_code(void)
> >       GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
> >       GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
> >
> > +     GUEST_SYNC(6);
> > +
> > +     /* OS Lock blocking software-breakpoint */
> > +     reset_debug_state();
> > +     enable_os_lock();
> > +     sw_bp_addr = 0;
> > +     asm volatile("brk #0");
> > +     GUEST_ASSERT_EQ(sw_bp_addr, 0);
>
> I haven't had a change to properly review the series, but this one
> definitely caught my eye. My expectations are that BRK is *not*
> affected by the OS Lock. The ARMv8 ARM goes as far as saying:
>
> <quote>
> Breakpoint Instruction exceptions are enabled regardless of the state
> of the OS Lock and the OS Double Lock.
> </quote>
>
> as well as:
>
> <quote>
> There is no enable control for Breakpoint Instruction exceptions. They
> are always enabled, and cannot be masked.
> </quote>

/facepalm I had thought I read "Breakpoint Instruction exceptions" in
the list on D2.5 "The effect of powerdown on debug exceptions",
although on second read I most definitely did not. And if I had read
the bottom of the section, I'd of seen one of the quotes.

> I wonder how your test succeeds, though.

Probably because the expectations I wrote match the non-architected
behavior I implemented :-)

--
Thanks,
Oliver

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hey Marc,

On Tue, Nov 2, 2021 at 4:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> On Tue, 02 Nov 2021 09:46:51 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > KVM now correctly handles the OS Lock for its guests. When set, KVM
> > blocks all debug exceptions originating from the guest. Add test cases
> > to the debug-exceptions test to assert that software breakpoint,
> > hardware breakpoint, watchpoint, and single-step exceptions are in fact
> > blocked.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index e5e6c92b60da..6b6ff81cdd23 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -23,7 +23,7 @@
> >  #define SPSR_D               (1 << 9)
> >  #define SPSR_SS              (1 << 21)
> >
> > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> >  static volatile uint64_t sw_bp_addr, hw_bp_addr;
> >  static volatile uint64_t wp_addr, wp_data_addr;
> >  static volatile uint64_t svc_addr;
> > @@ -47,6 +47,14 @@ static void reset_debug_state(void)
> >       isb();
> >  }
> >
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
> > +     isb();
> > +
> > +     GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> > +}
> > +
> >  static void install_wp(uint64_t addr)
> >  {
> >       uint32_t wcr;
> > @@ -99,6 +107,7 @@ static void guest_code(void)
> >       GUEST_SYNC(0);
> >
> >       /* Software-breakpoint */
> > +     reset_debug_state();
> >       asm volatile("sw_bp: brk #0");
> >       GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
> >
> > @@ -152,6 +161,51 @@ static void guest_code(void)
> >       GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
> >       GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
> >
> > +     GUEST_SYNC(6);
> > +
> > +     /* OS Lock blocking software-breakpoint */
> > +     reset_debug_state();
> > +     enable_os_lock();
> > +     sw_bp_addr = 0;
> > +     asm volatile("brk #0");
> > +     GUEST_ASSERT_EQ(sw_bp_addr, 0);
>
> I haven't had a change to properly review the series, but this one
> definitely caught my eye. My expectations are that BRK is *not*
> affected by the OS Lock. The ARMv8 ARM goes as far as saying:
>
> <quote>
> Breakpoint Instruction exceptions are enabled regardless of the state
> of the OS Lock and the OS Double Lock.
> </quote>
>
> as well as:
>
> <quote>
> There is no enable control for Breakpoint Instruction exceptions. They
> are always enabled, and cannot be masked.
> </quote>

/facepalm I had thought I read "Breakpoint Instruction exceptions" in
the list on D2.5 "The effect of powerdown on debug exceptions",
although on second read I most definitely did not. And if I had read
the bottom of the section, I'd of seen one of the quotes.

> I wonder how your test succeeds, though.

Probably because the expectations I wrote match the non-architected
behavior I implemented :-)

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hey Marc,

On Tue, Nov 2, 2021 at 4:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> On Tue, 02 Nov 2021 09:46:51 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > KVM now correctly handles the OS Lock for its guests. When set, KVM
> > blocks all debug exceptions originating from the guest. Add test cases
> > to the debug-exceptions test to assert that software breakpoint,
> > hardware breakpoint, watchpoint, and single-step exceptions are in fact
> > blocked.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index e5e6c92b60da..6b6ff81cdd23 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -23,7 +23,7 @@
> >  #define SPSR_D               (1 << 9)
> >  #define SPSR_SS              (1 << 21)
> >
> > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> >  static volatile uint64_t sw_bp_addr, hw_bp_addr;
> >  static volatile uint64_t wp_addr, wp_data_addr;
> >  static volatile uint64_t svc_addr;
> > @@ -47,6 +47,14 @@ static void reset_debug_state(void)
> >       isb();
> >  }
> >
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
> > +     isb();
> > +
> > +     GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> > +}
> > +
> >  static void install_wp(uint64_t addr)
> >  {
> >       uint32_t wcr;
> > @@ -99,6 +107,7 @@ static void guest_code(void)
> >       GUEST_SYNC(0);
> >
> >       /* Software-breakpoint */
> > +     reset_debug_state();
> >       asm volatile("sw_bp: brk #0");
> >       GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
> >
> > @@ -152,6 +161,51 @@ static void guest_code(void)
> >       GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
> >       GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
> >
> > +     GUEST_SYNC(6);
> > +
> > +     /* OS Lock blocking software-breakpoint */
> > +     reset_debug_state();
> > +     enable_os_lock();
> > +     sw_bp_addr = 0;
> > +     asm volatile("brk #0");
> > +     GUEST_ASSERT_EQ(sw_bp_addr, 0);
>
> I haven't had a change to properly review the series, but this one
> definitely caught my eye. My expectations are that BRK is *not*
> affected by the OS Lock. The ARMv8 ARM goes as far as saying:
>
> <quote>
> Breakpoint Instruction exceptions are enabled regardless of the state
> of the OS Lock and the OS Double Lock.
> </quote>
>
> as well as:
>
> <quote>
> There is no enable control for Breakpoint Instruction exceptions. They
> are always enabled, and cannot be masked.
> </quote>

/facepalm I had thought I read "Breakpoint Instruction exceptions" in
the list on D2.5 "The effect of powerdown on debug exceptions",
although on second read I most definitely did not. And if I had read
the bottom of the section, I'd of seen one of the quotes.

> I wonder how your test succeeds, though.

Probably because the expectations I wrote match the non-architected
behavior I implemented :-)

--
Thanks,
Oliver

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

On Tue, Nov 2, 2021 at 7:53 AM Oliver Upton <oupton@google.com> wrote:
> >
> > I haven't had a change to properly review the series, but this one
> > definitely caught my eye. My expectations are that BRK is *not*
> > affected by the OS Lock. The ARMv8 ARM goes as far as saying:
> >
> > <quote>
> > Breakpoint Instruction exceptions are enabled regardless of the state
> > of the OS Lock and the OS Double Lock.
> > </quote>
> >
> > as well as:
> >
> > <quote>
> > There is no enable control for Breakpoint Instruction exceptions. They
> > are always enabled, and cannot be masked.
> > </quote>
>
> /facepalm I had thought I read "Breakpoint Instruction exceptions" in
> the list on D2.5 "The effect of powerdown on debug exceptions",
> although on second read I most definitely did not. And if I had read
> the bottom of the section, I'd of seen one of the quotes.
>
> > I wonder how your test succeeds, though.
>
> Probably because the expectations I wrote match the non-architected
> behavior I implemented :-)

Alright, gave the series a good once over after this and fixed up
quite a few things. Unless you're ready for it, I'll hold back for a
bit to avoid spamming inboxes. As an FYI, here's the fixes I have
queued up:

v2 -> v3:
- Stop trapping debug exceptions when the OS Lock is enabled, as it
   does *not* block software breakpoint exceptions (Marc)
 - Trap accesses to debug registers if the OS Lock is enabled to prevent
   the guest from wiping out KVM's configuration of MDSCR_EL1
 - Update the debug-exceptions test to expect a software breakpoint
   exception even when the OS Lock is enabled.

--
Thanks,
Oliver

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

On Tue, Nov 2, 2021 at 7:53 AM Oliver Upton <oupton@google.com> wrote:
> >
> > I haven't had a change to properly review the series, but this one
> > definitely caught my eye. My expectations are that BRK is *not*
> > affected by the OS Lock. The ARMv8 ARM goes as far as saying:
> >
> > <quote>
> > Breakpoint Instruction exceptions are enabled regardless of the state
> > of the OS Lock and the OS Double Lock.
> > </quote>
> >
> > as well as:
> >
> > <quote>
> > There is no enable control for Breakpoint Instruction exceptions. They
> > are always enabled, and cannot be masked.
> > </quote>
>
> /facepalm I had thought I read "Breakpoint Instruction exceptions" in
> the list on D2.5 "The effect of powerdown on debug exceptions",
> although on second read I most definitely did not. And if I had read
> the bottom of the section, I'd of seen one of the quotes.
>
> > I wonder how your test succeeds, though.
>
> Probably because the expectations I wrote match the non-architected
> behavior I implemented :-)

Alright, gave the series a good once over after this and fixed up
quite a few things. Unless you're ready for it, I'll hold back for a
bit to avoid spamming inboxes. As an FYI, here's the fixes I have
queued up:

v2 -> v3:
- Stop trapping debug exceptions when the OS Lock is enabled, as it
   does *not* block software breakpoint exceptions (Marc)
 - Trap accesses to debug registers if the OS Lock is enabled to prevent
   the guest from wiping out KVM's configuration of MDSCR_EL1
 - Update the debug-exceptions test to expect a software breakpoint
   exception even when the OS Lock is enabled.

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

On Tue, Nov 2, 2021 at 7:53 AM Oliver Upton <oupton@google.com> wrote:
> >
> > I haven't had a change to properly review the series, but this one
> > definitely caught my eye. My expectations are that BRK is *not*
> > affected by the OS Lock. The ARMv8 ARM goes as far as saying:
> >
> > <quote>
> > Breakpoint Instruction exceptions are enabled regardless of the state
> > of the OS Lock and the OS Double Lock.
> > </quote>
> >
> > as well as:
> >
> > <quote>
> > There is no enable control for Breakpoint Instruction exceptions. They
> > are always enabled, and cannot be masked.
> > </quote>
>
> /facepalm I had thought I read "Breakpoint Instruction exceptions" in
> the list on D2.5 "The effect of powerdown on debug exceptions",
> although on second read I most definitely did not. And if I had read
> the bottom of the section, I'd of seen one of the quotes.
>
> > I wonder how your test succeeds, though.
>
> Probably because the expectations I wrote match the non-architected
> behavior I implemented :-)

Alright, gave the series a good once over after this and fixed up
quite a few things. Unless you're ready for it, I'll hold back for a
bit to avoid spamming inboxes. As an FYI, here's the fixes I have
queued up:

v2 -> v3:
- Stop trapping debug exceptions when the OS Lock is enabled, as it
   does *not* block software breakpoint exceptions (Marc)
 - Trap accesses to debug registers if the OS Lock is enabled to prevent
   the guest from wiping out KVM's configuration of MDSCR_EL1
 - Update the debug-exceptions test to expect a software breakpoint
   exception even when the OS Lock is enabled.

--
Thanks,
Oliver

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:51AM +0000, Oliver Upton wrote:
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);

should be: write_sysreg(val, reg);

> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);
> +
> +	GUEST_SYNC(7);
> +
> +	/* OS Lock blocking hardware-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	install_hw_bp(PC(hw_bp2));
> +	hw_bp_addr = 0;
> +	asm volatile("hw_bp2: nop");
> +	GUEST_ASSERT_EQ(hw_bp_addr, 0);
> +
> +	GUEST_SYNC(8);
> +
> +	/* OS Lock blocking watchpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	write_data = '\0';
> +	wp_data_addr = 0;
> +	install_wp(PC(write_data));
> +	write_data = 'x';
> +	GUEST_ASSERT_EQ(write_data, 'x');
> +	GUEST_ASSERT_EQ(wp_data_addr, 0);
> +
> +	GUEST_SYNC(9);
> +
> +	/* OS Lock blocking single-step */
> +	reset_debug_state();
> +	enable_os_lock();
> +	ss_addr[0] = 0;
> +	install_ss();
> +	ss_idx = 0;
> +	asm volatile("mrs x0, esr_el1\n\t"
> +		     "add x0, x0, #1\n\t"
> +		     "msr daifset, #8\n\t"
> +		     : : : "x0");
> +	GUEST_ASSERT_EQ(ss_addr[0], 0);
> +
>  	GUEST_DONE();
>  }
>  
> @@ -223,7 +277,7 @@ int main(int argc, char *argv[])
>  	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>  				ESR_EC_SVC64, guest_svc_handler);
>  
> -	for (stage = 0; stage < 7; stage++) {
> +	for (stage = 0; stage < 11; stage++) {
>  		vcpu_run(vm, VCPU_ID);
>  
>  		switch (get_ucall(vm, VCPU_ID, &uc)) {
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:51AM +0000, Oliver Upton wrote:
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);

should be: write_sysreg(val, reg);

> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);
> +
> +	GUEST_SYNC(7);
> +
> +	/* OS Lock blocking hardware-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	install_hw_bp(PC(hw_bp2));
> +	hw_bp_addr = 0;
> +	asm volatile("hw_bp2: nop");
> +	GUEST_ASSERT_EQ(hw_bp_addr, 0);
> +
> +	GUEST_SYNC(8);
> +
> +	/* OS Lock blocking watchpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	write_data = '\0';
> +	wp_data_addr = 0;
> +	install_wp(PC(write_data));
> +	write_data = 'x';
> +	GUEST_ASSERT_EQ(write_data, 'x');
> +	GUEST_ASSERT_EQ(wp_data_addr, 0);
> +
> +	GUEST_SYNC(9);
> +
> +	/* OS Lock blocking single-step */
> +	reset_debug_state();
> +	enable_os_lock();
> +	ss_addr[0] = 0;
> +	install_ss();
> +	ss_idx = 0;
> +	asm volatile("mrs x0, esr_el1\n\t"
> +		     "add x0, x0, #1\n\t"
> +		     "msr daifset, #8\n\t"
> +		     : : : "x0");
> +	GUEST_ASSERT_EQ(ss_addr[0], 0);
> +
>  	GUEST_DONE();
>  }
>  
> @@ -223,7 +277,7 @@ int main(int argc, char *argv[])
>  	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>  				ESR_EC_SVC64, guest_svc_handler);
>  
> -	for (stage = 0; stage < 7; stage++) {
> +	for (stage = 0; stage < 11; stage++) {
>  		vcpu_run(vm, VCPU_ID);
>  
>  		switch (get_ucall(vm, VCPU_ID, &uc)) {
> -- 
> 2.33.1.1089.g2158813163f-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:51AM +0000, Oliver Upton wrote:
> KVM now correctly handles the OS Lock for its guests. When set, KVM
> blocks all debug exceptions originating from the guest. Add test cases
> to the debug-exceptions test to assert that software breakpoint,
> hardware breakpoint, watchpoint, and single-step exceptions are in fact
> blocked.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index e5e6c92b60da..6b6ff81cdd23 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -23,7 +23,7 @@
>  #define SPSR_D		(1 << 9)
>  #define SPSR_SS		(1 << 21)
>  
> -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start;
> +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
>  static volatile uint64_t sw_bp_addr, hw_bp_addr;
>  static volatile uint64_t wp_addr, wp_data_addr;
>  static volatile uint64_t svc_addr;
> @@ -47,6 +47,14 @@ static void reset_debug_state(void)
>  	isb();
>  }
>  
> +static void enable_os_lock(void)
> +{
> +	write_sysreg(oslar_el1, 1);

should be: write_sysreg(val, reg);

> +	isb();
> +
> +	GUEST_ASSERT(read_sysreg(oslsr_el1) & 2);
> +}
> +
>  static void install_wp(uint64_t addr)
>  {
>  	uint32_t wcr;
> @@ -99,6 +107,7 @@ static void guest_code(void)
>  	GUEST_SYNC(0);
>  
>  	/* Software-breakpoint */
> +	reset_debug_state();
>  	asm volatile("sw_bp: brk #0");
>  	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp));
>  
> @@ -152,6 +161,51 @@ static void guest_code(void)
>  	GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4);
>  	GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8);
>  
> +	GUEST_SYNC(6);
> +
> +	/* OS Lock blocking software-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	sw_bp_addr = 0;
> +	asm volatile("brk #0");
> +	GUEST_ASSERT_EQ(sw_bp_addr, 0);
> +
> +	GUEST_SYNC(7);
> +
> +	/* OS Lock blocking hardware-breakpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	install_hw_bp(PC(hw_bp2));
> +	hw_bp_addr = 0;
> +	asm volatile("hw_bp2: nop");
> +	GUEST_ASSERT_EQ(hw_bp_addr, 0);
> +
> +	GUEST_SYNC(8);
> +
> +	/* OS Lock blocking watchpoint */
> +	reset_debug_state();
> +	enable_os_lock();
> +	write_data = '\0';
> +	wp_data_addr = 0;
> +	install_wp(PC(write_data));
> +	write_data = 'x';
> +	GUEST_ASSERT_EQ(write_data, 'x');
> +	GUEST_ASSERT_EQ(wp_data_addr, 0);
> +
> +	GUEST_SYNC(9);
> +
> +	/* OS Lock blocking single-step */
> +	reset_debug_state();
> +	enable_os_lock();
> +	ss_addr[0] = 0;
> +	install_ss();
> +	ss_idx = 0;
> +	asm volatile("mrs x0, esr_el1\n\t"
> +		     "add x0, x0, #1\n\t"
> +		     "msr daifset, #8\n\t"
> +		     : : : "x0");
> +	GUEST_ASSERT_EQ(ss_addr[0], 0);
> +
>  	GUEST_DONE();
>  }
>  
> @@ -223,7 +277,7 @@ int main(int argc, char *argv[])
>  	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
>  				ESR_EC_SVC64, guest_svc_handler);
>  
> -	for (stage = 0; stage < 7; stage++) {
> +	for (stage = 0; stage < 11; stage++) {
>  		vcpu_run(vm, VCPU_ID);
>  
>  		switch (get_ucall(vm, VCPU_ID, &uc)) {
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:27 PM Ricardo Koller <ricarkol@google.com> wrote:
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
>
> should be: write_sysreg(val, reg);

Yep, I'll do this once I rebase onto 5.16, as the sysreg rework isn't
available til then.

--
Thanks,
Oliver

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:27 PM Ricardo Koller <ricarkol@google.com> wrote:
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
>
> should be: write_sysreg(val, reg);

Yep, I'll do this once I rebase onto 5.16, as the sysreg rework isn't
available til then.

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

Re: [PATCH v2 6/6] selftests: KVM: Test OS lock behavior

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:27 PM Ricardo Koller <ricarkol@google.com> wrote:
> > +static void enable_os_lock(void)
> > +{
> > +     write_sysreg(oslar_el1, 1);
>
> should be: write_sysreg(val, reg);

Yep, I'll do this once I rebase onto 5.16, as the sysreg rework isn't
available til then.

--
Thanks,
Oliver

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
> 
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)		\
> +	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  			       struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  				MDCR_EL2_TDRA |
>  				MDCR_EL2_TDOSA);
>  
> -	/* Is the VM being debugged by userspace? */
> -	if (vcpu->guest_debug)
> +	/*
> +	 * Check if the VM is being debugged by userspace or the guest has
> +	 * enabled the OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>  		/* Route all software debug exceptions to EL2 */
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>  
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  
>  	kvm_arm_setup_mdcr_el2(vcpu);
>  
> -	/* Is Guest debugging in effect? */
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Check if the guest is being debugged or if the guest has enabled the
> +	 * OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>  						&vcpu->arch.debug_ptr->dbg_wcr[0],
>  						&vcpu->arch.debug_ptr->dbg_wvr[0]);
> +		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +			mdscr &= ~DBG_MDSCR_MDE;
> +			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);

I think this is missing the case where the guest is being debugged by
userspace _and_ from inside (the guest) at the same time. In this
situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
do with it, it injects the exception into the guest (1). With this "else
if", the guest would still get the debug exception when the os lock is
enabled.

(1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

>  		}
>  	}
>  
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
>  	u32 esr = kvm_vcpu_get_esr(vcpu);
> +	u8 esr_ec = ESR_ELx_EC(esr);
> +
> +	if (!vcpu->guest_debug) {
> +		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +			  "Unexpected debug exception\n");
> +		kvm_incr_pc(vcpu);
> +		return 1;
> +	}
>  
>  	run->exit_reason = KVM_EXIT_DEBUG;
>  	run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
> 
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)		\
> +	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  			       struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  				MDCR_EL2_TDRA |
>  				MDCR_EL2_TDOSA);
>  
> -	/* Is the VM being debugged by userspace? */
> -	if (vcpu->guest_debug)
> +	/*
> +	 * Check if the VM is being debugged by userspace or the guest has
> +	 * enabled the OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>  		/* Route all software debug exceptions to EL2 */
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>  
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  
>  	kvm_arm_setup_mdcr_el2(vcpu);
>  
> -	/* Is Guest debugging in effect? */
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Check if the guest is being debugged or if the guest has enabled the
> +	 * OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>  						&vcpu->arch.debug_ptr->dbg_wcr[0],
>  						&vcpu->arch.debug_ptr->dbg_wvr[0]);
> +		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +			mdscr &= ~DBG_MDSCR_MDE;
> +			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);

I think this is missing the case where the guest is being debugged by
userspace _and_ from inside (the guest) at the same time. In this
situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
do with it, it injects the exception into the guest (1). With this "else
if", the guest would still get the debug exception when the os lock is
enabled.

(1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

>  		}
>  	}
>  
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
>  	u32 esr = kvm_vcpu_get_esr(vcpu);
> +	u8 esr_ec = ESR_ELx_EC(esr);
> +
> +	if (!vcpu->guest_debug) {
> +		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +			  "Unexpected debug exception\n");
> +		kvm_incr_pc(vcpu);
> +		return 1;
> +	}
>  
>  	run->exit_reason = KVM_EXIT_DEBUG;
>  	run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
> -- 
> 2.33.1.1089.g2158813163f-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Ricardo Koller]

On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
> 
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)		\
> +	(__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  			       struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  				MDCR_EL2_TDRA |
>  				MDCR_EL2_TDOSA);
>  
> -	/* Is the VM being debugged by userspace? */
> -	if (vcpu->guest_debug)
> +	/*
> +	 * Check if the VM is being debugged by userspace or the guest has
> +	 * enabled the OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>  		/* Route all software debug exceptions to EL2 */
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>  
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  
>  	kvm_arm_setup_mdcr_el2(vcpu);
>  
> -	/* Is Guest debugging in effect? */
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Check if the guest is being debugged or if the guest has enabled the
> +	 * OS lock.
> +	 */
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>  						&vcpu->arch.debug_ptr->dbg_wcr[0],
>  						&vcpu->arch.debug_ptr->dbg_wvr[0]);
> +		} else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +			mdscr &= ~DBG_MDSCR_MDE;
> +			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);

I think this is missing the case where the guest is being debugged by
userspace _and_ from inside (the guest) at the same time. In this
situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
do with it, it injects the exception into the guest (1). With this "else
if", the guest would still get the debug exception when the os lock is
enabled.

(1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

>  		}
>  	}
>  
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_run *run = vcpu->run;
>  	u32 esr = kvm_vcpu_get_esr(vcpu);
> +	u8 esr_ec = ESR_ELx_EC(esr);
> +
> +	if (!vcpu->guest_debug) {
> +		WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +			  "Unexpected debug exception\n");
> +		kvm_incr_pc(vcpu);
> +		return 1;
> +	}
>  
>  	run->exit_reason = KVM_EXIT_DEBUG;
>  	run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_DC_ISW), access_dcsw },
> -- 
> 2.33.1.1089.g2158813163f-goog
> 

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:45 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)               \
> > +     (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                              struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                               MDCR_EL2_TDRA |
> >                               MDCR_EL2_TDOSA);
> >
> > -     /* Is the VM being debugged by userspace? */
> > -     if (vcpu->guest_debug)
> > +     /*
> > +      * Check if the VM is being debugged by userspace or the guest has
> > +      * enabled the OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
> >               /* Route all software debug exceptions to EL2 */
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> >
> > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >
> >       kvm_arm_setup_mdcr_el2(vcpu);
> >
> > -     /* Is Guest debugging in effect? */
> > -     if (vcpu->guest_debug) {
> > +     /*
> > +      * Check if the guest is being debugged or if the guest has enabled the
> > +      * OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> >               /* Save guest debug state */
> >               save_guest_debug_regs(vcpu);
> >
> > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >                       trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> >                                               &vcpu->arch.debug_ptr->dbg_wcr[0],
> >                                               &vcpu->arch.debug_ptr->dbg_wvr[0]);
> > +             } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> > +                     mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> > +                     mdscr &= ~DBG_MDSCR_MDE;
> > +                     vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>
> I think this is missing the case where the guest is being debugged by
> userspace _and_ from inside (the guest) at the same time. In this
> situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
> do with it, it injects the exception into the guest (1). With this "else
> if", the guest would still get the debug exception when the os lock is
> enabled.
>
> (1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

I wonder if this is a problem that KVM should even handle. KVM doesn't
do anything to help userspace inject the debug exception into the
guest, and from reading kvm_arm_handle_debug() it would seem that QEMU
is manually injecting the exception to EL1 and setting the PC to the
appropriate vector.

There is an issue, though, with migration: older KVM will not show
OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
architectural OS Lock, its reset value must be 1 (enabled). This would
all have the effect of discarding the guest's OS lock value and
blocking all debug exceptions intended for the guest until the next
reboot.

So it would seem that userspace needs to know about the OSLK bit to
correctly inject debug exceptions and migrate guests. If opt-in is
heavyweight, we could cure the migration issue by explicitly
documenting the OS lock being disabled out of reset as an erratum of
KVM. Doing so would be consistent with all prior versions of KVM. Of
course, adopting nonarchitected behavior in perpetuity seems a bit
unsavory :-)

--
Oliver

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:45 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)               \
> > +     (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                              struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                               MDCR_EL2_TDRA |
> >                               MDCR_EL2_TDOSA);
> >
> > -     /* Is the VM being debugged by userspace? */
> > -     if (vcpu->guest_debug)
> > +     /*
> > +      * Check if the VM is being debugged by userspace or the guest has
> > +      * enabled the OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
> >               /* Route all software debug exceptions to EL2 */
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> >
> > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >
> >       kvm_arm_setup_mdcr_el2(vcpu);
> >
> > -     /* Is Guest debugging in effect? */
> > -     if (vcpu->guest_debug) {
> > +     /*
> > +      * Check if the guest is being debugged or if the guest has enabled the
> > +      * OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> >               /* Save guest debug state */
> >               save_guest_debug_regs(vcpu);
> >
> > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >                       trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> >                                               &vcpu->arch.debug_ptr->dbg_wcr[0],
> >                                               &vcpu->arch.debug_ptr->dbg_wvr[0]);
> > +             } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> > +                     mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> > +                     mdscr &= ~DBG_MDSCR_MDE;
> > +                     vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>
> I think this is missing the case where the guest is being debugged by
> userspace _and_ from inside (the guest) at the same time. In this
> situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
> do with it, it injects the exception into the guest (1). With this "else
> if", the guest would still get the debug exception when the os lock is
> enabled.
>
> (1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

I wonder if this is a problem that KVM should even handle. KVM doesn't
do anything to help userspace inject the debug exception into the
guest, and from reading kvm_arm_handle_debug() it would seem that QEMU
is manually injecting the exception to EL1 and setting the PC to the
appropriate vector.

There is an issue, though, with migration: older KVM will not show
OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
architectural OS Lock, its reset value must be 1 (enabled). This would
all have the effect of discarding the guest's OS lock value and
blocking all debug exceptions intended for the guest until the next
reboot.

So it would seem that userspace needs to know about the OSLK bit to
correctly inject debug exceptions and migrate guests. If opt-in is
heavyweight, we could cure the migration issue by explicitly
documenting the OS lock being disabled out of reset as an erratum of
KVM. Doing so would be consistent with all prior versions of KVM. Of
course, adopting nonarchitected behavior in perpetuity seems a bit
unsavory :-)

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

Hi Ricardo,

On Tue, Nov 2, 2021 at 4:45 PM Ricardo Koller <ricarkol@google.com> wrote:
>
> On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote:
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)               \
> > +     (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                              struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                               MDCR_EL2_TDRA |
> >                               MDCR_EL2_TDOSA);
> >
> > -     /* Is the VM being debugged by userspace? */
> > -     if (vcpu->guest_debug)
> > +     /*
> > +      * Check if the VM is being debugged by userspace or the guest has
> > +      * enabled the OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
> >               /* Route all software debug exceptions to EL2 */
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> >
> > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >
> >       kvm_arm_setup_mdcr_el2(vcpu);
> >
> > -     /* Is Guest debugging in effect? */
> > -     if (vcpu->guest_debug) {
> > +     /*
> > +      * Check if the guest is being debugged or if the guest has enabled the
> > +      * OS lock.
> > +      */
> > +     if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> >               /* Save guest debug state */
> >               save_guest_debug_regs(vcpu);
> >
> > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> >                       trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
> >                                               &vcpu->arch.debug_ptr->dbg_wcr[0],
> >                                               &vcpu->arch.debug_ptr->dbg_wvr[0]);
> > +             } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> > +                     mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> > +                     mdscr &= ~DBG_MDSCR_MDE;
> > +                     vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>
> I think this is missing the case where the guest is being debugged by
> userspace _and_ from inside (the guest) at the same time. In this
> situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to
> do with it, it injects the exception into the guest (1). With this "else
> if", the guest would still get the debug exception when the os lock is
> enabled.
>
> (1) kvm_arm_handle_debug() is the one doing this in QEMU source code.

I wonder if this is a problem that KVM should even handle. KVM doesn't
do anything to help userspace inject the debug exception into the
guest, and from reading kvm_arm_handle_debug() it would seem that QEMU
is manually injecting the exception to EL1 and setting the PC to the
appropriate vector.

There is an issue, though, with migration: older KVM will not show
OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
architectural OS Lock, its reset value must be 1 (enabled). This would
all have the effect of discarding the guest's OS lock value and
blocking all debug exceptions intended for the guest until the next
reboot.

So it would seem that userspace needs to know about the OSLK bit to
correctly inject debug exceptions and migrate guests. If opt-in is
heavyweight, we could cure the migration issue by explicitly
documenting the OS lock being disabled out of reset as an erratum of
KVM. Doing so would be consistent with all prior versions of KVM. Of
course, adopting nonarchitected behavior in perpetuity seems a bit
unsavory :-)

--
Oliver

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

Re: [PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Any valid implementation of the architecture should generate an
> undefined exception for writes to a read-only register, such as
> OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
> behavior.
>
> Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
> write ever traps to EL2, inject an undef into the guest and print a
> warning.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji

Re: [PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Any valid implementation of the architecture should generate an
> undefined exception for writes to a read-only register, such as
> OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
> behavior.
>
> Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
> write ever traps to EL2, inject an undef into the guest and print a
> warning.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

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

Re: [PATCH v2 1/6] KVM: arm64: Correctly treat writes to OSLSR_EL1 as undefined

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Any valid implementation of the architecture should generate an
> undefined exception for writes to a read-only register, such as
> OSLSR_EL1. Nonetheless, the KVM handler actually implements write-ignore
> behavior.
>
> Align the trap handler for OSLSR_EL1 with hardware behavior. If such a
> write ever traps to EL2, inject an undef into the guest and print a
> warning.
>
> Signed-off-by: Oliver Upton <oupton@google.com>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji

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

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> reset value of the OSLK bit to 1. Allow the value to be migrated by
> making OSLSR_EL1.OSLK writable from userspace.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
>  2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index b268082d67ed..6ba4dc97b69d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -127,7 +127,13 @@
>  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
>  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
>  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> +
> +#define SYS_OSLAR_OSLK                 BIT(0)
> +
>  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> +
> +#define SYS_OSLSR_OSLK                 BIT(1)
> +
>  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
>  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
>  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0326b3df0736..acd8aa2e5a44 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -44,6 +44,10 @@
>   * 64bit interface.
>   */
>
> +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> +
>  static bool read_from_write_only(struct kvm_vcpu *vcpu,
>                                  struct sys_reg_params *params,
>                                  const struct sys_reg_desc *r)
> @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
>         return trap_raz_wi(vcpu, p, r);
>  }
>
> +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> +                          struct sys_reg_params *p,
> +                          const struct sys_reg_desc *r)
> +{
> +       u64 oslsr;
> +
> +       if (!p->is_write)
> +               return read_from_write_only(vcpu, p, r);
> +
> +       /* Forward the OSLK bit to OSLSR */
> +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> +       if (p->regval & SYS_OSLAR_OSLK)
> +               oslsr |= SYS_OSLSR_OSLK;
> +
> +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> +       return true;
> +}
> +
>  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>                            struct sys_reg_params *p,
>                            const struct sys_reg_desc *r)
> @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>         if (err)
>                 return err;
>
> -       if (val != rd->val)
> +       if ((val | SYS_OSLSR_OSLK) != rd->val)
>                 return -EINVAL;
>
> +       __vcpu_sys_reg(vcpu, rd->reg) = val;
>         return 0;
>  }
>
> @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
>         return __access_id_reg(vcpu, p, r, true);
>  }
>
> -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> -
>  /* Visibility overrides for SVE-specific control registers */
>  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>                                    const struct sys_reg_desc *rd)
> @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>         DBG_BCR_BVR_WCR_WVR_EL1(15),
>
>         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
>                 .set_user = set_oslsr_el1, },

Reviewed-by: Reiji Watanabe <reijiw@google.com>

I assume the reason why you changed the reset value for the
register is because Arm ARM says "the On a Cold reset,
this field resets to 1".

"4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
-------------------------------------------------------------
  - System registers: Reset to their architecturally defined
    values as for a warm reset to EL1 (resp. SVC)
-------------------------------------------------------------

Since Arm ARM doesn't say anything about a warm reset for the field,
I would guess the bit doesn't necessarily need to be set.

Thanks,
Reiji


>         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
>
>         DBGBXVR(0),
>         /* DBGOSLAR */
> -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
>         DBGBXVR(1),
>         /* DBGOSLSR */
>         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> --
> 2.33.1.1089.g2158813163f-goog
>

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> reset value of the OSLK bit to 1. Allow the value to be migrated by
> making OSLSR_EL1.OSLK writable from userspace.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
>  2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index b268082d67ed..6ba4dc97b69d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -127,7 +127,13 @@
>  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
>  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
>  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> +
> +#define SYS_OSLAR_OSLK                 BIT(0)
> +
>  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> +
> +#define SYS_OSLSR_OSLK                 BIT(1)
> +
>  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
>  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
>  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0326b3df0736..acd8aa2e5a44 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -44,6 +44,10 @@
>   * 64bit interface.
>   */
>
> +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> +
>  static bool read_from_write_only(struct kvm_vcpu *vcpu,
>                                  struct sys_reg_params *params,
>                                  const struct sys_reg_desc *r)
> @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
>         return trap_raz_wi(vcpu, p, r);
>  }
>
> +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> +                          struct sys_reg_params *p,
> +                          const struct sys_reg_desc *r)
> +{
> +       u64 oslsr;
> +
> +       if (!p->is_write)
> +               return read_from_write_only(vcpu, p, r);
> +
> +       /* Forward the OSLK bit to OSLSR */
> +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> +       if (p->regval & SYS_OSLAR_OSLK)
> +               oslsr |= SYS_OSLSR_OSLK;
> +
> +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> +       return true;
> +}
> +
>  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>                            struct sys_reg_params *p,
>                            const struct sys_reg_desc *r)
> @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>         if (err)
>                 return err;
>
> -       if (val != rd->val)
> +       if ((val | SYS_OSLSR_OSLK) != rd->val)
>                 return -EINVAL;
>
> +       __vcpu_sys_reg(vcpu, rd->reg) = val;
>         return 0;
>  }
>
> @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
>         return __access_id_reg(vcpu, p, r, true);
>  }
>
> -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> -
>  /* Visibility overrides for SVE-specific control registers */
>  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>                                    const struct sys_reg_desc *rd)
> @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>         DBG_BCR_BVR_WCR_WVR_EL1(15),
>
>         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
>                 .set_user = set_oslsr_el1, },

Reviewed-by: Reiji Watanabe <reijiw@google.com>

I assume the reason why you changed the reset value for the
register is because Arm ARM says "the On a Cold reset,
this field resets to 1".

"4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
-------------------------------------------------------------
  - System registers: Reset to their architecturally defined
    values as for a warm reset to EL1 (resp. SVC)
-------------------------------------------------------------

Since Arm ARM doesn't say anything about a warm reset for the field,
I would guess the bit doesn't necessarily need to be set.

Thanks,
Reiji


>         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
>
>         DBGBXVR(0),
>         /* DBGOSLAR */
> -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
>         DBGBXVR(1),
>         /* DBGOSLSR */
>         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> --
> 2.33.1.1089.g2158813163f-goog
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> reset value of the OSLK bit to 1. Allow the value to be migrated by
> making OSLSR_EL1.OSLK writable from userspace.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
>  2 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index b268082d67ed..6ba4dc97b69d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -127,7 +127,13 @@
>  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
>  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
>  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> +
> +#define SYS_OSLAR_OSLK                 BIT(0)
> +
>  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> +
> +#define SYS_OSLSR_OSLK                 BIT(1)
> +
>  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
>  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
>  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0326b3df0736..acd8aa2e5a44 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -44,6 +44,10 @@
>   * 64bit interface.
>   */
>
> +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> +
>  static bool read_from_write_only(struct kvm_vcpu *vcpu,
>                                  struct sys_reg_params *params,
>                                  const struct sys_reg_desc *r)
> @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
>         return trap_raz_wi(vcpu, p, r);
>  }
>
> +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> +                          struct sys_reg_params *p,
> +                          const struct sys_reg_desc *r)
> +{
> +       u64 oslsr;
> +
> +       if (!p->is_write)
> +               return read_from_write_only(vcpu, p, r);
> +
> +       /* Forward the OSLK bit to OSLSR */
> +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> +       if (p->regval & SYS_OSLAR_OSLK)
> +               oslsr |= SYS_OSLSR_OSLK;
> +
> +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> +       return true;
> +}
> +
>  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
>                            struct sys_reg_params *p,
>                            const struct sys_reg_desc *r)
> @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>         if (err)
>                 return err;
>
> -       if (val != rd->val)
> +       if ((val | SYS_OSLSR_OSLK) != rd->val)
>                 return -EINVAL;
>
> +       __vcpu_sys_reg(vcpu, rd->reg) = val;
>         return 0;
>  }
>
> @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
>         return __access_id_reg(vcpu, p, r, true);
>  }
>
> -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> -
>  /* Visibility overrides for SVE-specific control registers */
>  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>                                    const struct sys_reg_desc *rd)
> @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>         DBG_BCR_BVR_WCR_WVR_EL1(15),
>
>         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
>                 .set_user = set_oslsr_el1, },

Reviewed-by: Reiji Watanabe <reijiw@google.com>

I assume the reason why you changed the reset value for the
register is because Arm ARM says "the On a Cold reset,
this field resets to 1".

"4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
-------------------------------------------------------------
  - System registers: Reset to their architecturally defined
    values as for a warm reset to EL1 (resp. SVC)
-------------------------------------------------------------

Since Arm ARM doesn't say anything about a warm reset for the field,
I would guess the bit doesn't necessarily need to be set.

Thanks,
Reiji


>         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
>         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
>
>         DBGBXVR(0),
>         /* DBGOSLAR */
> -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
>         DBGBXVR(1),
>         /* DBGOSLSR */
>         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> --
> 2.33.1.1089.g2158813163f-goog
>

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

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:51 AM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > An upcoming change to KVM will context switch the OS Lock status between
> > guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> > using the stored value.
> >
> > Wire up a custom handler for writes from userspace and prevent any of
> > the invariant bits from changing.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index f8be56d5342b..c98f65c4a1f7 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -172,6 +172,7 @@ enum vcpu_sysreg {
> >         MDSCR_EL1,      /* Monitor Debug System Control Register */
> >         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
> >         DISR_EL1,       /* Deferred Interrupt Status Register */
> > +       OSLSR_EL1,      /* OS Lock Status Register */
>
> Sorry Marc, forgot to move this up per your suggestion on the last
> series. Only caught it once the patch went out the door.

Except for the above,
Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:51 AM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > An upcoming change to KVM will context switch the OS Lock status between
> > guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> > using the stored value.
> >
> > Wire up a custom handler for writes from userspace and prevent any of
> > the invariant bits from changing.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index f8be56d5342b..c98f65c4a1f7 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -172,6 +172,7 @@ enum vcpu_sysreg {
> >         MDSCR_EL1,      /* Monitor Debug System Control Register */
> >         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
> >         DISR_EL1,       /* Deferred Interrupt Status Register */
> > +       OSLSR_EL1,      /* OS Lock Status Register */
>
> Sorry Marc, forgot to move this up per your suggestion on the last
> series. Only caught it once the patch went out the door.

Except for the above,
Reviewed-by: Reiji Watanabe <reijiw@google.com>

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

Re: [PATCH v2 2/6] KVM: arm64: Stash OSLSR_EL1 in the cpu context

[Reiji Watanabe]

On Tue, Nov 2, 2021 at 2:51 AM Oliver Upton <oupton@google.com> wrote:
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > An upcoming change to KVM will context switch the OS Lock status between
> > guest/host. Add OSLSR_EL1 to the cpu context and handle guest reads
> > using the stored value.
> >
> > Wire up a custom handler for writes from userspace and prevent any of
> > the invariant bits from changing.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/sys_regs.c         | 31 ++++++++++++++++++++++++-------
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index f8be56d5342b..c98f65c4a1f7 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -172,6 +172,7 @@ enum vcpu_sysreg {
> >         MDSCR_EL1,      /* Monitor Debug System Control Register */
> >         MDCCINT_EL1,    /* Monitor Debug Comms Channel Interrupt Enable Reg */
> >         DISR_EL1,       /* Deferred Interrupt Status Register */
> > +       OSLSR_EL1,      /* OS Lock Status Register */
>
> Sorry Marc, forgot to move this up per your suggestion on the last
> series. Only caught it once the patch went out the door.

Except for the above,
Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks,
Reiji

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

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Ricardo Koller]

On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > making OSLSR_EL1.OSLK writable from userspace.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> >  2 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index b268082d67ed..6ba4dc97b69d 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -127,7 +127,13 @@
> >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > +
> > +#define SYS_OSLAR_OSLK                 BIT(0)
> > +
> >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > +
> > +#define SYS_OSLSR_OSLK                 BIT(1)
> > +
> >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 0326b3df0736..acd8aa2e5a44 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -44,6 +44,10 @@
> >   * 64bit interface.
> >   */
> >
> > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > +
> >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> >                                  struct sys_reg_params *params,
> >                                  const struct sys_reg_desc *r)
> > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> >         return trap_raz_wi(vcpu, p, r);
> >  }
> >
> > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > +                          struct sys_reg_params *p,
> > +                          const struct sys_reg_desc *r)
> > +{
> > +       u64 oslsr;
> > +
> > +       if (!p->is_write)
> > +               return read_from_write_only(vcpu, p, r);
> > +
> > +       /* Forward the OSLK bit to OSLSR */
> > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > +       if (p->regval & SYS_OSLAR_OSLK)
> > +               oslsr |= SYS_OSLSR_OSLK;
> > +
> > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > +       return true;
> > +}
> > +
> >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> >                            struct sys_reg_params *p,
> >                            const struct sys_reg_desc *r)
> > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >         if (err)
> >                 return err;
> >
> > -       if (val != rd->val)
> > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> >                 return -EINVAL;
> >
> > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> >         return 0;
> >  }
> >
> > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> >         return __access_id_reg(vcpu, p, r, true);
> >  }
> >
> > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > -
> >  /* Visibility overrides for SVE-specific control registers */
> >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >                                    const struct sys_reg_desc *rd)
> > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> >
> >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> >                 .set_user = set_oslsr_el1, },
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> I assume the reason why you changed the reset value for the
> register is because Arm ARM says "the On a Cold reset,
> this field resets to 1".
> 
> "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> -------------------------------------------------------------
>   - System registers: Reset to their architecturally defined
>     values as for a warm reset to EL1 (resp. SVC)
> -------------------------------------------------------------
> 
> Since Arm ARM doesn't say anything about a warm reset for the field,
> I would guess the bit doesn't necessarily need to be set.

That would be great, because it would avoid the migration issue that
Oliver described in [PATCH v2 4/6]:

	There is an issue, though, with migration: older KVM will not show
	OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
	architectural OS Lock, its reset value must be 1 (enabled). This would
	all have the effect of discarding the guest's OS lock value and
	blocking all debug exceptions intended for the guest until the next
	reboot.

> 
> Thanks,
> Reiji
> 
> 
> >         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
> >         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> > @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
> >
> >         DBGBXVR(0),
> >         /* DBGOSLAR */
> > -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> > +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
> >         DBGBXVR(1),
> >         /* DBGOSLSR */
> >         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Ricardo Koller]

On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > making OSLSR_EL1.OSLK writable from userspace.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> >  2 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index b268082d67ed..6ba4dc97b69d 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -127,7 +127,13 @@
> >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > +
> > +#define SYS_OSLAR_OSLK                 BIT(0)
> > +
> >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > +
> > +#define SYS_OSLSR_OSLK                 BIT(1)
> > +
> >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 0326b3df0736..acd8aa2e5a44 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -44,6 +44,10 @@
> >   * 64bit interface.
> >   */
> >
> > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > +
> >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> >                                  struct sys_reg_params *params,
> >                                  const struct sys_reg_desc *r)
> > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> >         return trap_raz_wi(vcpu, p, r);
> >  }
> >
> > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > +                          struct sys_reg_params *p,
> > +                          const struct sys_reg_desc *r)
> > +{
> > +       u64 oslsr;
> > +
> > +       if (!p->is_write)
> > +               return read_from_write_only(vcpu, p, r);
> > +
> > +       /* Forward the OSLK bit to OSLSR */
> > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > +       if (p->regval & SYS_OSLAR_OSLK)
> > +               oslsr |= SYS_OSLSR_OSLK;
> > +
> > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > +       return true;
> > +}
> > +
> >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> >                            struct sys_reg_params *p,
> >                            const struct sys_reg_desc *r)
> > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >         if (err)
> >                 return err;
> >
> > -       if (val != rd->val)
> > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> >                 return -EINVAL;
> >
> > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> >         return 0;
> >  }
> >
> > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> >         return __access_id_reg(vcpu, p, r, true);
> >  }
> >
> > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > -
> >  /* Visibility overrides for SVE-specific control registers */
> >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >                                    const struct sys_reg_desc *rd)
> > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> >
> >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> >                 .set_user = set_oslsr_el1, },
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> I assume the reason why you changed the reset value for the
> register is because Arm ARM says "the On a Cold reset,
> this field resets to 1".
> 
> "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> -------------------------------------------------------------
>   - System registers: Reset to their architecturally defined
>     values as for a warm reset to EL1 (resp. SVC)
> -------------------------------------------------------------
> 
> Since Arm ARM doesn't say anything about a warm reset for the field,
> I would guess the bit doesn't necessarily need to be set.

That would be great, because it would avoid the migration issue that
Oliver described in [PATCH v2 4/6]:

	There is an issue, though, with migration: older KVM will not show
	OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
	architectural OS Lock, its reset value must be 1 (enabled). This would
	all have the effect of discarding the guest's OS lock value and
	blocking all debug exceptions intended for the guest until the next
	reboot.

> 
> Thanks,
> Reiji
> 
> 
> >         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
> >         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> > @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
> >
> >         DBGBXVR(0),
> >         /* DBGOSLAR */
> > -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> > +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
> >         DBGBXVR(1),
> >         /* DBGOSLSR */
> >         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Ricardo Koller]

On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > making OSLSR_EL1.OSLK writable from userspace.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> >  2 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index b268082d67ed..6ba4dc97b69d 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -127,7 +127,13 @@
> >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > +
> > +#define SYS_OSLAR_OSLK                 BIT(0)
> > +
> >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > +
> > +#define SYS_OSLSR_OSLK                 BIT(1)
> > +
> >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 0326b3df0736..acd8aa2e5a44 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -44,6 +44,10 @@
> >   * 64bit interface.
> >   */
> >
> > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > +
> >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> >                                  struct sys_reg_params *params,
> >                                  const struct sys_reg_desc *r)
> > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> >         return trap_raz_wi(vcpu, p, r);
> >  }
> >
> > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > +                          struct sys_reg_params *p,
> > +                          const struct sys_reg_desc *r)
> > +{
> > +       u64 oslsr;
> > +
> > +       if (!p->is_write)
> > +               return read_from_write_only(vcpu, p, r);
> > +
> > +       /* Forward the OSLK bit to OSLSR */
> > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > +       if (p->regval & SYS_OSLAR_OSLK)
> > +               oslsr |= SYS_OSLSR_OSLK;
> > +
> > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > +       return true;
> > +}
> > +
> >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> >                            struct sys_reg_params *p,
> >                            const struct sys_reg_desc *r)
> > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> >         if (err)
> >                 return err;
> >
> > -       if (val != rd->val)
> > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> >                 return -EINVAL;
> >
> > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> >         return 0;
> >  }
> >
> > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> >         return __access_id_reg(vcpu, p, r, true);
> >  }
> >
> > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > -
> >  /* Visibility overrides for SVE-specific control registers */
> >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >                                    const struct sys_reg_desc *rd)
> > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> >
> >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> >                 .set_user = set_oslsr_el1, },
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> 
> I assume the reason why you changed the reset value for the
> register is because Arm ARM says "the On a Cold reset,
> this field resets to 1".
> 
> "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> -------------------------------------------------------------
>   - System registers: Reset to their architecturally defined
>     values as for a warm reset to EL1 (resp. SVC)
> -------------------------------------------------------------
> 
> Since Arm ARM doesn't say anything about a warm reset for the field,
> I would guess the bit doesn't necessarily need to be set.

That would be great, because it would avoid the migration issue that
Oliver described in [PATCH v2 4/6]:

	There is an issue, though, with migration: older KVM will not show
	OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an
	architectural OS Lock, its reset value must be 1 (enabled). This would
	all have the effect of discarding the guest's OS lock value and
	blocking all debug exceptions intended for the guest until the next
	reboot.

> 
> Thanks,
> Reiji
> 
> 
> >         { SYS_DESC(SYS_OSDLR_EL1), trap_raz_wi },
> >         { SYS_DESC(SYS_DBGPRCR_EL1), trap_raz_wi },
> > @@ -1930,7 +1949,7 @@ static const struct sys_reg_desc cp14_regs[] = {
> >
> >         DBGBXVR(0),
> >         /* DBGOSLAR */
> > -       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_raz_wi },
> > +       { Op1( 0), CRn( 1), CRm( 0), Op2( 4), trap_oslar_el1 },
> >         DBGBXVR(1),
> >         /* DBGOSLSR */
> >         { Op1( 0), CRn( 1), CRm( 1), Op2( 4), trap_oslsr_el1, NULL, OSLSR_EL1 },
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

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

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

On Wed, Nov 03, 2021 at 08:47:42PM -0700, Ricardo Koller wrote:
> On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> > On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > > making OSLSR_EL1.OSLK writable from userspace.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> > >  2 files changed, 33 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index b268082d67ed..6ba4dc97b69d 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -127,7 +127,13 @@
> > >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> > >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> > >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > > +
> > > +#define SYS_OSLAR_OSLK                 BIT(0)
> > > +
> > >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > > +
> > > +#define SYS_OSLSR_OSLK                 BIT(1)
> > > +
> > >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> > >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> > >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 0326b3df0736..acd8aa2e5a44 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -44,6 +44,10 @@
> > >   * 64bit interface.
> > >   */
> > >
> > > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > +
> > >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> > >                                  struct sys_reg_params *params,
> > >                                  const struct sys_reg_desc *r)
> > > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> > >         return trap_raz_wi(vcpu, p, r);
> > >  }
> > >
> > > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > > +                          struct sys_reg_params *p,
> > > +                          const struct sys_reg_desc *r)
> > > +{
> > > +       u64 oslsr;
> > > +
> > > +       if (!p->is_write)
> > > +               return read_from_write_only(vcpu, p, r);
> > > +
> > > +       /* Forward the OSLK bit to OSLSR */
> > > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > > +       if (p->regval & SYS_OSLAR_OSLK)
> > > +               oslsr |= SYS_OSLSR_OSLK;
> > > +
> > > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > > +       return true;
> > > +}
> > > +
> > >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> > >                            struct sys_reg_params *p,
> > >                            const struct sys_reg_desc *r)
> > > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >         if (err)
> > >                 return err;
> > >
> > > -       if (val != rd->val)
> > > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> > >                 return -EINVAL;
> > >
> > > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> > >         return 0;
> > >  }
> > >
> > > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > >         return __access_id_reg(vcpu, p, r, true);
> > >  }
> > >
> > > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > -
> > >  /* Visibility overrides for SVE-specific control registers */
> > >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >                                    const struct sys_reg_desc *rd)
> > > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> > >
> > >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> > >                 .set_user = set_oslsr_el1, },
> > 
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > 
> > I assume the reason why you changed the reset value for the
> > register is because Arm ARM says "the On a Cold reset,
> > this field resets to 1".
> > 
> > "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> > -------------------------------------------------------------
> >   - System registers: Reset to their architecturally defined
> >     values as for a warm reset to EL1 (resp. SVC)
> > -------------------------------------------------------------
> > 
> > Since Arm ARM doesn't say anything about a warm reset for the field,
> > I would guess the bit doesn't necessarily need to be set.
> 
> That would be great, because it would avoid the migration issue that
> Oliver described in [PATCH v2 4/6]:

Yeah, awesome! Means I can be even lazier and things will "Just Work"
:-)

--
Oliver

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

On Wed, Nov 03, 2021 at 08:47:42PM -0700, Ricardo Koller wrote:
> On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> > On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > > making OSLSR_EL1.OSLK writable from userspace.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> > >  2 files changed, 33 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index b268082d67ed..6ba4dc97b69d 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -127,7 +127,13 @@
> > >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> > >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> > >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > > +
> > > +#define SYS_OSLAR_OSLK                 BIT(0)
> > > +
> > >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > > +
> > > +#define SYS_OSLSR_OSLK                 BIT(1)
> > > +
> > >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> > >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> > >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 0326b3df0736..acd8aa2e5a44 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -44,6 +44,10 @@
> > >   * 64bit interface.
> > >   */
> > >
> > > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > +
> > >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> > >                                  struct sys_reg_params *params,
> > >                                  const struct sys_reg_desc *r)
> > > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> > >         return trap_raz_wi(vcpu, p, r);
> > >  }
> > >
> > > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > > +                          struct sys_reg_params *p,
> > > +                          const struct sys_reg_desc *r)
> > > +{
> > > +       u64 oslsr;
> > > +
> > > +       if (!p->is_write)
> > > +               return read_from_write_only(vcpu, p, r);
> > > +
> > > +       /* Forward the OSLK bit to OSLSR */
> > > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > > +       if (p->regval & SYS_OSLAR_OSLK)
> > > +               oslsr |= SYS_OSLSR_OSLK;
> > > +
> > > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > > +       return true;
> > > +}
> > > +
> > >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> > >                            struct sys_reg_params *p,
> > >                            const struct sys_reg_desc *r)
> > > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >         if (err)
> > >                 return err;
> > >
> > > -       if (val != rd->val)
> > > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> > >                 return -EINVAL;
> > >
> > > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> > >         return 0;
> > >  }
> > >
> > > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > >         return __access_id_reg(vcpu, p, r, true);
> > >  }
> > >
> > > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > -
> > >  /* Visibility overrides for SVE-specific control registers */
> > >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >                                    const struct sys_reg_desc *rd)
> > > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> > >
> > >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> > >                 .set_user = set_oslsr_el1, },
> > 
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > 
> > I assume the reason why you changed the reset value for the
> > register is because Arm ARM says "the On a Cold reset,
> > this field resets to 1".
> > 
> > "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> > -------------------------------------------------------------
> >   - System registers: Reset to their architecturally defined
> >     values as for a warm reset to EL1 (resp. SVC)
> > -------------------------------------------------------------
> > 
> > Since Arm ARM doesn't say anything about a warm reset for the field,
> > I would guess the bit doesn't necessarily need to be set.
> 
> That would be great, because it would avoid the migration issue that
> Oliver described in [PATCH v2 4/6]:

Yeah, awesome! Means I can be even lazier and things will "Just Work"
:-)

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

Re: [PATCH v2 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Oliver Upton]

On Wed, Nov 03, 2021 at 08:47:42PM -0700, Ricardo Koller wrote:
> On Wed, Nov 03, 2021 at 08:31:35PM -0700, Reiji Watanabe wrote:
> > On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Change the
> > > reset value of the OSLK bit to 1. Allow the value to be migrated by
> > > making OSLSR_EL1.OSLK writable from userspace.
> > >
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 35 +++++++++++++++++++++++++--------
> > >  2 files changed, 33 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index b268082d67ed..6ba4dc97b69d 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -127,7 +127,13 @@
> > >  #define SYS_DBGWCRn_EL1(n)             sys_reg(2, 0, 0, n, 7)
> > >  #define SYS_MDRAR_EL1                  sys_reg(2, 0, 1, 0, 0)
> > >  #define SYS_OSLAR_EL1                  sys_reg(2, 0, 1, 0, 4)
> > > +
> > > +#define SYS_OSLAR_OSLK                 BIT(0)
> > > +
> > >  #define SYS_OSLSR_EL1                  sys_reg(2, 0, 1, 1, 4)
> > > +
> > > +#define SYS_OSLSR_OSLK                 BIT(1)
> > > +
> > >  #define SYS_OSDLR_EL1                  sys_reg(2, 0, 1, 3, 4)
> > >  #define SYS_DBGPRCR_EL1                        sys_reg(2, 0, 1, 4, 4)
> > >  #define SYS_DBGCLAIMSET_EL1            sys_reg(2, 0, 7, 8, 6)
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 0326b3df0736..acd8aa2e5a44 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -44,6 +44,10 @@
> > >   * 64bit interface.
> > >   */
> > >
> > > +static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > +static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > +static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > +
> > >  static bool read_from_write_only(struct kvm_vcpu *vcpu,
> > >                                  struct sys_reg_params *params,
> > >                                  const struct sys_reg_desc *r)
> > > @@ -287,6 +291,24 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
> > >         return trap_raz_wi(vcpu, p, r);
> > >  }
> > >
> > > +static bool trap_oslar_el1(struct kvm_vcpu *vcpu,
> > > +                          struct sys_reg_params *p,
> > > +                          const struct sys_reg_desc *r)
> > > +{
> > > +       u64 oslsr;
> > > +
> > > +       if (!p->is_write)
> > > +               return read_from_write_only(vcpu, p, r);
> > > +
> > > +       /* Forward the OSLK bit to OSLSR */
> > > +       oslsr = __vcpu_sys_reg(vcpu, OSLSR_EL1) & ~SYS_OSLSR_OSLK;
> > > +       if (p->regval & SYS_OSLAR_OSLK)
> > > +               oslsr |= SYS_OSLSR_OSLK;
> > > +
> > > +       __vcpu_sys_reg(vcpu, OSLSR_EL1) = oslsr;
> > > +       return true;
> > > +}
> > > +
> > >  static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> > >                            struct sys_reg_params *p,
> > >                            const struct sys_reg_desc *r)
> > > @@ -309,9 +331,10 @@ static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > >         if (err)
> > >                 return err;
> > >
> > > -       if (val != rd->val)
> > > +       if ((val | SYS_OSLSR_OSLK) != rd->val)
> > >                 return -EINVAL;
> > >
> > > +       __vcpu_sys_reg(vcpu, rd->reg) = val;
> > >         return 0;
> > >  }
> > >
> > > @@ -1176,10 +1199,6 @@ static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > >         return __access_id_reg(vcpu, p, r, true);
> > >  }
> > >
> > > -static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
> > > -static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
> > > -static u64 sys_reg_to_index(const struct sys_reg_desc *reg);
> > > -
> > >  /* Visibility overrides for SVE-specific control registers */
> > >  static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >                                    const struct sys_reg_desc *rd)
> > > @@ -1456,8 +1475,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > >         DBG_BCR_BVR_WCR_WVR_EL1(15),
> > >
> > >         { SYS_DESC(SYS_MDRAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLAR_EL1), trap_raz_wi },
> > > -       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x00000008,
> > > +       { SYS_DESC(SYS_OSLAR_EL1), trap_oslar_el1 },
> > > +       { SYS_DESC(SYS_OSLSR_EL1), trap_oslsr_el1, reset_val, OSLSR_EL1, 0x0000000A,
> > >                 .set_user = set_oslsr_el1, },
> > 
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > 
> > I assume the reason why you changed the reset value for the
> > register is because Arm ARM says "the On a Cold reset,
> > this field resets to 1".
> > 
> > "4.82 KVM_ARM_VCPU_INIT" in Documentation/virt/kvm/api.rst says:
> > -------------------------------------------------------------
> >   - System registers: Reset to their architecturally defined
> >     values as for a warm reset to EL1 (resp. SVC)
> > -------------------------------------------------------------
> > 
> > Since Arm ARM doesn't say anything about a warm reset for the field,
> > I would guess the bit doesn't necessarily need to be set.
> 
> That would be great, because it would avoid the migration issue that
> Oliver described in [PATCH v2 4/6]:

Yeah, awesome! Means I can be even lazier and things will "Just Work"
:-)

--
Oliver

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Reiji Watanabe]

Hi Oliver,

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
>
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)

I would think the name of this macro might sound like it generates
a code that is evaluated as bool :)


> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>                                struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>                                 MDCR_EL2_TDRA |
>                                 MDCR_EL2_TDOSA);
>
> -       /* Is the VM being debugged by userspace? */
> -       if (vcpu->guest_debug)
> +       /*
> +        * Check if the VM is being debugged by userspace or the guest has
> +        * enabled the OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))

IMHO, it might be nicer to create a macro or function that abstracts the
condition that needs save_guest_debug_regs/restore_guest_debug_regs.
(rather than putting those conditions in each part of codes where they
are needed)

Thanks,
Reiji




>                 /* Route all software debug exceptions to EL2 */
>                 vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>
>         kvm_arm_setup_mdcr_el2(vcpu);
>
> -       /* Is Guest debugging in effect? */
> -       if (vcpu->guest_debug) {
> +       /*
> +        * Check if the guest is being debugged or if the guest has enabled the
> +        * OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 /* Save guest debug state */
>                 save_guest_debug_regs(vcpu);
>
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>                         trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>                                                 &vcpu->arch.debug_ptr->dbg_wcr[0],
>                                                 &vcpu->arch.debug_ptr->dbg_wvr[0]);
> +               } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +                       mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +                       mdscr &= ~DBG_MDSCR_MDE;
> +                       vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>                 }
>         }
>
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>         trace_kvm_arm_clear_debug(vcpu->guest_debug);
>
> -       if (vcpu->guest_debug) {
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 restore_guest_debug_regs(vcpu);
>
>                 /*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_run *run = vcpu->run;
>         u32 esr = kvm_vcpu_get_esr(vcpu);
> +       u8 esr_ec = ESR_ELx_EC(esr);
> +
> +       if (!vcpu->guest_debug) {
> +               WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +                         "Unexpected debug exception\n");
> +               kvm_incr_pc(vcpu);
> +               return 1;
> +       }
>
>         run->exit_reason = KVM_EXIT_DEBUG;
>         run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>         { SYS_DESC(SYS_DC_ISW), access_dcsw },
> --
> 2.33.1.1089.g2158813163f-goog
>

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Reiji Watanabe]

Hi Oliver,

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
>
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)

I would think the name of this macro might sound like it generates
a code that is evaluated as bool :)


> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>                                struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>                                 MDCR_EL2_TDRA |
>                                 MDCR_EL2_TDOSA);
>
> -       /* Is the VM being debugged by userspace? */
> -       if (vcpu->guest_debug)
> +       /*
> +        * Check if the VM is being debugged by userspace or the guest has
> +        * enabled the OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))

IMHO, it might be nicer to create a macro or function that abstracts the
condition that needs save_guest_debug_regs/restore_guest_debug_regs.
(rather than putting those conditions in each part of codes where they
are needed)

Thanks,
Reiji




>                 /* Route all software debug exceptions to EL2 */
>                 vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>
>         kvm_arm_setup_mdcr_el2(vcpu);
>
> -       /* Is Guest debugging in effect? */
> -       if (vcpu->guest_debug) {
> +       /*
> +        * Check if the guest is being debugged or if the guest has enabled the
> +        * OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 /* Save guest debug state */
>                 save_guest_debug_regs(vcpu);
>
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>                         trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>                                                 &vcpu->arch.debug_ptr->dbg_wcr[0],
>                                                 &vcpu->arch.debug_ptr->dbg_wvr[0]);
> +               } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +                       mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +                       mdscr &= ~DBG_MDSCR_MDE;
> +                       vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>                 }
>         }
>
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>         trace_kvm_arm_clear_debug(vcpu->guest_debug);
>
> -       if (vcpu->guest_debug) {
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 restore_guest_debug_regs(vcpu);
>
>                 /*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_run *run = vcpu->run;
>         u32 esr = kvm_vcpu_get_esr(vcpu);
> +       u8 esr_ec = ESR_ELx_EC(esr);
> +
> +       if (!vcpu->guest_debug) {
> +               WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +                         "Unexpected debug exception\n");
> +               kvm_incr_pc(vcpu);
> +               return 1;
> +       }
>
>         run->exit_reason = KVM_EXIT_DEBUG;
>         run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>         { SYS_DESC(SYS_DC_ISW), access_dcsw },
> --
> 2.33.1.1089.g2158813163f-goog
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Reiji Watanabe]

Hi Oliver,

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
>
> The OS lock blocks all debug exceptions at every EL. To date, KVM has
> not implemented the OS lock for its guests, despite the fact that it is
> mandatory per the architecture. Simple context switching between the
> guest and host is not appropriate, as its effects are not constrained to
> the guest context.
>
> Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> blocking all but software breakpoint instructions. To handle breakpoint
> instructions, trap debug exceptions to EL2 and skip the instruction.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
>  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  4 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index c98f65c4a1f7..f13b8b79b06d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
>  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> +
> +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)

I would think the name of this macro might sound like it generates
a code that is evaluated as bool :)


> +
>  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>                                struct kvm_device_attr *attr);
>  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index db9361338b2a..5690a9c99c89 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>                                 MDCR_EL2_TDRA |
>                                 MDCR_EL2_TDOSA);
>
> -       /* Is the VM being debugged by userspace? */
> -       if (vcpu->guest_debug)
> +       /*
> +        * Check if the VM is being debugged by userspace or the guest has
> +        * enabled the OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))

IMHO, it might be nicer to create a macro or function that abstracts the
condition that needs save_guest_debug_regs/restore_guest_debug_regs.
(rather than putting those conditions in each part of codes where they
are needed)

Thanks,
Reiji




>                 /* Route all software debug exceptions to EL2 */
>                 vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>
> @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>
>         kvm_arm_setup_mdcr_el2(vcpu);
>
> -       /* Is Guest debugging in effect? */
> -       if (vcpu->guest_debug) {
> +       /*
> +        * Check if the guest is being debugged or if the guest has enabled the
> +        * OS lock.
> +        */
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 /* Save guest debug state */
>                 save_guest_debug_regs(vcpu);
>
> @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>                         trace_kvm_arm_set_regset("WAPTS", get_num_wrps(),
>                                                 &vcpu->arch.debug_ptr->dbg_wcr[0],
>                                                 &vcpu->arch.debug_ptr->dbg_wvr[0]);
> +               } else if (kvm_vcpu_os_lock_enabled(vcpu)) {
> +                       mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> +                       mdscr &= ~DBG_MDSCR_MDE;
> +                       vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>                 }
>         }
>
> @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>         trace_kvm_arm_clear_debug(vcpu->guest_debug);
>
> -       if (vcpu->guest_debug) {
> +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>                 restore_guest_debug_regs(vcpu);
>
>                 /*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
>  {
>         struct kvm_run *run = vcpu->run;
>         u32 esr = kvm_vcpu_get_esr(vcpu);
> +       u8 esr_ec = ESR_ELx_EC(esr);
> +
> +       if (!vcpu->guest_debug) {
> +               WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> +                         "Unexpected debug exception\n");
> +               kvm_incr_pc(vcpu);
> +               return 1;
> +       }
>
>         run->exit_reason = KVM_EXIT_DEBUG;
>         run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu,
>   * Debug handling: We do trap most, if not all debug related system
>   * registers. The implementation is good enough to ensure that a guest
>   * can use these with minimal performance degradation. The drawback is
> - * that we don't implement any of the external debug, none of the
> - * OSlock protocol. This should be revisited if we ever encounter a
> - * more demanding guest...
> + * that we don't implement any of the external debug architecture.
> + * This should be revisited if we ever encounter a more demanding
> + * guest...
>   */
>  static const struct sys_reg_desc sys_reg_descs[] = {
>         { SYS_DESC(SYS_DC_ISW), access_dcsw },
> --
> 2.33.1.1089.g2158813163f-goog
>

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Oliver,
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> > +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
>
> I would think the name of this macro might sound like it generates
> a code that is evaluated as bool :)

Hey! Nobody ever said this would coerce the returned value into a bool :-P

In all seriousness, good point. I agree that the statement should
obviously evaluate to a bool, given the naming of the macro.

>
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                                struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                                 MDCR_EL2_TDRA |
> >                                 MDCR_EL2_TDOSA);
> >
> > -       /* Is the VM being debugged by userspace? */
> > -       if (vcpu->guest_debug)
> > +       /*
> > +        * Check if the VM is being debugged by userspace or the guest has
> > +        * enabled the OS lock.
> > +        */
> > +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>
> IMHO, it might be nicer to create a macro or function that abstracts the
> condition that needs save_guest_debug_regs/restore_guest_debug_regs.
> (rather than putting those conditions in each part of codes where they
> are needed)
>

I completely agree, and it comes with the added benefit that the
macro/function can be named something informative so as to suggest the
purpose for saving guest registers.

Thanks for the review!

--
Oliver

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Oliver,
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> > +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
>
> I would think the name of this macro might sound like it generates
> a code that is evaluated as bool :)

Hey! Nobody ever said this would coerce the returned value into a bool :-P

In all seriousness, good point. I agree that the statement should
obviously evaluate to a bool, given the naming of the macro.

>
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                                struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                                 MDCR_EL2_TDRA |
> >                                 MDCR_EL2_TDOSA);
> >
> > -       /* Is the VM being debugged by userspace? */
> > -       if (vcpu->guest_debug)
> > +       /*
> > +        * Check if the VM is being debugged by userspace or the guest has
> > +        * enabled the OS lock.
> > +        */
> > +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>
> IMHO, it might be nicer to create a macro or function that abstracts the
> condition that needs save_guest_debug_regs/restore_guest_debug_regs.
> (rather than putting those conditions in each part of codes where they
> are needed)
>

I completely agree, and it comes with the added benefit that the
macro/function can be named something informative so as to suggest the
purpose for saving guest registers.

Thanks for the review!

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

Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Oliver,
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote:
> >
> > The OS lock blocks all debug exceptions at every EL. To date, KVM has
> > not implemented the OS lock for its guests, despite the fact that it is
> > mandatory per the architecture. Simple context switching between the
> > guest and host is not appropriate, as its effects are not constrained to
> > the guest context.
> >
> > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby
> > blocking all but software breakpoint instructions. To handle breakpoint
> > instructions, trap debug exceptions to EL2 and skip the instruction.
> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 20 +++++++++++++++-----
> >  arch/arm64/kvm/handle_exit.c      |  8 ++++++++
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  4 files changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index c98f65c4a1f7..f13b8b79b06d 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu);
> >  void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu);
> > +
> > +#define kvm_vcpu_os_lock_enabled(vcpu)         \
> > +       (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK)
>
> I would think the name of this macro might sound like it generates
> a code that is evaluated as bool :)

Hey! Nobody ever said this would coerce the returned value into a bool :-P

In all seriousness, good point. I agree that the statement should
obviously evaluate to a bool, given the naming of the macro.

>
> > +
> >  int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >                                struct kvm_device_attr *attr);
> >  int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index db9361338b2a..5690a9c99c89 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >                                 MDCR_EL2_TDRA |
> >                                 MDCR_EL2_TDOSA);
> >
> > -       /* Is the VM being debugged by userspace? */
> > -       if (vcpu->guest_debug)
> > +       /*
> > +        * Check if the VM is being debugged by userspace or the guest has
> > +        * enabled the OS lock.
> > +        */
> > +       if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu))
>
> IMHO, it might be nicer to create a macro or function that abstracts the
> condition that needs save_guest_debug_regs/restore_guest_debug_regs.
> (rather than putting those conditions in each part of codes where they
> are needed)
>

I completely agree, and it comes with the added benefit that the
macro/function can be named something informative so as to suggest the
purpose for saving guest registers.

Thanks for the review!

--
Oliver

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