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

[Marc Zyngier]

On 2020-06-25 13:19, Alexandru Elisei wrote:
> 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.

Cool. Will you be posting such patch?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...