diff mbox series

[v8,09/69] KVM: arm64: nv: Reset VMPIDR_EL2 and VPIDR_EL2 to sane values

Message ID 20230131092504.2880505-10-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: ARMv8.3/8.4 Nested Virtualization support | expand

Commit Message

Marc Zyngier Jan. 31, 2023, 9:24 a.m. UTC 
From: Christoffer Dall <christoffer.dall@arm.com>

The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
let's be nice to a guest hypervisor behaving foolishly and reset these
to something reasonable anyway.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Oliver Upton Jan. 31, 2023, 8:17 p.m. UTC | #1 
On Tue, Jan 31, 2023 at 09:24:04AM +0000, Marc Zyngier wrote:
> From: Christoffer Dall <christoffer.dall@arm.com>
> 
> The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
> let's be nice to a guest hypervisor behaving foolishly and reset these
> to something reasonable anyway.

Must we be so kind? :)

In all seriousness, I've found the hexspeak value of reset_unknown() to
be a rather useful debugging aid. And I can promise you that I'll use NV
to debug my own crap changes!

Any particular reason against just using reset_unknown()?
Marc Zyngier Jan. 31, 2023, 10:04 p.m. UTC | #2 
On Tue, 31 Jan 2023 20:17:50 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Jan 31, 2023 at 09:24:04AM +0000, Marc Zyngier wrote:
> > From: Christoffer Dall <christoffer.dall@arm.com>
> > 
> > The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
> > let's be nice to a guest hypervisor behaving foolishly and reset these
> > to something reasonable anyway.
> 
> Must we be so kind? :)
> 
> In all seriousness, I've found the hexspeak value of reset_unknown() to
> be a rather useful debugging aid. And I can promise you that I'll use NV
> to debug my own crap changes!
> 
> Any particular reason against just using reset_unknown()?

Because debugging NV itself is hell when all you have is a model!

As we were trying to debug the early code base, we really wanted to
make it easy to run tiny guests without much setup, and work out of
the box. That's how this sort of changes came about.

In any case, something like this the hack below works as well (I just
booted an L1 and a couple of L2s with it, and nothing caught fire).

	M.

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 924afc40ab8b..c1016a35a996 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -924,16 +924,6 @@ static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), MPIDR_EL1);
 }
 
-static void reset_vmpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
-{
-	vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), VMPIDR_EL2);
-}
-
-static void reset_vpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
-{
-	vcpu_write_sys_reg(vcpu, read_cpuid_id(), VPIDR_EL2);
-}
-
 static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
 				   const struct sys_reg_desc *r)
 {
@@ -2678,8 +2668,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ PMU_SYS_REG(SYS_PMCCFILTR_EL0), .access = access_pmu_evtyper,
 	  .reset = reset_val, .reg = PMCCFILTR_EL0, .val = 0 },
 
-	EL2_REG(VPIDR_EL2, access_rw, reset_vpidr, 0),
-	EL2_REG(VMPIDR_EL2, access_rw, reset_vmpidr, 0),
+	EL2_REG(VPIDR_EL2, access_rw, reset_unknown, 0),
+	EL2_REG(VMPIDR_EL2, access_rw, reset_unknown, 0),
 	EL2_REG(SCTLR_EL2, access_rw, reset_val, SCTLR_EL2_RES1),
 	EL2_REG(ACTLR_EL2, access_rw, reset_val, 0),
 	EL2_REG(HCR_EL2, access_rw, reset_val, 0),
Oliver Upton Jan. 31, 2023, 10:09 p.m. UTC | #3 
On Tue, Jan 31, 2023 at 10:04:40PM +0000, Marc Zyngier wrote:
> On Tue, 31 Jan 2023 20:17:50 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Tue, Jan 31, 2023 at 09:24:04AM +0000, Marc Zyngier wrote:
> > > From: Christoffer Dall <christoffer.dall@arm.com>
> > > 
> > > The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
> > > let's be nice to a guest hypervisor behaving foolishly and reset these
> > > to something reasonable anyway.
> > 
> > Must we be so kind? :)
> > 
> > In all seriousness, I've found the hexspeak value of reset_unknown() to
> > be a rather useful debugging aid. And I can promise you that I'll use NV
> > to debug my own crap changes!
> > 
> > Any particular reason against just using reset_unknown()?
> 
> Because debugging NV itself is hell when all you have is a model!

