On Mon, Jan 10, 2022 at 4:04 PM Reiji Watanabe [off-list ref] wrote:

On Fri, Jan 7, 2022 at 3:43 PM Raghavendra Rao Ananta
[off-list ref] wrote:

quoted

Hi Reiji,

On Thu, Jan 6, 2022 at 10:07 PM Reiji Watanabe [off-list ref] wrote:

quoted

Hi Raghu,

On Tue, Jan 4, 2022 at 11:49 AM Raghavendra Rao Ananta
[off-list ref] wrote:

quoted

Capture the start of the KVM VM, which is basically the
start of any vCPU run. This state of the VM is helpful
in the upcoming patches to prevent user-space from
configuring certain VM features after the VM has started
running.

Signed-off-by: Raghavendra Rao Ananta <redacted>
---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c310648cc8f1..d0bd8f7a026c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h

@@ -623,6 +623,7 @@ struct kvm {
        struct notifier_block pm_notifier;
 #endif
        char stats_id[KVM_STATS_NAME_SIZE];
+       bool vm_started;

Since KVM_RUN on any vCPUs doesn't necessarily mean that the VM
started yet, the name might be a bit misleading IMHO.  I would
think 'has_run_once' or 'ran_once' might be more clear (?).

I always struggle with the names; but if you feel that 'ran_once'
makes more sense for a reader, I can change it.

I would prefer 'ran_once'.

Yes, that makes sense. I think the name created a lot of confusion for
the patch.

Thanks,
Raghavendra

quoted

quoted

quoted

 };

 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
        }
 }

+#define kvm_vm_has_started(kvm) (kvm->vm_started)
+
 extern bool kvm_rebooting;

 extern unsigned int halt_poll_ns;

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 72c4e6b39389..962b91ac2064 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c

@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
        int r;
        struct kvm_fpu *fpu = NULL;
        struct kvm_sregs *kvm_sregs = NULL;
+       struct kvm *kvm = vcpu->kvm;

        if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
                return -EIO;

@@ -3723,6 +3724,14 @@ static long kvm_vcpu_ioctl(struct file *filp,
                        if (oldpid)
                                synchronize_rcu();
                        put_pid(oldpid);
+
+                       /*
+                        * Since we land here even on the first vCPU run,
+                        * we can mark that the VM has started running.
+                        */

It might be nicer to add a comment why the code below gets kvm->lock.

I've been going back and forth on this one. Initially I considered
simply going with atomic_t, but the patch 4/11 (KVM: arm64: Setup a
framework for hypercall bitmap firmware registers)
kvm_arm_set_fw_reg_bmap()'s implementation felt like we need a lock to
have the whole 'is the register busy?' operation atomic. But, that's
just one of the applications.

I understand why you need the code to get the lock here with the
current implementation.
But, since the code just set the one field (vm_started) with the lock,
I thought the intention of getting the lock might not be so obvious.
(But, maybe clear enough looking at the code in the patch-4)

Thanks,
Reiji

quoted

quoted

Anyway, the patch generally looks good to me, and thank you
for making this change (it works for my purpose as well).

Reviewed-by: Reiji Watanabe <redacted>

Glad that it's helping you as well and thanks for the review.

Regards,
Raghavendra

quoted

Thanks,
Reiji

quoted

+                       mutex_lock(&kvm->lock);
+                       kvm->vm_started = true;
+                       mutex_unlock(&kvm->lock);
                }
                r = kvm_arch_vcpu_ioctl_run(vcpu);
                trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--
2.34.1.448.ga2b2bfdf31-goog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel