Re: [PATCH v2 12/94] KVM: arm64: nv: Add EL2->EL1 translation helpers

[Marc Zyngier <maz@kernel.org>]

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.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm