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

[PATCH v3 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 (except breakpoint instructions) from the perspective
of the guest. This is done by disabling MDE and SS in MDSCR_EL1.

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.16-rc2. Tested on an Ampere Altra
machine with the included selftests patches.

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             |  6 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 27 +++++--
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 6 files changed, 145 insertions(+), 23 deletions(-)

-- 
2.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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 (except breakpoint instructions) from the perspective
of the guest. This is done by disabling MDE and SS in MDSCR_EL1.

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.16-rc2. Tested on an Ampere Altra
machine with the included selftests patches.

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             |  6 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 27 +++++--
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 6 files changed, 145 insertions(+), 23 deletions(-)

-- 
2.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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 (except breakpoint instructions) from the perspective
of the guest. This is done by disabling MDE and SS in MDSCR_EL1.

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.16-rc2. Tested on an Ampere Altra
machine with the included selftests patches.

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             |  6 ++
 arch/arm64/include/asm/sysreg.h               |  6 ++
 arch/arm64/kvm/debug.c                        | 27 +++++--
 arch/arm64/kvm/sys_regs.c                     | 70 ++++++++++++++-----
 .../selftests/kvm/aarch64/debug-exceptions.c  | 58 ++++++++++++++-
 .../selftests/kvm/aarch64/get-reg-list.c      |  1 +
 6 files changed, 145 insertions(+), 23 deletions(-)

-- 
2.34.0.rc2.393.gf8c9666880-goog


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

[PATCH v3 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.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
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 e3ec1a44f94d..11b4212c2036 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.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
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 e3ec1a44f94d..11b4212c2036 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.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
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 e3ec1a44f94d..11b4212c2036 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.34.0.rc2.393.gf8c9666880-goog


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

[PATCH v3 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.

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2a5f7f38006f..53fc8a6eaf1c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,8 +172,10 @@ enum vcpu_sysreg {
 	PAR_EL1,	/* Physical Address Register */
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
+	OSLSR_EL1,	/* OS Lock Status Register */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
 
+
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
 	PMSELR_EL0,	/* Event Counter Selection Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 11b4212c2036..7bf350b3d9cd 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,
@@ -1448,7 +1464,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 },
@@ -1923,7 +1940,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.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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.

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2a5f7f38006f..53fc8a6eaf1c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,8 +172,10 @@ enum vcpu_sysreg {
 	PAR_EL1,	/* Physical Address Register */
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
+	OSLSR_EL1,	/* OS Lock Status Register */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
 
+
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
 	PMSELR_EL0,	/* Event Counter Selection Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 11b4212c2036..7bf350b3d9cd 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,
@@ -1448,7 +1464,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 },
@@ -1923,7 +1940,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.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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.

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 2a5f7f38006f..53fc8a6eaf1c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -172,8 +172,10 @@ enum vcpu_sysreg {
 	PAR_EL1,	/* Physical Address Register */
 	MDSCR_EL1,	/* Monitor Debug System Control Register */
 	MDCCINT_EL1,	/* Monitor Debug Comms Channel Interrupt Enable Reg */
+	OSLSR_EL1,	/* OS Lock Status Register */
 	DISR_EL1,	/* Deferred Interrupt Status Register */
 
+
 	/* Performance Monitors Registers */
 	PMCR_EL0,	/* Control Register */
 	PMSELR_EL0,	/* Event Counter Selection Register */
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 11b4212c2036..7bf350b3d9cd 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,
@@ -1448,7 +1464,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 },
@@ -1923,7 +1940,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.34.0.rc2.393.gf8c9666880-goog


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

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

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
the value for now.

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

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16b3f1a1d468..9fad61a82047 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
 }
 
@@ -1180,10 +1203,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)
@@ -1463,7 +1482,7 @@ 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_OSLAR_EL1), trap_oslar_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 },
@@ -1937,7 +1956,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.34.0.rc2.393.gf8c9666880-goog

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

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
the value for now.

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

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16b3f1a1d468..9fad61a82047 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
 }
 
@@ -1180,10 +1203,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)
@@ -1463,7 +1482,7 @@ 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_OSLAR_EL1), trap_oslar_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 },
@@ -1937,7 +1956,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.34.0.rc2.393.gf8c9666880-goog

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

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

