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

Marc Zyngier
2020-07-06T12:17:35Z

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...