Sure, I can update the patch again. But I have a question first, what is
has_vhe() in the untested patch above?
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