[Oliver Upton]

Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
the value for now.

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

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 16b3f1a1d468..9fad61a82047 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
 }
 
@@ -1180,10 +1203,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)
@@ -1463,7 +1482,7 @@ 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_OSLAR_EL1), trap_oslar_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 },
@@ -1937,7 +1956,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.34.0.rc2.393.gf8c9666880-goog


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

[PATCH v3 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            | 27 +++++++++++++++++++++++----
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 53fc8a6eaf1c..e5a06ff1cba6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -726,6 +726,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..7835c76347ce 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
 }
 
+/*
+ * Returns true if the host needs to use the debug registers.
+ */
+static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
+{
+	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
+}
+
 /**
  * kvm_arm_init_debug - grab what we need for debug
  *
@@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 	 *  - Userspace is using the hardware to debug the guest
 	 *  (KVM_GUESTDBG_USE_HW is set).
 	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
+	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
 	 */
 	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
-	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
+	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
+	    kvm_vcpu_os_lock_enabled(vcpu))
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 
 	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
@@ -160,8 +170,10 @@ 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 we need to use the debug registers.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	/*
+	 * Restore the guest's debug registers if we were using them.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5dbdb45d6d44..1346906f5c46 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1453,9 +1453,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.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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            | 27 +++++++++++++++++++++++----
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 53fc8a6eaf1c..e5a06ff1cba6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -726,6 +726,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..7835c76347ce 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
 }
 
+/*
+ * Returns true if the host needs to use the debug registers.
+ */
+static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
+{
+	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
+}
+
 /**
  * kvm_arm_init_debug - grab what we need for debug
  *
@@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 	 *  - Userspace is using the hardware to debug the guest
 	 *  (KVM_GUESTDBG_USE_HW is set).
 	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
+	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
 	 */
 	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
-	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
+	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
+	    kvm_vcpu_os_lock_enabled(vcpu))
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 
 	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
@@ -160,8 +170,10 @@ 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 we need to use the debug registers.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	/*
+	 * Restore the guest's debug registers if we were using them.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5dbdb45d6d44..1346906f5c46 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1453,9 +1453,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.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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            | 27 +++++++++++++++++++++++----
 arch/arm64/kvm/sys_regs.c         |  6 +++---
 3 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 53fc8a6eaf1c..e5a06ff1cba6 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -726,6 +726,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..7835c76347ce 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
 				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
 }
 
+/*
+ * Returns true if the host needs to use the debug registers.
+ */
+static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
+{
+	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
+}
+
 /**
  * kvm_arm_init_debug - grab what we need for debug
  *
@@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
 	 *  - Userspace is using the hardware to debug the guest
 	 *  (KVM_GUESTDBG_USE_HW is set).
 	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
+	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
 	 */
 	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
-	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
+	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
+	    kvm_vcpu_os_lock_enabled(vcpu))
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 
 	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
@@ -160,8 +170,10 @@ 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 we need to use the debug registers.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		/* Save guest debug state */
 		save_guest_debug_regs(vcpu);
 
@@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
 {
 	trace_kvm_arm_clear_debug(vcpu->guest_debug);
 
-	if (vcpu->guest_debug) {
+	/*
+	 * Restore the guest's debug registers if we were using them.
+	 */
+	if (host_using_debug_regs(vcpu)) {
 		restore_guest_debug_regs(vcpu);
 
 		/*
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 5dbdb45d6d44..1346906f5c46 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1453,9 +1453,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.34.0.rc2.393.gf8c9666880-goog


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

[PATCH v3 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.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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.34.0.rc2.393.gf8c9666880-goog


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

[PATCH v3 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 ea189d83abf7..63b2178210c4 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, sw_bp2, 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(1, oslar_el1);
+	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 does not block software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("sw_bp2: brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp2));
+
+	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.34.0.rc2.393.gf8c9666880-goog

[PATCH v3 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 ea189d83abf7..63b2178210c4 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, sw_bp2, 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(1, oslar_el1);
+	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 does not block software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("sw_bp2: brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp2));
+
+	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.34.0.rc2.393.gf8c9666880-goog

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

[PATCH v3 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 ea189d83abf7..63b2178210c4 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, sw_bp2, 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(1, oslar_el1);
+	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 does not block software-breakpoint */
+	reset_debug_state();
+	enable_os_lock();
+	sw_bp_addr = 0;
+	asm volatile("sw_bp2: brk #0");
+	GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp2));
+
+	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.34.0.rc2.393.gf8c9666880-goog


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

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

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:06 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> the value for now.
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..9fad61a82047 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;

This looks odd. It means that once I have set the lock from userspace,
I can't clear it?

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 v3 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:06 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> the value for now.
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..9fad61a82047 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;

This looks odd. It means that once I have set the lock from userspace,
I can't clear it?

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 v3 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:06 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> the value for now.
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/sysreg.h |  6 ++++++
>  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 16b3f1a1d468..9fad61a82047 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;

This looks odd. It means that once I have set the lock from userspace,
I can't clear it?

Thanks,

	M.

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

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

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:07 +0000,
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.

Skipping breakpoint instructions? I don't think you can do that, as
the guest does rely on BRK always being effective. I also don't see
where you do that...

> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 53fc8a6eaf1c..e5a06ff1cba6 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -726,6 +726,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..7835c76347ce 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>  }
>  
> +/*
> + * Returns true if the host needs to use the debug registers.
> + */
> +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);

