[PATCH v2 01/17] KVM: arm64: Factor out stage 2 page table data from struct kvm

[Alexandru Elisei]

Hi Marc,

On 6/16/20 5:18 PM, Marc Zyngier wrote:
> Hi Alexandru,
> [..]
>>> [..]
>>>
>>>  /**
>>> - * 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;
>>>      }
>>> @@ -1024,8 +1040,20 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm)
>>>      if (WARN_ON(pgd_phys & ~kvm_vttbr_baddr_mask(kvm)))
>>>          return -EINVAL;
>>
>> We don't free the pgd if we get the error above, but we do free it below, if
>> allocating last_vcpu_ran fails. Shouldn't we free it in both cases?
>
> Worth investigating. This code gets majorly revamped in the NV series, so it is
> likely that I missed something in the middle.

You didn't miss anything, I checked and it's the same in the upstream version of KVM.

kvm_arch_init_vm() returns with an error if this functions fails, so it's up to
the function to do the clean up. kvm_alloc_pages_exact() returns NULL on error, so
at this point we have a valid allocation of physical contiguous pages. Failing to
create a VM is not a fatal error for the system, so I'm thinking that maybe we
should free those pages for the rest of the system to use. However, this is a
minor issue, and the patch isn't supposed to make any functional changes, so it
can be probably be left for another patch and not add more to an already big series.

Thanks,
Alex