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