Just the name of the function has sent my head spinning. Even if the
*effects* of the host debug and the OS Lock are vaguely similar from
the guest PoV, they really are different things, and I'd rather not
lob them together.

> +}
> +
>  /**
>   * kvm_arm_init_debug - grab what we need for debug
>   *
> @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	 *  - Userspace is using the hardware to debug the guest
>  	 *  (KVM_GUESTDBG_USE_HW is set).
>  	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> +	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
>  	 */
>  	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> -	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> +	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> +	    kvm_vcpu_os_lock_enabled(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  
>  	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {

I'd rather you expand the helper here and add the comment you have in
the commit message explaining the machine-wide effect of the OS Lock.

>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Restore the guest's debug registers if we were using them.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5dbdb45d6d44..1346906f5c46 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1453,9 +1453,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 },

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 v3 4/6] KVM: arm64: Emulate the OS Lock

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:07 +0000,
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.

Skipping breakpoint instructions? I don't think you can do that, as
the guest does rely on BRK always being effective. I also don't see
where you do that...

> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 53fc8a6eaf1c..e5a06ff1cba6 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -726,6 +726,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..7835c76347ce 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>  }
>  
> +/*
> + * Returns true if the host needs to use the debug registers.
> + */
> +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);

Just the name of the function has sent my head spinning. Even if the
*effects* of the host debug and the OS Lock are vaguely similar from
the guest PoV, they really are different things, and I'd rather not
lob them together.

> +}
> +
>  /**
>   * kvm_arm_init_debug - grab what we need for debug
>   *
> @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	 *  - Userspace is using the hardware to debug the guest
>  	 *  (KVM_GUESTDBG_USE_HW is set).
>  	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> +	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
>  	 */
>  	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> -	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> +	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> +	    kvm_vcpu_os_lock_enabled(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  
>  	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {

I'd rather you expand the helper here and add the comment you have in
the commit message explaining the machine-wide effect of the OS Lock.

>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Restore the guest's debug registers if we were using them.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5dbdb45d6d44..1346906f5c46 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1453,9 +1453,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 },

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 v3 4/6] KVM: arm64: Emulate the OS Lock

[Marc Zyngier]

On Tue, 23 Nov 2021 21:01:07 +0000,
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.

Skipping breakpoint instructions? I don't think you can do that, as
the guest does rely on BRK always being effective. I also don't see
where you do that...

> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  4 ++++
>  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
>  arch/arm64/kvm/sys_regs.c         |  6 +++---
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 53fc8a6eaf1c..e5a06ff1cba6 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -726,6 +726,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..7835c76347ce 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  				vcpu_read_sys_reg(vcpu, MDSCR_EL1));
>  }
>  
> +/*
> + * Returns true if the host needs to use the debug registers.
> + */
> +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);

Just the name of the function has sent my head spinning. Even if the
*effects* of the host debug and the OS Lock are vaguely similar from
the guest PoV, they really are different things, and I'd rather not
lob them together.

