Hi Alex, Marc,
(just on this last_vcpu_ran thing...)
On 11/05/2020 17:38, Alexandru Elisei wrote:
> On 4/22/20 1:00 PM, Marc Zyngier wrote:
>> From: Christoffer Dall <christoffer.dall@xxxxxxx>
>>
>> As we are about to reuse our stage 2 page table manipulation code for
>> shadow stage 2 page tables in the context of nested virtualization, we
>> are going to manage multiple stage 2 page tables for a single VM.
>>
>> This requires some pretty invasive changes to our data structures,
>> which moves the vmid and pgd pointers into a separate structure and
>> change pretty much all of our mmu code to operate on this structure
>> instead.
>>
>> The new structure is called struct kvm_s2_mmu.
>>
>> There is no intended functional change by this patch alone.
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 7dd8fefa6aecd..664a5d92ae9b8 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -63,19 +63,32 @@ struct kvm_vmid {
>> u32 vmid;
>> };
>>
>> -struct kvm_arch {
>> +struct kvm_s2_mmu {
>> struct kvm_vmid vmid;
>>
>> - /* stage2 entry level table */
>> - pgd_t *pgd;
>> - phys_addr_t pgd_phys;
>> -
>> - /* VTCR_EL2 value for this VM */
>> - u64 vtcr;
>> + /*
>> + * stage2 entry level table
>> + *
>> + * Two kvm_s2_mmu structures in the same VM can point to the same pgd
>> + * here. This happens when running a non-VHE guest hypervisor which
>> + * uses the canonical stage 2 page table for both vEL2 and for vEL1/0
>> + * with vHCR_EL2.VM == 0.
>
> It makes more sense to me to say that a non-VHE guest hypervisor will use the
> canonical stage *1* page table when running at EL2
Can KVM say anything about stage1? Its totally under the the guests control even at vEL2...
> (the "Non-secure EL2 translation regime" as ARM DDI 0487F.b calls it on page D5-2543).
> I think that's
> the only situation where vEL2 and vEL1&0 will use the same L0 stage 2 tables. It's
> been quite some time since I reviewed the initial version of the NV patches, did I
> get that wrong?
>> + */
>> + pgd_t *pgd;
>> + phys_addr_t pgd_phys;
>>
>> /* The last vcpu id that ran on each physical CPU */
>> int __percpu *last_vcpu_ran;
>
> It makes sense for the other fields to be part of kvm_s2_mmu, but I'm struggling
> to figure out why last_vcpu_ran is here. Would you mind sharing the rationale? I
> don't see this change in v1 or v2 of the NV series.
Marc may have a better rationale. My thinking was because kvm_vmid is in here too.
last_vcpu_ran exists to prevent KVM accidentally emulating CNP without the opt-in. (we
call it defacto CNP).
The guest may expect to be able to use asid-4 with different page tables on different
vCPUs, assuming the TLB isn't shared. But if KVM is switching between those vCPU on one
physical CPU, the TLB is shared, ... the VMID and ASID are the same, but the page tables
are not. Not fun to debug!
NV makes this problem per-stage2, because each stage2 has its own VMID, we need to track
the vcpu_id that last ran this stage2 on this physical CPU. If its not the same, we need
to blow away this VMIDs TLB entries.
The workaround lives in virt/kvm/arm/arm.c::kvm_arch_vcpu_load()
> More below.
(lightly trimmed!)
Thanks,
James
>>
>> + struct kvm *kvm;
>> +};
[...]
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 53b3ba9173ba7..03f01fcfa2bd5 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>
> There's a comment that still mentions arch.vmid that you missed in this file:
>
> static bool need_new_vmid_gen(struct kvm_vmid *vmid)
> {
> u64 current_vmid_gen = atomic64_read(&kvm_vmid_gen);
> smp_rmb(); /* Orders read of kvm_vmid_gen and kvm->arch.vmid */
>
[..]
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index e3b9ee268823b..2f99749048285 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -886,21 +898,23 @@ int create_hyp_exec_mappings(phys_addr_t phys_addr, size_t size,
>> }
>>
>> /**
>> - * kvm_alloc_stage2_pgd - allocate level-1 table for stage-2 translation.
>> - * @kvm: The KVM struct pointer for the VM.
>> + * kvm_init_stage2_mmu - Initialise a S2 MMU strucrure
>> + * @kvm: The pointer to the KVM structure
>> + * @mmu: The pointer to the s2 MMU structure
>> *
>> * Allocates only the stage-2 HW PGD level table(s) of size defined by
>> - * stage2_pgd_size(kvm).
>> + * stage2_pgd_size(mmu->kvm).
>> *
>> * Note we don't need locking here as this is only called when the VM is
>> * created, which can only be done once.
>> */
>> -int kvm_alloc_stage2_pgd(struct kvm *kvm)
>> +int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
>> {
>> phys_addr_t pgd_phys;
>> pgd_t *pgd;
>> + int cpu;
>>
>> - if (kvm->arch.pgd != NULL) {
>> + if (mmu->pgd != NULL) {
>> kvm_err("kvm_arch already initialized?\n");
>> return -EINVAL;
>> }
>> @@ -914,8 +928,20 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>> if (WARN_ON(pgd_phys & ~kvm_vttbr_baddr_mask(kvm)))
> We don't free the pgd here, but we do free it if alloc_percpu fails. Is that
> intentional?
>> return -EINVAL;
>>
>> - kvm->arch.pgd = pgd;
>> - kvm->arch.pgd_phys = pgd_phys;
>> + mmu->last_vcpu_ran = alloc_percpu(typeof(*mmu->last_vcpu_ran));
>> + if (!mmu->last_vcpu_ran) {
>> + free_pages_exact(pgd, stage2_pgd_size(kvm));
>> + return -ENOMEM;
>> + }
>> +
>> + for_each_possible_cpu(cpu)
>> + *per_cpu_ptr(mmu->last_vcpu_ran, cpu) = -1;
>> +
>> + mmu->kvm = kvm;
>> + mmu->pgd = pgd;
>> + mmu->pgd_phys = pgd_phys;
>> + mmu->vmid.vmid_gen = 0;
>> +
>> return 0;
>> }
>>
>> @@ -986,39 +1012,34 @@ void stage2_unmap_vm(struct kvm *kvm)
>> srcu_read_unlock(&kvm->srcu, idx);
>> }
>>
>> -/**
>> - * kvm_free_stage2_pgd - free all stage-2 tables
>> - * @kvm: The KVM struct pointer for the VM.
>> - *
>> - * Walks the level-1 page table pointed to by kvm->arch.pgd and frees all
>> - * underlying level-2 and level-3 tables before freeing the actual level-1 table
>> - * and setting the struct pointer to NULL.
>> - */
>> -void kvm_free_stage2_pgd(struct kvm *kvm)
>> +void kvm_free_stage2_pgd(struct kvm_s2_mmu *mmu)
>> {
>> + struct kvm *kvm = mmu->kvm;
>> void *pgd = NULL;
>>
>> spin_lock(&kvm->mmu_lock);
>> - if (kvm->arch.pgd) {
>> - unmap_stage2_range(kvm, 0, kvm_phys_size(kvm));
>> - pgd = READ_ONCE(kvm->arch.pgd);
>> - kvm->arch.pgd = NULL;
>> - kvm->arch.pgd_phys = 0;
>> + if (mmu->pgd) {
>> + unmap_stage2_range(mmu, 0, kvm_phys_size(kvm));
>> + pgd = READ_ONCE(mmu->pgd);
>> + mmu->pgd = NULL;
>
> The kvm->arch.pgd_phys = 0 instruction seems to have been dropped here. Is that
> intentional?
