On Wed, Apr 17, 2013 at 08:24:21AM -0700, Andrew Honig wrote: > I'm happy to not add a new API and use __kvm_set_memory_region to > unregister private memory regions, but I thought chaning the API was > the approach you asked for when I sent a previous patch. See the end > of: http://article.gmane.org/gmane.comp.emulators.kvm.devel/107753 > > Did I misunderstand your comment from 8 April? > Ugh, yes. My "Please send a second version" was in response to your question "or should I send a second version of this current patch?" Where second version is the same as the first one but uses __kvm_set_memory_region() for slot deletion. I am sorry about the confusion :( > On Wed, Apr 17, 2013 at 6:10 AM, Gleb Natapov <gleb@xxxxxxxxxx> wrote: > > On Mon, Apr 15, 2013 at 03:10:32PM -0700, Andrew Honig wrote: > >> > >> The motivation for this patch is to fix a 20KB leak of memory in vmx.c > >> when a VM is created and destroyed. > >> > >> On x86/vmx platforms KVM needs 5 pages of userspace memory per VM for > >> architecture specific reasons. It currently allocates the pages on behalf > >> of user space, but has no way of cleanly freeing that memory while the > >> user space process is still running. For user space processes that want > >> more control over that memory, this patch allows user space to provide the > >> memory that KVM uses. > >> > >> Signed-off-by: Andrew Honig <ahonig@xxxxxxxxxx> > > I thought we agreed on not adding new API and using > > __kvm_set_memory_region() to unregister private memory regions. > > > > > >> --- > >> Documentation/virtual/kvm/api.txt | 8 +++++++ > >> arch/arm/kvm/arm.c | 6 +++++ > >> arch/ia64/kvm/kvm-ia64.c | 6 +++++ > >> arch/powerpc/kvm/powerpc.c | 6 +++++ > >> arch/s390/kvm/kvm-s390.c | 6 +++++ > >> arch/x86/include/asm/kvm_host.h | 7 ++++++ > >> arch/x86/kvm/svm.c | 8 +++++++ > >> arch/x86/kvm/vmx.c | 47 ++++++++++++++++++++++++++++++++++--- > >> arch/x86/kvm/x86.c | 12 ++++++++-- > >> include/linux/kvm_host.h | 2 ++ > >> virt/kvm/kvm_main.c | 2 ++ > >> 11 files changed, 105 insertions(+), 5 deletions(-) > >> > >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt > >> index 119358d..aa18cac 100644 > >> --- a/Documentation/virtual/kvm/api.txt > >> +++ b/Documentation/virtual/kvm/api.txt > >> @@ -879,6 +879,14 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr > >> be identical. This allows large pages in the guest to be backed by large > >> pages in the host. > >> > >> +On x86/vmx architectures KVM needs 5 pages of user space memory for architecture > >> +specific reasons. Calling this ioctl with the special memslot > >> +KVM_PRIVATE_MEMORY_MEMSLOT will tell kvm which user space memory to use for > >> +that memory. If that memslot is not set before creating VCPUs for a VM then > >> +kvm will allocate the memory on behalf of user space, but userspace will not > >> +be able to free that memory. User space should treat this memory as opaque > >> +and not modify it. > >> + > >> The flags field supports two flags: KVM_MEM_LOG_DIRTY_PAGES and > >> KVM_MEM_READONLY. The former can be set to instruct KVM to keep track of > >> writes to memory within the slot. See KVM_GET_DIRTY_LOG ioctl to know how to > >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > >> index 5a93698..ac52f14 100644 > >> --- a/arch/arm/kvm/arm.c > >> +++ b/arch/arm/kvm/arm.c > >> @@ -228,6 +228,12 @@ int kvm_arch_set_memory_region(struct kvm *kvm, > >> return 0; > >> } > >> > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return 0; > >> +} > >> + > >> int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> struct kvm_memory_slot *memslot, > >> struct kvm_memory_slot old, > >> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > >> index ad3126a..570dd97 100644 > >> --- a/arch/ia64/kvm/kvm-ia64.c > >> +++ b/arch/ia64/kvm/kvm-ia64.c > >> @@ -1576,6 +1576,12 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages) > >> return 0; > >> } > >> > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return 0; > >> +} > >> + > >> int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> struct kvm_memory_slot *memslot, > >> struct kvm_memory_slot old, > >> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > >> index 934413c..6e3843b 100644 > >> --- a/arch/powerpc/kvm/powerpc.c > >> +++ b/arch/powerpc/kvm/powerpc.c > >> @@ -410,6 +410,12 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages) > >> return kvmppc_core_create_memslot(slot, npages); > >> } > >> > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return 0; > >> +} > >> + > >> int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> struct kvm_memory_slot *memslot, > >> struct kvm_memory_slot old, > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index 4cf35a0..a97f495 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -971,6 +971,12 @@ int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages) > >> return 0; > >> } > >> > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return 0; > >> +} > >> + > >> /* Section: memory related */ > >> int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> struct kvm_memory_slot *memslot, > >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > >> index 4979778..7215817 100644 > >> --- a/arch/x86/include/asm/kvm_host.h > >> +++ b/arch/x86/include/asm/kvm_host.h > >> @@ -37,6 +37,7 @@ > >> /* memory slots that are not exposed to userspace */ > >> #define KVM_PRIVATE_MEM_SLOTS 3 > >> #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS) > >> +#define KVM_PRIVATE_MEMORY_MEMSLOT 0x80000001 > >> > >> #define KVM_MMIO_SIZE 16 > >> > >> @@ -553,6 +554,9 @@ struct kvm_arch { > >> struct page *ept_identity_pagetable; > >> bool ept_identity_pagetable_done; > >> gpa_t ept_identity_map_addr; > >> + unsigned long ept_ptr; > >> + unsigned long apic_ptr; > >> + unsigned long tss_ptr; > >> > >> unsigned long irq_sources_bitmap; > >> s64 kvmclock_offset; > >> @@ -640,6 +644,9 @@ struct kvm_x86_ops { > >> bool (*cpu_has_accelerated_tpr)(void); > >> void (*cpuid_update)(struct kvm_vcpu *vcpu); > >> > >> + int (*set_private_memory)(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem); > >> + > >> /* Create, but do not attach this VCPU */ > >> struct kvm_vcpu *(*vcpu_create)(struct kvm *kvm, unsigned id); > >> void (*vcpu_free)(struct kvm_vcpu *vcpu); > >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > >> index e1b1ce2..3cc4e56 100644 > >> --- a/arch/x86/kvm/svm.c > >> +++ b/arch/x86/kvm/svm.c > >> @@ -1211,6 +1211,12 @@ static int svm_vcpu_reset(struct kvm_vcpu *vcpu) > >> return 0; > >> } > >> > >> +static int svm_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return 0; > >> +} > >> + > >> static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) > >> { > >> struct vcpu_svm *svm; > >> @@ -4257,6 +4263,8 @@ static struct kvm_x86_ops svm_x86_ops = { > >> .hardware_disable = svm_hardware_disable, > >> .cpu_has_accelerated_tpr = svm_cpu_has_accelerated_tpr, > >> > >> + .set_private_memory = svm_set_private_memory, > >> + > >> .vcpu_create = svm_create_vcpu, > >> .vcpu_free = svm_free_vcpu, > >> .vcpu_reset = svm_vcpu_reset, > >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> index 6667042..796ac07 100644 > >> --- a/arch/x86/kvm/vmx.c > >> +++ b/arch/x86/kvm/vmx.c > >> @@ -3692,7 +3692,13 @@ static int alloc_apic_access_page(struct kvm *kvm) > >> kvm_userspace_mem.flags = 0; > >> kvm_userspace_mem.guest_phys_addr = 0xfee00000ULL; > >> kvm_userspace_mem.memory_size = PAGE_SIZE; > >> - r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false); > >> + if (kvm->arch.apic_ptr) { > >> + kvm_userspace_mem.userspace_addr = kvm->arch.apic_ptr; > >> + r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, true); > >> + } else { > >> + r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false); > >> + } > >> + > >> if (r) > >> goto out; > >> > >> @@ -3722,7 +3728,13 @@ static int alloc_identity_pagetable(struct kvm *kvm) > >> kvm_userspace_mem.guest_phys_addr = > >> kvm->arch.ept_identity_map_addr; > >> kvm_userspace_mem.memory_size = PAGE_SIZE; > >> - r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false); > >> + if (kvm->arch.ept_ptr) { > >> + kvm_userspace_mem.userspace_addr = kvm->arch.ept_ptr; > >> + r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, true); > >> + } else { > >> + r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false); > >> + } > >> + > >> if (r) > >> goto out; > >> > >> @@ -4362,7 +4374,13 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr) > >> .flags = 0, > >> }; > >> > >> - ret = kvm_set_memory_region(kvm, &tss_mem, false); > >> + if (kvm->arch.tss_ptr) { > >> + tss_mem.userspace_addr = kvm->arch.tss_ptr; > >> + ret = kvm_set_memory_region(kvm, &tss_mem, true); > >> + } else { > >> + ret = kvm_set_memory_region(kvm, &tss_mem, false); > >> + } > >> + > >> if (ret) > >> return ret; > >> kvm->arch.tss_addr = addr; > >> @@ -6683,6 +6701,27 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu) > >> vmx_complete_interrupts(vmx); > >> } > >> > >> +static int vmx_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + /* > >> + * Early sanity checking so userspace gets an error message during > >> + * memory setup and not when trying to use this memory. > >> + * Checks to see if the memory is valid are performed later when > >> + * the memory is used. > >> + */ > >> + if (!mem->userspace_addr || mem->userspace_addr & (PAGE_SIZE - 1) || > >> + mem->memory_size & (PAGE_SIZE - 1) || > >> + mem->memory_size < PAGE_SIZE * 5) > >> + return -EINVAL; > >> + > >> + kvm->arch.ept_ptr = mem->userspace_addr; > >> + kvm->arch.apic_ptr = mem->userspace_addr + PAGE_SIZE; > >> + kvm->arch.tss_ptr = mem->userspace_addr + PAGE_SIZE * 2; > >> + > >> + return 0; > >> +} > >> + > >> static void vmx_free_vcpu(struct kvm_vcpu *vcpu) > >> { > >> struct vcpu_vmx *vmx = to_vmx(vcpu); > >> @@ -7532,6 +7571,8 @@ static struct kvm_x86_ops vmx_x86_ops = { > >> .hardware_disable = hardware_disable, > >> .cpu_has_accelerated_tpr = report_flexpriority, > >> > >> + .set_private_memory = vmx_set_private_memory, > >> + > >> .vcpu_create = vmx_create_vcpu, > >> .vcpu_free = vmx_free_vcpu, > >> .vcpu_reset = vmx_vcpu_reset, > >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >> index e172132..7045d0a 100644 > >> --- a/arch/x86/kvm/x86.c > >> +++ b/arch/x86/kvm/x86.c > >> @@ -6809,6 +6809,12 @@ void kvm_arch_sync_events(struct kvm *kvm) > >> kvm_free_pit(kvm); > >> } > >> > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem) > >> +{ > >> + return kvm_x86_ops->set_private_memory(kvm, mem); > >> +} > >> + > >> void kvm_arch_destroy_vm(struct kvm *kvm) > >> { > >> kvm_iommu_unmap_guest(kvm); > >> @@ -6913,7 +6919,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> * Only private memory slots need to be mapped here since > >> * KVM_SET_MEMORY_REGION ioctl is no longer supported. > >> */ > >> - if ((memslot->id >= KVM_USER_MEM_SLOTS) && npages && !old.npages) { > >> + if ((memslot->id >= KVM_USER_MEM_SLOTS) && npages && !old.npages && > >> + !user_alloc) { > >> unsigned long userspace_addr; > >> > >> /* > >> @@ -6941,7 +6948,8 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, > >> > >> int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT; > >> > >> - if ((mem->slot >= KVM_USER_MEM_SLOTS) && old.npages && !npages) { > >> + if ((mem->slot >= KVM_USER_MEM_SLOTS) && old.npages && !npages && > >> + !user_alloc) { > >> int ret; > >> > >> ret = vm_munmap(old.userspace_addr, > >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > >> index c139582..f441d1f 100644 > >> --- a/include/linux/kvm_host.h > >> +++ b/include/linux/kvm_host.h > >> @@ -461,6 +461,8 @@ int __kvm_set_memory_region(struct kvm *kvm, > >> void kvm_arch_free_memslot(struct kvm_memory_slot *free, > >> struct kvm_memory_slot *dont); > >> int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages); > >> +int kvm_arch_set_private_memory(struct kvm *kvm, > >> + struct kvm_userspace_memory_region *mem); > >> int kvm_arch_prepare_memory_region(struct kvm *kvm, > >> struct kvm_memory_slot *memslot, > >> struct kvm_memory_slot old, > >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > >> index f18013f..5372225 100644 > >> --- a/virt/kvm/kvm_main.c > >> +++ b/virt/kvm/kvm_main.c > >> @@ -949,6 +949,8 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, > >> kvm_userspace_memory_region *mem, > >> bool user_alloc) > >> { > >> + if (mem->slot == KVM_PRIVATE_MEMORY_MEMSLOT) > >> + return kvm_arch_set_private_memory(kvm, mem); > >> if (mem->slot >= KVM_USER_MEM_SLOTS) > >> return -EINVAL; > >> return kvm_set_memory_region(kvm, mem, user_alloc); > >> -- > >> 1.7.10.4 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe kvm" in > >> the body of a message to majordomo@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > > Gleb. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
