Re: [RFC][PATCH 1/5] [PATCH 1/5] kvm: register in task_struct

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 09/04/2018 09:15 AM, Fengguang Wu wrote:
> On Tue, Sep 04, 2018 at 08:37:03AM +0200, Nikita Leshenko wrote:
>> On 4 Sep 2018, at 2:46, Fengguang Wu <fengguang.wu@intel.com> wrote:
>>>
>>> Here it goes:
>>>
>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>> index 99ce070e7dcb..27c5446f3deb 100644
>>> --- a/include/linux/mm_types.h
>>> +++ b/include/linux/mm_types.h
>>> @@ -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
>>> @@ -489,10 +490,19 @@ struct mm_struct {
>>> A A A A /* HMM needs to track a few things per mm */
>>> A A A A struct hmm *hmm;
>>> #endif
>>> +#if IS_ENABLED(CONFIG_KVM)
>>> +A A A  struct kvm *kvm;
>>> +#endif
>>> } __randomize_layout;
>>> 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
>>> +
>>> static inline void mm_init_cpumask(struct mm_struct *mm)
>>> {
>>> #ifdef CONFIG_CPUMASK_OFFSTACK
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 0c483720de8d..dca6156a7b35 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -3892,7 +3892,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
>>> A A A A if (type == KVM_EVENT_CREATE_VM) {
>>> A A A A A A A  add_uevent_var(env, "EVENT=create");
>>> A A A A A A A  kvm->userspace_pid = task_pid_nr(current);
>>> -A A A A A A A  current->kvm = kvm;
>>> +A A A A A A A  current->mm->kvm = kvm;
>> I think you also need to reset kvm to NULL once the VM is
>> destroyed, otherwise it would point to dangling memory.
> 
> Good point! Here is the incremental patch:
> 
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3894,6 +3894,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
> A A A A A A A A A A A A A A  kvm->userspace_pid = task_pid_nr(current);
> A A A A A A A A A A A A A A  current->mm->kvm = kvm;
> A A A A A A  } else if (type == KVM_EVENT_DESTROY_VM) {
> +A A A A A A A A A A A A A A  current->mm->kvm = NULL;
> A A A A A A A A A A A A A A  add_uevent_var(env, "EVENT=destroy");
> A A A A A A  }
> A A A A A A  add_uevent_var(env, "PID=%d", kvm->userspace_pid);

I think you should put both code snippets somewhere else. This has probably nothing to do
with the uevent. Instead this should go into kvm_destroy_vm and kvm_create_vm. Make sure
to take care of the error handling.

Can you point us to the original discussion about the why and what you are
trying to achieve?