> +}
> +
>  /**
>   * kvm_arm_init_debug - grab what we need for debug
>   *
> @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  	 *  - Userspace is using the hardware to debug the guest
>  	 *  (KVM_GUESTDBG_USE_HW is set).
>  	 *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> +	 *  - The guest has enabled the OS Lock (debug exceptions are blocked).
>  	 */
>  	if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> -	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> +	    !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> +	    kvm_vcpu_os_lock_enabled(vcpu))
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  
>  	trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {

I'd rather you expand the helper here and add the comment you have in
the commit message explaining the machine-wide effect of the OS Lock.

>  		/* Save guest debug state */
>  		save_guest_debug_regs(vcpu);
>  
> @@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
>  {
>  	trace_kvm_arm_clear_debug(vcpu->guest_debug);
>  
> -	if (vcpu->guest_debug) {
> +	/*
> +	 * Restore the guest's debug registers if we were using them.
> +	 */
> +	if (host_using_debug_regs(vcpu)) {
>  		restore_guest_debug_regs(vcpu);
>  
>  		/*
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 5dbdb45d6d44..1346906f5c46 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1453,9 +1453,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 },

Thanks,

	M.

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

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

[Oliver Upton]

Hey Marc,

Apologies for my delay in getting back to you, I was OOO for a while.

On Mon, Nov 29, 2021 at 8:16 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:07 +0000,
> 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.
>
> Skipping breakpoint instructions? I don't think you can do that, as
> the guest does rely on BRK always being effective. I also don't see
> where you do that...

Right, this comment in the commit message is stale. In the previous
iteration I had done this, but removed it per your suggestion. I'll
fix the msg in the next round.

> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 53fc8a6eaf1c..e5a06ff1cba6 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -726,6 +726,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..7835c76347ce 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> >                               vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> >  }
> >
> > +/*
> > + * Returns true if the host needs to use the debug registers.
> > + */
> > +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> > +{
> > +     return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
>
> Just the name of the function has sent my head spinning. Even if the
> *effects* of the host debug and the OS Lock are vaguely similar from
> the guest PoV, they really are different things, and I'd rather not
> lob them together.
>
> > +}
> > +
> >  /**
> >   * kvm_arm_init_debug - grab what we need for debug
> >   *
> > @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >        *  - Userspace is using the hardware to debug the guest
> >        *  (KVM_GUESTDBG_USE_HW is set).
> >        *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> > +      *  - The guest has enabled the OS Lock (debug exceptions are blocked).
> >        */
> >       if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> > -         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> > +         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> > +         kvm_vcpu_os_lock_enabled(vcpu))
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> >
> >       trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> > @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> > +      */
> > +     if (host_using_debug_regs(vcpu)) {
>
> I'd rather you expand the helper here and add the comment you have in
> the commit message explaining the machine-wide effect of the OS Lock.

Ack to here and the above comment. Thanks for the review!

--
Oliver

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

[Oliver Upton]

Hey Marc,

Apologies for my delay in getting back to you, I was OOO for a while.

On Mon, Nov 29, 2021 at 8:16 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:07 +0000,
> 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.
>
> Skipping breakpoint instructions? I don't think you can do that, as
> the guest does rely on BRK always being effective. I also don't see
> where you do that...

Right, this comment in the commit message is stale. In the previous
iteration I had done this, but removed it per your suggestion. I'll
fix the msg in the next round.

> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 53fc8a6eaf1c..e5a06ff1cba6 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -726,6 +726,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..7835c76347ce 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> >                               vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> >  }
> >
> > +/*
> > + * Returns true if the host needs to use the debug registers.
> > + */
> > +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> > +{
> > +     return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
>
> Just the name of the function has sent my head spinning. Even if the
> *effects* of the host debug and the OS Lock are vaguely similar from
> the guest PoV, they really are different things, and I'd rather not
> lob them together.
>
> > +}
> > +
> >  /**
> >   * kvm_arm_init_debug - grab what we need for debug
> >   *
> > @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >        *  - Userspace is using the hardware to debug the guest
> >        *  (KVM_GUESTDBG_USE_HW is set).
> >        *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> > +      *  - The guest has enabled the OS Lock (debug exceptions are blocked).
> >        */
> >       if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> > -         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> > +         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> > +         kvm_vcpu_os_lock_enabled(vcpu))
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> >
> >       trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> > @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> > +      */
> > +     if (host_using_debug_regs(vcpu)) {
>
> I'd rather you expand the helper here and add the comment you have in
> the commit message explaining the machine-wide effect of the OS Lock.

Ack to here and the above comment. Thanks for the review!

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

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

[Oliver Upton]

Hey Marc,

Apologies for my delay in getting back to you, I was OOO for a while.

On Mon, Nov 29, 2021 at 8:16 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:07 +0000,
> 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.
>
> Skipping breakpoint instructions? I don't think you can do that, as
> the guest does rely on BRK always being effective. I also don't see
> where you do that...

Right, this comment in the commit message is stale. In the previous
iteration I had done this, but removed it per your suggestion. I'll
fix the msg in the next round.

> >
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  4 ++++
> >  arch/arm64/kvm/debug.c            | 27 +++++++++++++++++++++++----
> >  arch/arm64/kvm/sys_regs.c         |  6 +++---
> >  3 files changed, 30 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 53fc8a6eaf1c..e5a06ff1cba6 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -726,6 +726,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..7835c76347ce 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> >                               vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> >  }
> >
> > +/*
> > + * Returns true if the host needs to use the debug registers.
> > + */
> > +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu)
> > +{
> > +     return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu);
>
> Just the name of the function has sent my head spinning. Even if the
> *effects* of the host debug and the OS Lock are vaguely similar from
> the guest PoV, they really are different things, and I'd rather not
> lob them together.
>
> > +}
> > +
> >  /**
> >   * kvm_arm_init_debug - grab what we need for debug
> >   *
> > @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
> >        *  - Userspace is using the hardware to debug the guest
> >        *  (KVM_GUESTDBG_USE_HW is set).
> >        *  - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear).
> > +      *  - The guest has enabled the OS Lock (debug exceptions are blocked).
> >        */
> >       if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) ||
> > -         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> > +         !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) ||
> > +         kvm_vcpu_os_lock_enabled(vcpu))
> >               vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
> >
> >       trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2);
> > @@ -160,8 +170,10 @@ 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 we need to use the debug registers.
> > +      */
> > +     if (host_using_debug_regs(vcpu)) {
>
> I'd rather you expand the helper here and add the comment you have in
> the commit message explaining the machine-wide effect of the OS Lock.

Ack to here and the above comment. 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

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

[Oliver Upton]

On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:06 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > the value for now.
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> >  2 files changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 16b3f1a1d468..9fad61a82047 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
>
> This looks odd. It means that once I have set the lock from userspace,
> I can't clear it?

This does read weird, but I believe it makes sense still. rd->val is
the value of the register after warm reset, and does not store the
current value of the actual register. The true value is stashed in
kvm_cpu_context. Really, what I'm asserting here is that the only RW
bit is the OSLK bit. If any of the other bits are changed it should
return an error.

I can either add a comment or make a macro for the expected register
value (or both) to make this more clear.

--
Thanks,
Oliver

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

[Oliver Upton]

On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:06 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > the value for now.
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> >  2 files changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 16b3f1a1d468..9fad61a82047 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
>
> This looks odd. It means that once I have set the lock from userspace,
> I can't clear it?

This does read weird, but I believe it makes sense still. rd->val is
the value of the register after warm reset, and does not store the
current value of the actual register. The true value is stashed in
kvm_cpu_context. Really, what I'm asserting here is that the only RW
bit is the OSLK bit. If any of the other bits are changed it should
return an error.

I can either add a comment or make a macro for the expected register
value (or both) to make this more clear.

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

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

[Oliver Upton]

On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 23 Nov 2021 21:01:06 +0000,
> Oliver Upton <oupton@google.com> wrote:
> >
> > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > the value for now.
> >
> > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > Signed-off-by: Oliver Upton <oupton@google.com>
> > ---
> >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> >  2 files changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > index 16b3f1a1d468..9fad61a82047 100644
> > --- a/arch/arm64/include/asm/sysreg.h
> > +++ b/arch/arm64/include/asm/sysreg.h
> > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
>
> This looks odd. It means that once I have set the lock from userspace,
> I can't clear it?

This does read weird, but I believe it makes sense still. rd->val is
the value of the register after warm reset, and does not store the
current value of the actual register. The true value is stashed in
kvm_cpu_context. Really, what I'm asserting here is that the only RW
bit is the OSLK bit. If any of the other bits are changed it should
return an error.

I can either add a comment or make a macro for the expected register
value (or both) to make this more clear.

--
Thanks,
Oliver

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

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

[Marc Zyngier]

Hi Oliver,

On Mon, 06 Dec 2021 17:39:05 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 23 Nov 2021 21:01:06 +0000,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > > the value for now.
> > >
> > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> > >  2 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index 16b3f1a1d468..9fad61a82047 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
> >
> > This looks odd. It means that once I have set the lock from userspace,
> > I can't clear it?
> 
> This does read weird, but I believe it makes sense still. rd->val is
> the value of the register after warm reset, and does not store the
> current value of the actual register. The true value is stashed in
> kvm_cpu_context. Really, what I'm asserting here is that the only RW
> bit is the OSLK bit. If any of the other bits are changed it should
> return an error.

Ah, the beauty of reading code in patches only. Of course, rd->val is
only the reset value. And isn't called that, just to be confusing.

Apologies for the noise.

> I can either add a comment or make a macro for the expected register
> value (or both) to make this more clear.

A macro for the reset value would certainly be beneficial.

But also, why not check the value against the current state and ignore
the reset state altogether, since by the time you can poke at the
vcpu, it has already been reset? It would certainly be more idiomatic.

Thanks,

	M.

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

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

[Marc Zyngier]

Hi Oliver,

On Mon, 06 Dec 2021 17:39:05 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 23 Nov 2021 21:01:06 +0000,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > > the value for now.
> > >
> > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> > >  2 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index 16b3f1a1d468..9fad61a82047 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
> >
> > This looks odd. It means that once I have set the lock from userspace,
> > I can't clear it?
> 
> This does read weird, but I believe it makes sense still. rd->val is
> the value of the register after warm reset, and does not store the
> current value of the actual register. The true value is stashed in
> kvm_cpu_context. Really, what I'm asserting here is that the only RW
> bit is the OSLK bit. If any of the other bits are changed it should
> return an error.

Ah, the beauty of reading code in patches only. Of course, rd->val is
only the reset value. And isn't called that, just to be confusing.

Apologies for the noise.

> I can either add a comment or make a macro for the expected register
> value (or both) to make this more clear.

A macro for the reset value would certainly be beneficial.

But also, why not check the value against the current state and ignore
the reset state altogether, since by the time you can poke at the
vcpu, it has already been reset? It would certainly be more idiomatic.

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 v3 3/6] KVM: arm64: Allow guest to set the OSLK bit

