Thanks again for all the discussion. Let me summarize where we're at:
x86: needs vzalloc
powerpc: needs kzalloc
s390: wants kzalloc
arm when has_vhe() is false: needs kzalloc
arm when has_vhe() is true: could potentially use vzalloc
I think that Paolo's suggestion to use compile-time defines to select
between baseline vzalloc and kzalloc allocators seems like the best
baseline solution. Then, we could refine that for arm to ultimately select
the allocator based on has_vhe(). Unless I hear otherwise, this is what
I'll do for v4.
Thanks,
Marc
On Thu, May 10, 2018 at 2:35 AM Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 09/05/18 22:48, Marc Orr wrote:
> > Sure, I can update the patch again. But I have a question first, what is
> > has_vhe() in the untested patch above?
> VHE stands for "Virtualization Host Extension". The has_vhe() predicate
> indicates whether the kernel runs in the EL2 exception level or not.
> When true, there is no separation between the kernel itself and its
> hypervisor role. We're in a single address space, and can happily map
> memory anywhere. This happens when the CPU implements at least the
> ARMv8.1 architecture.
> When has_vhe() is false (which is the case on ARMv7 and ARMv8.0), the
> host kernel runs at EL1, and the hypervisor at EL2. This means that the
> two have different page tables, and have to cope with some rather strict
> requirements about what can be mapped in both (on ARMv8.0, EL1 has twice
> the addressing range as EL2, which is at best unpleasant).
> That's why the !VHE platforms use kmalloc, while the VHE ones could be
> converted to vmalloc. I still need to give it a good shake on a proper
> HW platform (I did a quick test on a model, but that's not quite the
> same...).
> Thanks,
> M.
> > Thanks,
> > Marc
> > On Tue, May 8, 2018 at 3:18 PM Paolo Bonzini <pbonzini@xxxxxxxxxx>
wrote:
> >
> >> On 08/05/2018 20:17, Marc Zyngier wrote:
> >>> On 08/05/18 15:56, Paolo Bonzini wrote:
> >>>> On 08/05/2018 07:16, Christian Borntraeger wrote:
> >>>>> On 05/08/2018 07:14 AM, Paul Mackerras wrote:
> >>>>>> On Mon, May 07, 2018 at 03:18:46PM -0700, Marc Orr wrote:
> >>>>>>> The kvm struct is (currently) tens of kilo-bytes, which turns out
> > to be a
> >>>>>>> large amount of memory to allocate contiguously via kzalloc. Thus,
> > this
> >>>>>>> patch changes the kzalloc to a vzalloc, so that the memory
> > allocation is
> >>>>>>> less likely to fail in resource-constrained environments.
> >>>>>>
> >>>>>> This will break HV KVM on powerpc, which needs the KVM struct to be
> >>>>>> physically contiguous and in the linear mapping. We'll need to add
> >>>>>> #define __KVM_HAVE_ARCH_VM_ALLOC to
> > arch/powerpc/include/asm/kvm_host.h
> >>>>>> and the kzalloc/kfree variant to arch/powerpc/kvm/powerpc.c much
like
> >>>>>> you did on arm.
> >>>>>
> >>>>> In the end I also want kmalloc for s390 (since we do not need the
> > vmalloc for
> >>>>> s390 as we are < 16kb).
> >>>>>
> >>>>> So Paolo,
> >>>>> can we turn things around and only use vmalloc for x86?
> >>>>>
> >>>>
> >>>> The other idea that the #define would pick kzalloc vs. vzalloc
(instead
> >>>> of enabling a custom kvm_arch_alloc_vm) would be even better I think.
> >>>
> >>> I'm not sure it is better, as it forces the architecture to make that
> >>> choice at compile time, while I'd like to be able to do it at runtime.
> >>>
> >>> Something like this (untested):
> >>>
> >>> diff --git a/arch/arm/include/asm/kvm_host.h
> > b/arch/arm/include/asm/kvm_host.h
> >>> index c6a749568dd6..b6b1bdafd08e 100644
> >>> --- a/arch/arm/include/asm/kvm_host.h
> >>> +++ b/arch/arm/include/asm/kvm_host.h
> >>> @@ -315,4 +315,8 @@ static inline bool
> > kvm_arm_harden_branch_predictor(void)
> >>> static inline void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu) {}
> >>> static inline void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu) {}
> >>>
> >>> +#define __KVM_HAVE_ARCH_VM_ALLOC
> >>> +struct kvm *kvm_arch_alloc_vm(void);
> >>> +void kvm_arch_free_vm(struct kvm *kvm);
> >>> +
> >>> #endif /* __ARM_KVM_HOST_H__ */
> >>> diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> >>> index ab46bc70add6..51a64ff6965e 100644
> >>> --- a/arch/arm64/include/asm/kvm_host.h
> >>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>> @@ -452,4 +452,8 @@ static inline bool
> > kvm_arm_harden_branch_predictor(void)
> >>> void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu);
> >>> void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu);
> >>>
> >>> +#define __KVM_HAVE_ARCH_VM_ALLOC
> >>> +struct kvm *kvm_arch_alloc_vm(void);
> >>> +void kvm_arch_free_vm(struct kvm *kvm);
> >>> +
> >>> #endif /* __ARM64_KVM_HOST_H__ */
> >>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> >>> index dba629c5f8ac..21d74e215519 100644
> >>> --- a/virt/kvm/arm/arm.c
> >>> +++ b/virt/kvm/arm/arm.c
> >>> @@ -249,6 +249,21 @@ long kvm_arch_dev_ioctl(struct file *filp,
> >>> return -EINVAL;
> >>> }
> >>>
> >>> +struct kvm *kvm_arch_alloc_vm(void)
> >>> +{
> >>> + if (!has_vhe())
> >>> + return kzalloc(sizeof(struct kvm), GFP_KERNEL);
> >>> +
> >>> + return vzalloc(sizeof(struct kvm));
> >>> +}
> >>> +
> >>> +void kvm_arch_free_vm(struct kvm *kvm)
> >>> +{
> >>> + if (!has_vhe())
> >>> + kfree(kvm);
> >>> + else
> >>> + vfree(kvm);
> >>> +}
> >>>
> >>> struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int
id)
> >>> {
> >>>
> >>> Thanks,
> >>>
> >>> M.
> >>>
> >
> >> Sounds good then.
> >
> >> Paolo
> --
> Jazz is not dead. It just smells funny...
