On 24/07/2019 11:25, Tomasz Nowicki wrote:
> On 21.06.2019 11:38, Marc Zyngier wrote:
>> From: Jintack Lim <jintack.lim@xxxxxxxxxx>
>>
>> When supporting nested virtualization a guest hypervisor executing AT
>> instructions must be trapped and emulated by the host hypervisor,
>> because untrapped AT instructions operating on S1E1 will use the wrong
>> translation regieme (the one used to emulate virtual EL2 in EL1 instead
>> of virtual EL1) and AT instructions operating on S12 will not work from
>> EL1.
>>
>> This patch does several things.
>>
>> 1. List and define all AT system instructions to emulate and document
>> the emulation design.
>>
>> 2. Implement AT instruction handling logic in EL2. This will be used to
>> emulate AT instructions executed in the virtual EL2.
>>
>> AT instruction emulation works by loading the proper processor
>> context, which depends on the trapped instruction and the virtual
>> HCR_EL2, to the EL1 virtual memory control registers and executing AT
>> instructions. Note that ctxt->hw_sys_regs is expected to have the
>> proper processor context before calling the handling
>> function(__kvm_at_insn) implemented in this patch.
>>
>> 4. Emulate AT S1E[01] instructions by issuing the same instructions in
>> EL2. We set the physical EL1 registers, NV and NV1 bits as described in
>> the AT instruction emulation overview.
>>
>> 5. Emulate AT A12E[01] instructions in two steps: First, do the stage-1
>> translation by reusing the existing AT emulation functions. Second, do
>> the stage-2 translation by walking the guest hypervisor's stage-2 page
>> table in software. Record the translation result to PAR_EL1.
>>
>> 6. Emulate AT S1E2 instructions by issuing the corresponding S1E1
>> instructions in EL2. We set the physical EL1 registers and the HCR_EL2
>> register as described in the AT instruction emulation overview.
>>
>> 7. Forward system instruction traps to the virtual EL2 if the corresponding
>> virtual AT bit is set in the virtual HCR_EL2.
>>
>> [ Much logic above has been reworked by Marc Zyngier ]
>>
>> Signed-off-by: Jintack Lim <jintack.lim@xxxxxxxxxx>
>> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>> Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxx>
>> ---
>> arch/arm64/include/asm/kvm_arm.h | 2 +
>> arch/arm64/include/asm/kvm_asm.h | 2 +
>> arch/arm64/include/asm/sysreg.h | 17 +++
>> arch/arm64/kvm/hyp/Makefile | 1 +
>> arch/arm64/kvm/hyp/at.c | 217 +++++++++++++++++++++++++++++++
>> arch/arm64/kvm/hyp/switch.c | 13 +-
>> arch/arm64/kvm/sys_regs.c | 202 +++++++++++++++++++++++++++-
>> 7 files changed, 450 insertions(+), 4 deletions(-)
>> create mode 100644 arch/arm64/kvm/hyp/at.c
>>
>
> [...]
>
>> +
>> +void __kvm_at_s1e01(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
>> +{
>> + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt;
>> + struct mmu_config config;
>> + struct kvm_s2_mmu *mmu;
>> +
>> + /*
>> + * We can only get here when trapping from vEL2, so we're
>> + * translating a guest guest VA.
>> + *
>> + * FIXME: Obtaining the S2 MMU for a a guest guest is horribly
>> + * racy, and we may not find it.
>> + */
>> + spin_lock(&vcpu->kvm->mmu_lock);
>> +
>> + mmu = lookup_s2_mmu(vcpu->kvm,
>> + vcpu_read_sys_reg(vcpu, VTTBR_EL2),
>> + vcpu_read_sys_reg(vcpu, HCR_EL2));
>> +
>> + if (WARN_ON(!mmu))
>> + goto out;
>> +
>> + /* We've trapped, so everything is live on the CPU. */
>> + __mmu_config_save(&config);
>> +
>> + write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], SYS_TTBR0);
>> + write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], SYS_TTBR1);
>> + write_sysreg_el1(ctxt->sys_regs[TCR_EL1], SYS_TCR);
>> + write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1], SYS_SCTLR);
>> + write_sysreg(kvm_get_vttbr(mmu), vttbr_el2);
>> + /* FIXME: write S2 MMU VTCR_EL2 */
>> + write_sysreg(config.hcr & ~HCR_TGE, hcr_el2);
>
> All below AT S1E* operations may initiate stage-1 PTW. And stage-1 table
> walk addresses are themselves subject to stage-2 translation.
>
> So should we enable stage-2 translation here by setting HCR_VM bit?
Hopefully that's already the case, otherwise nothing works. Have you
verified that it is actually clear at this stage?
It's a bit of a moot point as we need a full S1 PTW in SW anyway (so
most of that code will eventually be removed), but at least this should
be fixed if it needs to.
Thanks,
M. (who really needs to get on top of these review comments)
--
Jazz is not dead. It just smells funny...
