| Message ID | 20200211174938.27809-13-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: ARMv8.3/8.4 Nested Virtualization support | expand |
On Tue, Feb 11, 2020 at 05:48:16PM +0000, Marc Zyngier wrote: > Some EL2 system registers immediately affect the current execution > of the system, so we need to use their respective EL1 counterparts. > For this we need to define a mapping between the two. > > These helpers will get used in subsequent patches. > > Co-developed-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/kvm_emulate.h | 6 ++++ > arch/arm64/kvm/sys_regs.c | 48 ++++++++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 282e9ddbe1bc..486978d0346b 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -58,6 +58,12 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu); > int kvm_inject_nested_sync(struct kvm_vcpu *vcpu, u64 esr_el2); > int kvm_inject_nested_irq(struct kvm_vcpu *vcpu); > > +u64 translate_tcr(u64 tcr); > +u64 translate_cptr(u64 tcr); > +u64 translate_sctlr(u64 tcr); > +u64 translate_ttbr0(u64 tcr); > +u64 translate_cnthctl(u64 tcr); Sorry to bikeshed, but could we please make the direction of translation explicit in the name? e.g. tcr_el2_to_tcr_el1(), or tcr_el2_to_el1()? > + > static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) > { > return !(vcpu->arch.hcr_el2 & HCR_RW); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 4b5310ea3bf8..634d3ee6799c 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -65,6 +65,54 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu, > return false; > } > > +static u64 tcr_el2_ips_to_tcr_el1_ps(u64 tcr_el2) > +{ > + return ((tcr_el2 & TCR_EL2_PS_MASK) >> TCR_EL2_PS_SHIFT) > + << TCR_IPS_SHIFT; > +} > + > +u64 translate_tcr(u64 tcr) > +{ > + return TCR_EPD1_MASK | /* disable TTBR1_EL1 */ > + ((tcr & TCR_EL2_TBI) ? TCR_TBI0 : 0) | > + tcr_el2_ips_to_tcr_el1_ps(tcr) | > + (tcr & TCR_EL2_TG0_MASK) | > + (tcr & TCR_EL2_ORGN0_MASK) | > + (tcr & TCR_EL2_IRGN0_MASK) | > + (tcr & TCR_EL2_T0SZ_MASK); > +} I'm guessing this is only meant to cover a !VHE guest EL2 for the moment, so only covers HCR_EL2.E2H=0? It might be worth mentioning in the commit message. It looks like this is missing some bits (e.g. TBID, HPD, HD, HA) that could apply to the Guest-EL2 Stage-1. Maybe those are added by later patches, but that's not obvious to me at this point in the series. > + > +u64 translate_cptr(u64 cptr_el2) > +{ > + u64 cpacr_el1 = 0; > + > + if (!(cptr_el2 & CPTR_EL2_TFP)) > + cpacr_el1 |= CPACR_EL1_FPEN; > + if (cptr_el2 & CPTR_EL2_TTA) > + cpacr_el1 |= CPACR_EL1_TTA; > + if (!(cptr_el2 & CPTR_EL2_TZ)) > + cpacr_el1 |= CPACR_EL1_ZEN; > + > + return cpacr_el1; > +} Looking in ARM DDI 0487E.a I also see TCPAC and TAM; I guess we don't need to map those to anthing? > + > +u64 translate_sctlr(u64 sctlr) > +{ > + /* Bit 20 is RES1 in SCTLR_EL1, but RES0 in SCTLR_EL2 */ > + return sctlr | BIT(20); > +} Looking in ARM DDI 0487E.a section D13.2.105, bit 20 is TSCXT, so this might need to be reconsidered. > + > +u64 translate_ttbr0(u64 ttbr0) > +{ > + /* Force ASID to 0 (ASID 0 or RES0) */ > + return ttbr0 & ~GENMASK_ULL(63, 48); > +} Again, I assume this is only meant to provide a !VHE EL2 as this stands. > + > +u64 translate_cnthctl(u64 cnthctl) > +{ > + return ((cnthctl & 0x3) << 10) | (cnthctl & 0xfc); > +} I assume this yields CNTKCTL_EL1, but I don't entirely follow. For virtual-EL2 don't we have to force EL1P(C)TEN so that virtual-EL2 accesses don't trap? Thanks, Mark.
Hi Mark, Congratulations, you will now be CC'd on all the subsequent postings of this series! Yes, I'm that nice! ;-) On 2020-02-17 14:56, Mark Rutland wrote: > On Tue, Feb 11, 2020 at 05:48:16PM +0000, Marc Zyngier wrote: >> Some EL2 system registers immediately affect the current execution >> of the system, so we need to use their respective EL1 counterparts. >> For this we need to define a mapping between the two. >> >> These helpers will get used in subsequent patches. >> >> Co-developed-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/include/asm/kvm_emulate.h | 6 ++++ >> arch/arm64/kvm/sys_regs.c | 48 >> ++++++++++++++++++++++++++++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_emulate.h >> b/arch/arm64/include/asm/kvm_emulate.h >> index 282e9ddbe1bc..486978d0346b 100644 >> --- a/arch/arm64/include/asm/kvm_emulate.h >> +++ b/arch/arm64/include/asm/kvm_emulate.h >> @@ -58,6 +58,12 @@ void kvm_emulate_nested_eret(struct kvm_vcpu >> *vcpu); >> int kvm_inject_nested_sync(struct kvm_vcpu *vcpu, u64 esr_el2); >> int kvm_inject_nested_irq(struct kvm_vcpu *vcpu); >> >> +u64 translate_tcr(u64 tcr); >> +u64 translate_cptr(u64 tcr); >> +u64 translate_sctlr(u64 tcr); >> +u64 translate_ttbr0(u64 tcr); >> +u64 translate_cnthctl(u64 tcr); > > Sorry to bikeshed, but could we please make the direction of > translation > explicit in the name? e.g. tcr_el2_to_tcr_el1(), or tcr_el2_to_el1()? Sure, that's an easy one! > >> + >> static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) >> { >> return !(vcpu->arch.hcr_el2 & HCR_RW); >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 4b5310ea3bf8..634d3ee6799c 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -65,6 +65,54 @@ static bool write_to_read_only(struct kvm_vcpu >> *vcpu, >> return false; >> } >> >> +static u64 tcr_el2_ips_to_tcr_el1_ps(u64 tcr_el2) >> +{ >> + return ((tcr_el2 & TCR_EL2_PS_MASK) >> TCR_EL2_PS_SHIFT) >> + << TCR_IPS_SHIFT; >> +} >> + >> +u64 translate_tcr(u64 tcr) >> +{ >> + return TCR_EPD1_MASK | /* disable TTBR1_EL1 */ >> + ((tcr & TCR_EL2_TBI) ? TCR_TBI0 : 0) | >> + tcr_el2_ips_to_tcr_el1_ps(tcr) | >> + (tcr & TCR_EL2_TG0_MASK) | >> + (tcr & TCR_EL2_ORGN0_MASK) | >> + (tcr & TCR_EL2_IRGN0_MASK) | >> + (tcr & TCR_EL2_T0SZ_MASK); >> +} > > I'm guessing this is only meant to cover a !VHE guest EL2 for the > moment, so only covers HCR_EL2.E2H=0? It might be worth mentioning in > the commit message. Indeed, all the "translate_*" function have a single purpose: converting an !VHE EL2 layout into an EL1 layout. > It looks like this is missing some bits (e.g. TBID, HPD, HD, HA) that > could apply to the Guest-EL2 Stage-1. Maybe those are added by later > patches, but that's not obvious to me at this point in the series. ARMv8.3-PAUTH isn't supported, and ARMv8.1-TTHM cannot be supported at Stage-2, so we don't support it at Stage-1 either (even if we technically could). Maybe worth suggesting to the powers that be... ARMv8.1-HPD is an oversight though, and we should be able to support it. > >> + >> +u64 translate_cptr(u64 cptr_el2) >> +{ >> + u64 cpacr_el1 = 0; >> + >> + if (!(cptr_el2 & CPTR_EL2_TFP)) >> + cpacr_el1 |= CPACR_EL1_FPEN; >> + if (cptr_el2 & CPTR_EL2_TTA) >> + cpacr_el1 |= CPACR_EL1_TTA; >> + if (!(cptr_el2 & CPTR_EL2_TZ)) >> + cpacr_el1 |= CPACR_EL1_ZEN; >> + >> + return cpacr_el1; >> +} > > Looking in ARM DDI 0487E.a I also see TCPAC and TAM; I guess we don't > need to map those to anthing? TCPAC allows us to trap CPACR_EL1, but we always have the physical CPTR_EL2.TCPAC set in this case, so that bit doesn't need to translate into anything. TAM doesn't seem to translate into anything in CPACR_EL1, and I don't plan to support the AMU any time soon with NV! ;-) > >> + >> +u64 translate_sctlr(u64 sctlr) >> +{ >> + /* Bit 20 is RES1 in SCTLR_EL1, but RES0 in SCTLR_EL2 */ >> + return sctlr | BIT(20); >> +} > > Looking in ARM DDI 0487E.a section D13.2.105, bit 20 is TSCXT, so this > might need to be reconsidered. Huhuh, nice catch! We need to detect ARMv8.0-CSV2 here, and set the bit accordingly. > >> + >> +u64 translate_ttbr0(u64 ttbr0) >> +{ >> + /* Force ASID to 0 (ASID 0 or RES0) */ >> + return ttbr0 & ~GENMASK_ULL(63, 48); >> +} > > Again, I assume this is only meant to provide a !VHE EL2 as this > stands. Indeed. I guess I need to write a better commit message to make this clear. > >> + >> +u64 translate_cnthctl(u64 cnthctl) >> +{ >> + return ((cnthctl & 0x3) << 10) | (cnthctl & 0xfc); >> +} > > I assume this yields CNTKCTL_EL1, but I don't entirely follow. For > virtual-EL2 don't we have to force EL1P(C)TEN so that virtual-EL2 > accesses don't trap? A non-VHE guest will use the _EL2 instructions to access its own timer, which will trap. At the same time, we use the EL1 timer to implement the vEL2 timer, meaning we also need to trap the EL1 timer. Yes, !VHE looses on all fronts. We could treat !VHE specially in the wap we map timers (complete emulation for vEL2, and direct access for guest timers). But this seems pretty complex for very little gain. Thanks, M.
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 282e9ddbe1bc..486978d0346b 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -58,6 +58,12 @@ void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu); int kvm_inject_nested_sync(struct kvm_vcpu *vcpu, u64 esr_el2); int kvm_inject_nested_irq(struct kvm_vcpu *vcpu); +u64 translate_tcr(u64 tcr); +u64 translate_cptr(u64 tcr); +u64 translate_sctlr(u64 tcr); +u64 translate_ttbr0(u64 tcr); +u64 translate_cnthctl(u64 tcr); + static inline bool vcpu_el1_is_32bit(struct kvm_vcpu *vcpu) { return !(vcpu->arch.hcr_el2 & HCR_RW); diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 4b5310ea3bf8..634d3ee6799c 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -65,6 +65,54 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu, return false; } +static u64 tcr_el2_ips_to_tcr_el1_ps(u64 tcr_el2) +{ + return ((tcr_el2 & TCR_EL2_PS_MASK) >> TCR_EL2_PS_SHIFT) + << TCR_IPS_SHIFT; +} + +u64 translate_tcr(u64 tcr) +{ + return TCR_EPD1_MASK | /* disable TTBR1_EL1 */ + ((tcr & TCR_EL2_TBI) ? TCR_TBI0 : 0) | + tcr_el2_ips_to_tcr_el1_ps(tcr) | + (tcr & TCR_EL2_TG0_MASK) | + (tcr & TCR_EL2_ORGN0_MASK) | + (tcr & TCR_EL2_IRGN0_MASK) | + (tcr & TCR_EL2_T0SZ_MASK); +} + +u64 translate_cptr(u64 cptr_el2) +{ + u64 cpacr_el1 = 0; + + if (!(cptr_el2 & CPTR_EL2_TFP)) + cpacr_el1 |= CPACR_EL1_FPEN; + if (cptr_el2 & CPTR_EL2_TTA) + cpacr_el1 |= CPACR_EL1_TTA; + if (!(cptr_el2 & CPTR_EL2_TZ)) + cpacr_el1 |= CPACR_EL1_ZEN; + + return cpacr_el1; +} + +u64 translate_sctlr(u64 sctlr) +{ + /* Bit 20 is RES1 in SCTLR_EL1, but RES0 in SCTLR_EL2 */ + return sctlr | BIT(20); +} + +u64 translate_ttbr0(u64 ttbr0) +{ + /* Force ASID to 0 (ASID 0 or RES0) */ + return ttbr0 & ~GENMASK_ULL(63, 48); +} + +u64 translate_cnthctl(u64 cnthctl) +{ + return ((cnthctl & 0x3) << 10) | (cnthctl & 0xfc); +} + u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg) { if (!vcpu->arch.sysregs_loaded_on_cpu)