Date:   Tue, 11 Jan 2022 17:36:25 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Raghavendra Rao Ananta <rananta@google.com>
Cc:     Marc Zyngier <maz@kernel.org>, Andrew Jones <drjones@redhat.com>,
        James Morse <james.morse@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        kvm@vger.kernel.org, Catalin Marinas <catalin.marinas@arm.com>,
        Peter Shier <pshier@google.com>, linux-kernel@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Will Deacon <will@kernel.org>, kvmarm@lists.cs.columbia.edu,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start
Message-ID: <Yd3AGRtkBgWSmGf2@google.com>
References: <20220104194918.373612-1-rananta@google.com>
 <20220104194918.373612-2-rananta@google.com>
 <Ydjje8qBOP3zDOZi@google.com>
 <CAJHc60ziKv6P4ZmpLXrv+s4DrrDtOwuQRAc4bKcrbR3aNAK5mQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJHc60ziKv6P4ZmpLXrv+s4DrrDtOwuQRAc4bKcrbR3aNAK5mQ@mail.gmail.com>
Precedence: bulk

On Mon, Jan 10, 2022, Raghavendra Rao Ananta wrote:
> On Fri, Jan 7, 2022 at 5:06 PM Sean Christopherson <seanjc@google.com> 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.

> > > +                      */
> > > +                     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).

> > > +                     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.