[Marc Zyngier]

Hi Oliver,

On Mon, 06 Dec 2021 17:39:05 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> On Mon, Nov 29, 2021 at 5:51 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 23 Nov 2021 21:01:06 +0000,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > Allow writes to OSLAR and forward the OSLK bit to OSLSR. Do nothing with
> > > the value for now.
> > >
> > > Reviewed-by: Reiji Watanabe <reijiw@google.com>
> > > Signed-off-by: Oliver Upton <oupton@google.com>
> > > ---
> > >  arch/arm64/include/asm/sysreg.h |  6 ++++++
> > >  arch/arm64/kvm/sys_regs.c       | 33 ++++++++++++++++++++++++++-------
> > >  2 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> > > index 16b3f1a1d468..9fad61a82047 100644
> > > --- a/arch/arm64/include/asm/sysreg.h
> > > +++ b/arch/arm64/include/asm/sysreg.h
> > > @@ -129,7 +129,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 7bf350b3d9cd..5dbdb45d6d44 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;
> >
> > This looks odd. It means that once I have set the lock from userspace,
> > I can't clear it?
> 
> This does read weird, but I believe it makes sense still. rd->val is
> the value of the register after warm reset, and does not store the
> current value of the actual register. The true value is stashed in
> kvm_cpu_context. Really, what I'm asserting here is that the only RW
> bit is the OSLK bit. If any of the other bits are changed it should
> return an error.

Ah, the beauty of reading code in patches only. Of course, rd->val is
only the reset value. And isn't called that, just to be confusing.

Apologies for the noise.

> I can either add a comment or make a macro for the expected register
> value (or both) to make this more clear.

A macro for the reset value would certainly be beneficial.

But also, why not check the value against the current state and ignore
the reset state altogether, since by the time you can poke at the
vcpu, it has already been reset? It would certainly be more idiomatic.

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