Re: [RFC PATCH v3 01/11] KVM: Capture VM start

[Raghavendra Rao Ananta]

On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Jan 10, 2022, Raghavendra Rao Ananta wrote:
> > On Fri, Jan 7, 2022 at 5:06 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Jan 04, 2022, Raghavendra Rao Ananta wrote:
> > > > +#define kvm_vm_has_started(kvm) (kvm->vm_started)
> > >
> > > Needs parantheses around (kvm), but why bother with a macro?  This is the same
> > > header that defines struct kvm.
> > >
> > No specific reason for creating a macro as such. I can remove it if it
> > feels noisy.
>
> Please do.  In the future, don't use a macro unless there's a good reason to do
> so.  Don't get me wrong, I love abusing macros, but for things like this they are
> completely inferior to
>
>   static inline bool kvm_vm_has_started(struct kvm *kvm)
>   {
>         return kvm->vm_started;
>   }
>
> because a helper function gives us type safety, doesn't suffer from concatenation
> of tokens potentially doing weird things, is easier to extend to a multi-line
> implementation, etc...
>
> An example of when it's ok to use a macro is x86's
>
>   #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)
>
> which uses a macro instead of a proper function to avoid a circular dependency
> due to arch/x86/include/asm/kvm_host.h being included by include/linux/kvm_host.h
> and thus x86's implementation of kvm_arch_vcpu_memslots_id() coming before the
> definition of struct kvm_vcpu.  But that's very much an exception and done only
> because the alternatives suck more.
>
Understood. Thanks for the explanation! Will switch to an inline function.

> > > > +                      */
> > > > +                     mutex_lock(&kvm->lock);
> > >
> > > This adds unnecessary lock contention when running vCPUs.  The naive solution
> > > would be:
> > >                         if (!kvm->vm_started) {
> > >                                 ...
> > >                         }
> > >
> > Not sure if I understood the solution..
>
> In your proposed patch, KVM_RUN will take kvm->lock _every_ time.  That introduces
> unnecessary contention as it will serialize this bit of code if multiple vCPUs
> are attempting KVM_RUN.  By checking !vm_started, only the "first" KVM_RUN for a
> VM will acquire kvm->lock and thus avoid contention once the VM is up and running.
> There's still a possibility that multiple vCPUs will contend for kvm->lock on their
> first KVM_RUN, hence the quotes.  I called it "naive" because it's possible there's
> a more elegant solution depending on the use case, e.g. a lockless approach might
> work (or it might not).
>
But is it safe to read kvm->vm_started without grabbing the lock in
the first place? use atomic_t maybe for this?

> > > > +                     kvm->vm_started = true;
> > > > +                     mutex_unlock(&kvm->lock);
> > >
> > > Lastly, why is this in generic KVM?
> > >
> > The v1 of the series originally had it in the arm specific code.
> > However, I was suggested to move it to the generic code since the book
> > keeping is not arch specific and could be helpful to others too [1].
>
> I'm definitely in favor of moving/adding thing to generic KVM when it makes sense,
> but I'm skeptical in this particular case.  The code _is_ arch specific in that
> arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g.
> versus a hypothetical x86 use case that might be completely ok with a lockless
> implementation.  And it's not obvious that there's a plausible, safe use case
> outside of arm64, e.g. on x86, there is very, very little that is truly shared
> across the entire VM/system, most things are per-thread/core/package in some way,
> shape, or form.  In other words, I'm a wary of providing something like this for
> x86 because odds are good that any use will be functionally incorrect.
I've been going back and forth on this. I've seen a couple of
variables declared in the generic struct and used only in the arch
code. vcpu->valid_wakeup for instance, which is used only by s390
arch. Maybe I'm looking at it the wrong way as to what can and can't
go in the generic kvm code.

Thanks,
Raghavendra