VM is associated with an address space and not a specific thread.
>From Documentation/virtual/kvm/api.txt:
Only run VM ioctls from the same process (address space) that was used
to create the VM.
CC: Nikita Leshenko <[email protected]>
CC: Christian Borntraeger <[email protected]>
Signed-off-by: Fengguang Wu <[email protected]>
---
include/linux/mm_types.h | 11 +++++++++++
virt/kvm/kvm_main.c | 3 +++
2 files changed, 14 insertions(+)
--- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
+++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
@@ -27,6 +27,7 @@ typedef int vm_fault_t;
struct address_space;
struct mem_cgroup;
struct hmm;
+struct kvm;
/*
* Each physical page in the system has a struct page associated with
@@ -496,6 +497,10 @@ struct mm_struct {
/* HMM needs to track a few things per mm */
struct hmm *hmm;
#endif
+
+#if IS_ENABLED(CONFIG_KVM)
+ struct kvm *kvm;
+#endif
} __randomize_layout;
/*
@@ -507,6 +512,12 @@ struct mm_struct {
extern struct mm_struct init_mm;
+#if IS_ENABLED(CONFIG_KVM)
+static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; }
+#else
+static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; }
+#endif
+
/* Pointer magic because the dynamic array size confuses some compilers. */
static inline void mm_init_cpumask(struct mm_struct *mm)
{
--- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
+++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
@@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k
struct mm_struct *mm = kvm->mm;
kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
+ mm->kvm = NULL;
kvm_destroy_vm_debugfs(kvm);
kvm_arch_sync_events(kvm);
spin_lock(&kvm_lock);
@@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig
fput(file);
return -ENOMEM;
}
+
+ kvm->mm->kvm = kvm;
kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
fd_install(r, file);
On Wed, Dec 26, 2018 at 09:15:00PM +0800, Fengguang Wu wrote:
> VM is associated with an address space and not a specific thread.
>
> >From Documentation/virtual/kvm/api.txt:
> Only run VM ioctls from the same process (address space) that was used
> to create the VM.
Hi, Fengguang,
AFAIU the commit message only explains why a kvm object needs to bind
to a single mm object (say, the reason why there is kvm->mm) however
not the reverse (say, the reason why there is mm->kvm), while the
latter is what this patch really needs?
I'm thinking whether it's legal for multiple VMs to run on a single mm
address space. I don't see a limitation so far but it's very possible
I am just missing something there (if there is, IMHO they might be
something nice to put into the commit message?). Thanks,
>
> CC: Nikita Leshenko <[email protected]>
> CC: Christian Borntraeger <[email protected]>
> Signed-off-by: Fengguang Wu <[email protected]>
> ---
> include/linux/mm_types.h | 11 +++++++++++
> virt/kvm/kvm_main.c | 3 +++
> 2 files changed, 14 insertions(+)
>
> --- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
> +++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
> @@ -27,6 +27,7 @@ typedef int vm_fault_t;
> struct address_space;
> struct mem_cgroup;
> struct hmm;
> +struct kvm;
>
> /*
> * Each physical page in the system has a struct page associated with
> @@ -496,6 +497,10 @@ struct mm_struct {
> /* HMM needs to track a few things per mm */
> struct hmm *hmm;
> #endif
> +
> +#if IS_ENABLED(CONFIG_KVM)
> + struct kvm *kvm;
> +#endif
> } __randomize_layout;
>
> /*
> @@ -507,6 +512,12 @@ struct mm_struct {
>
> extern struct mm_struct init_mm;
>
> +#if IS_ENABLED(CONFIG_KVM)
> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; }
> +#else
> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; }
> +#endif
> +
> /* Pointer magic because the dynamic array size confuses some compilers. */
> static inline void mm_init_cpumask(struct mm_struct *mm)
> {
> --- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
> +++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
> @@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k
> struct mm_struct *mm = kvm->mm;
>
> kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
> + mm->kvm = NULL;
> kvm_destroy_vm_debugfs(kvm);
> kvm_arch_sync_events(kvm);
> spin_lock(&kvm_lock);
> @@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig
> fput(file);
> return -ENOMEM;
> }
> +
> + kvm->mm->kvm = kvm;
> kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>
> fd_install(r, file);
>
>
Regards,
--
Peter Xu
Hi Peter,
On Sat, Feb 02, 2019 at 02:57:41PM +0800, Peter Xu wrote:
>On Wed, Dec 26, 2018 at 09:15:00PM +0800, Fengguang Wu wrote:
>> VM is associated with an address space and not a specific thread.
>>
>> >From Documentation/virtual/kvm/api.txt:
>> Only run VM ioctls from the same process (address space) that was used
>> to create the VM.
>
>Hi, Fengguang,
>
>AFAIU the commit message only explains why a kvm object needs to bind
>to a single mm object (say, the reason why there is kvm->mm) however
>not the reverse (say, the reason why there is mm->kvm), while the
>latter is what this patch really needs?
Yeah good point. The addition of mm->kvm makes code in this patchset
simple. However if that field is considered not general useful for
other possible users, and the added space overheads is a concern, we
can instead do with a flag (saying the mm is referenced by some KVM),
and add extra lookup code to find out the exact kvm instance.
>I'm thinking whether it's legal for multiple VMs to run on a single mm
>address space. I don't see a limitation so far but it's very possible
>I am just missing something there (if there is, IMHO they might be
>something nice to put into the commit message?). Thanks,
So far one QEMU only starts one KVM. I cannot think of any strong
benefit to start multiple KVMs in one single QEMU, so it may well
remain so in future. Anyway it's internal data structure instead of
API, which can adapt to possible future changes.
Thanks,
Fengguang
>> CC: Nikita Leshenko <[email protected]>
>> CC: Christian Borntraeger <[email protected]>
>> Signed-off-by: Fengguang Wu <[email protected]>
>> ---
>> include/linux/mm_types.h | 11 +++++++++++
>> virt/kvm/kvm_main.c | 3 +++
>> 2 files changed, 14 insertions(+)
>>
>> --- linux.orig/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
>> +++ linux/include/linux/mm_types.h 2018-12-23 19:58:06.993417137 +0800
>> @@ -27,6 +27,7 @@ typedef int vm_fault_t;
>> struct address_space;
>> struct mem_cgroup;
>> struct hmm;
>> +struct kvm;
>>
>> /*
>> * Each physical page in the system has a struct page associated with
>> @@ -496,6 +497,10 @@ struct mm_struct {
>> /* HMM needs to track a few things per mm */
>> struct hmm *hmm;
>> #endif
>> +
>> +#if IS_ENABLED(CONFIG_KVM)
>> + struct kvm *kvm;
>> +#endif
>> } __randomize_layout;
>>
>> /*
>> @@ -507,6 +512,12 @@ struct mm_struct {
>>
>> extern struct mm_struct init_mm;
>>
>> +#if IS_ENABLED(CONFIG_KVM)
>> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return mm->kvm; }
>> +#else
>> +static inline struct kvm *mm_kvm(struct mm_struct *mm) { return NULL; }
>> +#endif
>> +
>> /* Pointer magic because the dynamic array size confuses some compilers. */
>> static inline void mm_init_cpumask(struct mm_struct *mm)
>> {
>> --- linux.orig/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
>> +++ linux/virt/kvm/kvm_main.c 2018-12-23 19:58:06.993417137 +0800
>> @@ -727,6 +727,7 @@ static void kvm_destroy_vm(struct kvm *k
>> struct mm_struct *mm = kvm->mm;
>>
>> kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
>> + mm->kvm = NULL;
>> kvm_destroy_vm_debugfs(kvm);
>> kvm_arch_sync_events(kvm);
>> spin_lock(&kvm_lock);
>> @@ -3224,6 +3225,8 @@ static int kvm_dev_ioctl_create_vm(unsig
>> fput(file);
>> return -ENOMEM;
>> }
>> +
>> + kvm->mm->kvm = kvm;
>> kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>>
>> fd_install(r, file);
>>
>>
>
>Regards,
>
>--
>Peter Xu
>
On 02/02/19 07:57, Peter Xu wrote:
>
> I'm thinking whether it's legal for multiple VMs to run on a single mm
> address space. I don't see a limitation so far but it's very possible
> I am just missing something there (if there is, IMHO they might be
> something nice to put into the commit message?). Thanks,
Yes, it certainly is legal, and even useful in fact.
For example there are people running WebAssembly in a KVM sandbox. In
that case you can have multiple KVM instances in a single process.
It seems to me that there is already a perfect way to link an mm to its
users, which is the MMU notifier. Why do you need a separate
proc_ept_idle_operations? You could change ept_idle_read into an MMU
notifier callback, and have core mm/ core combine the output of
mm_idle_read and all the MMU notifiers? Basically, ept_idle_ctrl
becomes an argument to the new MMU notifier callback, or something like
that.
Paolo