Blech!

> As we were trying to debug the early code base, we really wanted to
> make it easy to run tiny guests without much setup, and work out of
> the box. That's how this sort of changes came about.
> 
> In any case, something like this the hack below works as well (I just
> booted an L1 and a couple of L2s with it, and nothing caught fire).

LGTM, mind squashing it into the previous patch?
Marc Zyngier Jan. 31, 2023, 10:16 p.m. UTC | #4 
On Tue, 31 Jan 2023 22:09:53 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Jan 31, 2023 at 10:04:40PM +0000, Marc Zyngier wrote:
> > On Tue, 31 Jan 2023 20:17:50 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > On Tue, Jan 31, 2023 at 09:24:04AM +0000, Marc Zyngier wrote:
> > > > From: Christoffer Dall <christoffer.dall@arm.com>
> > > > 
> > > > The VMPIDR_EL2 and VPIDR_EL2 are architecturally UNKNOWN at reset, but
> > > > let's be nice to a guest hypervisor behaving foolishly and reset these
> > > > to something reasonable anyway.
> > > 
> > > Must we be so kind? :)
> > > 
> > > In all seriousness, I've found the hexspeak value of reset_unknown() to
> > > be a rather useful debugging aid. And I can promise you that I'll use NV
> > > to debug my own crap changes!
> > > 
> > > Any particular reason against just using reset_unknown()?
> > 
> > Because debugging NV itself is hell when all you have is a model!
> 
> Blech!
> 
> > As we were trying to debug the early code base, we really wanted to
> > make it easy to run tiny guests without much setup, and work out of
> > the box. That's how this sort of changes came about.
> > 
> > In any case, something like this the hack below works as well (I just
> > booted an L1 and a couple of L2s with it, and nothing caught fire).
> 
> LGTM, mind squashing it into the previous patch?

Sure thing.

	M.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index d11c9c4c177a..2a1e8f77b56c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -667,7 +667,7 @@  static void reset_actlr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	vcpu_write_sys_reg(vcpu, actlr, ACTLR_EL1);
 }
 
-static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+static u64 compute_reset_mpidr(struct kvm_vcpu *vcpu)
 {
 	u64 mpidr;
 
@@ -681,7 +681,24 @@  static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 	mpidr = (vcpu->vcpu_id & 0x0f) << MPIDR_LEVEL_SHIFT(0);
 	mpidr |= ((vcpu->vcpu_id >> 4) & 0xff) << MPIDR_LEVEL_SHIFT(1);
 	mpidr |= ((vcpu->vcpu_id >> 12) & 0xff) << MPIDR_LEVEL_SHIFT(2);
-	vcpu_write_sys_reg(vcpu, (1ULL << 31) | mpidr, MPIDR_EL1);
+	mpidr |= (1ULL << 31);
+
+	return mpidr;
+}
+
+static void reset_mpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), MPIDR_EL1);
+}
+
+static void reset_vmpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	vcpu_write_sys_reg(vcpu, compute_reset_mpidr(vcpu), VMPIDR_EL2);
+}
+
+static void reset_vpidr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
+{
+	vcpu_write_sys_reg(vcpu, read_cpuid_id(), VPIDR_EL2);
 }
 
 static unsigned int pmu_visibility(const struct kvm_vcpu *vcpu,
@@ -2100,8 +2117,8 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ PMU_SYS_REG(SYS_PMCCFILTR_EL0), .access = access_pmu_evtyper,
 	  .reset = reset_val, .reg = PMCCFILTR_EL0, .val = 0 },
 
-	EL2_REG(VPIDR_EL2, access_rw, reset_val, 0),
-	EL2_REG(VMPIDR_EL2, access_rw, reset_val, 0),
+	EL2_REG(VPIDR_EL2, access_rw, reset_vpidr, 0),
+	EL2_REG(VMPIDR_EL2, access_rw, reset_vmpidr, 0),
 	EL2_REG(SCTLR_EL2, access_rw, reset_val, SCTLR_EL2_RES1),
 	EL2_REG(ACTLR_EL2, access_rw, reset_val, 0),
 	EL2_REG(HCR_EL2, access_rw, reset_val, 0),