Message-ID: <YhkfW76Rit6DYs9T@google.com>
Date:   Fri, 25 Feb 2022 18:26:35 +0000
From:   Oliver Upton <oupton@...gle.com>
To:     Raghavendra Rao Ananta <rananta@...gle.com>
Cc:     Marc Zyngier <maz@...nel.org>, Andrew Jones <drjones@...hat.com>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Peter Shier <pshier@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>,
        Reiji Watanabe <reijiw@...gle.com>,
        Jing Zhang <jingzhangos@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v4 02/13] KVM: arm64: Introduce KVM_CAP_ARM_REG_SCOPE

On Fri, Feb 25, 2022 at 09:34:35AM -0800, Raghavendra Rao Ananta wrote:
> Hey Oliver,
> 
> On Thu, Feb 24, 2022 at 10:43 PM Oliver Upton <oupton@...gle.com> wrote:
> >
> > On Thu, Feb 24, 2022 at 05:25:48PM +0000, Raghavendra Rao Ananta wrote:
> > > KVM_[GET|SET]_ONE_REG act on per-vCPU basis. Currently certain
> > > ARM64 registers, such as KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_[1|2],
> > > are accessed via this interface even though the effect that
> > > they have are really per-VM. As a result, userspace could just
> > > waste cycles to read/write the same information for every vCPU
> > > that it spawns, only to realize that there's absolutely no change
> > > in the VM's state. The problem gets worse in proportion to the
> > > number of vCPUs created.
> > >
> > > As a result, to avoid this redundancy, introduce the capability
> > > KVM_CAP_ARM_REG_SCOPE. If enabled, KVM_GET_REG_LIST will advertise
> > > the registers that are VM-scoped by dynamically modifying the
> > > register encoding. KVM_REG_ARM_SCOPE_* helper macros are introduced
> > > to decode the same. By learning this, userspace can access such
> > > registers only once.
> > >
> > > Signed-off-by: Raghavendra Rao Ananta <rananta@...gle.com>
> > > ---
> > >  Documentation/virt/kvm/api.rst    | 16 ++++++++++++++++
> > >  arch/arm64/include/asm/kvm_host.h |  3 +++
> > >  arch/arm64/include/uapi/asm/kvm.h |  6 ++++++
> > >  arch/arm64/kvm/arm.c              | 13 +++++++------
> > >  include/uapi/linux/kvm.h          |  1 +
> > >  5 files changed, 33 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > > index a4267104db50..7e7b3439f540 100644
> > > --- a/Documentation/virt/kvm/api.rst
> > > +++ b/Documentation/virt/kvm/api.rst
> > > @@ -7561,3 +7561,19 @@ The argument to KVM_ENABLE_CAP is also a bitmask, and must be a subset
> > >  of the result of KVM_CHECK_EXTENSION.  KVM will forward to userspace
> > >  the hypercalls whose corresponding bit is in the argument, and return
> > >  ENOSYS for the others.
> > > +
> > > +8.34 KVM_CAP_ARM_REG_SCOPE
> > > +--------------------------
> > > +
> > > +:Architectures: arm64
> > > +
> > > +The capability, if enabled, amends the existing register encoding
> > > +with additional information to the userspace if a particular register
> > > +is scoped per-vCPU or per-VM via KVM_GET_REG_LIST. KVM provides
> > > +KVM_REG_ARM_SCOPE_* helper macros to decode the same. Userspace can
> > > +use this information from the register encoding to access a VM-scopped
> > > +regiser only once, as opposed to accessing it for every vCPU for the
> > > +same effect.
> > > +
> >
> > Could you describe the encoding changes in 4.68 'KVM_SET_ONE_REG', along
> > with the other ARM encoding details?
> >
> > > +On the other hand, if the capability is disabled, all the registers
> > > +remain vCPU-scopped by default, retaining backward compatibility.
> >
> > typo: vCPU-scoped
> >
> > That said, I don't believe we need to document behavior if the CAP is
> > disabled, as the implicated ioctls should continue to work the same.
> >
> Sure, I'll address the above two Doc comments.
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index 5bc01e62c08a..8132de6bd718 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -136,6 +136,9 @@ struct kvm_arch {
> > >
> > >       /* Memory Tagging Extension enabled for the guest */
> > >       bool mte_enabled;
> > > +
> > > +     /* Register scoping enabled for KVM registers */
> > > +     bool reg_scope_enabled;
> > >  };
> > >
> > >  struct kvm_vcpu_fault_info {
> > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > > index b3edde68bc3e..c35447cc0e0c 100644
> > > --- a/arch/arm64/include/uapi/asm/kvm.h
> > > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > > @@ -199,6 +199,12 @@ struct kvm_arm_copy_mte_tags {
> > >  #define KVM_REG_ARM_COPROC_MASK              0x000000000FFF0000
> > >  #define KVM_REG_ARM_COPROC_SHIFT     16
> > >
> > > +/* Defines if a KVM register is one per-vCPU or one per-VM */
> > > +#define KVM_REG_ARM_SCOPE_MASK               0x0000000010000000
> > > +#define KVM_REG_ARM_SCOPE_SHIFT              28
> >
> > Thinking about the advertisement of VM- and vCPU-scoped registers, this
> > could be generally useful. Might it make sense to add such an encoding
> > to the arch-generic register definitions?
> >
> > If that is the case, we may want to snap up a few more bits (a nybble)
> > for future expansion.
> >
> That's a great idea! But I wonder if we'll get a push-back since there
> are no users of it in other arch(s) yet. Not sure if there was any
> need/discussion regarding the same, but I'm happy to share a patch for
> the same if you sense that there's a strong potential for the patch.
> 

I'm unsure if this is actually of interest to other architectures, it
just doesn't seem ARM-specific so we should probably raise the question
so we only grab these bits once.

> > > +#define KVM_REG_ARM_SCOPE_VCPU               0
> > > +#define KVM_REG_ARM_SCOPE_VM         1
> > > +
> > >  /* Normal registers are mapped as coprocessor 16. */
> > >  #define KVM_REG_ARM_CORE             (0x0010 << KVM_REG_ARM_COPROC_SHIFT)
> > >  #define KVM_REG_ARM_CORE_REG(name)   (offsetof(struct kvm_regs, name) / sizeof(__u32))
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index ecc5958e27fe..107977c82c6c 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -81,26 +81,26 @@ int kvm_arch_check_processor_compat(void *opaque)
> > >  int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> > >                           struct kvm_enable_cap *cap)
> > >  {
> > > -     int r;
> > > +     int r = 0;
> > >
> > >       if (cap->flags)
> > >               return -EINVAL;
> > >
> > >       switch (cap->cap) {
> > >       case KVM_CAP_ARM_NISV_TO_USER:
> > > -             r = 0;
> > >               kvm->arch.return_nisv_io_abort_to_user = true;
> > >               break;
> > >       case KVM_CAP_ARM_MTE:
> > >               mutex_lock(&kvm->lock);
> > > -             if (!system_supports_mte() || kvm->created_vcpus) {
> > > +             if (!system_supports_mte() || kvm->created_vcpus)
> > >                       r = -EINVAL;
> > > -             } else {
> > > -                     r = 0;
> > > +             else
> > >                       kvm->arch.mte_enabled = true;
> > > -             }
> > >               mutex_unlock(&kvm->lock);
> > >               break;
> >
> > Hmm.. these all look like cleanups. If you want to propose these, could
> > you do it in a separate patch?
> >
> Ahh, I thought I could squeeze it in. But sure, I can separate it out.
> > > +     case KVM_CAP_ARM_REG_SCOPE:
> > > +             WRITE_ONCE(kvm->arch.reg_scope_enabled, true);
> > > +             break;
> > >       default:
> > >               r = -EINVAL;
> > >               break;
> > > @@ -209,6 +209,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >       case KVM_CAP_SET_GUEST_DEBUG:
> > >       case KVM_CAP_VCPU_ATTRIBUTES:
> > >       case KVM_CAP_PTP_KVM:
> > > +     case KVM_CAP_ARM_REG_SCOPE:
> >
> > It is a bit odd to advertise a capability (and allow userspace to enable
> > it), despite the fact that the feature itself hasn't yet been
> > implemented.
> >
> > Is it possible to fold the feature in to the patch that exposes it to
> > userspace? Otherwise, you could punt advertisement of the CAP until it
> > is actually implemented in kernel.
> >
> Well, I didn't want to complicate the patch, but technically the
> feature is available with this patch, including all the CAP and macro
> definitions. Userspace can still decode the scope information, only
> that no registers are added yet, which is done in the next patch. So,
> the userspace can still remain the same between this and the next
> patch.

But the series isn't cleanly bisectable. There will exist commits in
history that report KVM_CAP_ARM_REG_SCOPE as implemented even though
that is not actually the case. You should really only advertise support
to userspace when the feature is implemented.

Defining kvm->arch.reg_scope_enabled can be done earlier so you have a
bit to test and guard all of the new code, and only expose the CAP in
the last patch of the series.

Also, as an FYI Marc has a patch that I'll be picking up in my own
series which uses bits instead of bools to keep track of certain
VM-wide features:

https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/mmu/guest-MMIO-guard&id=7dd0a13a4217b870f2e83cdc6045e5ce482a5340

Marc, if neither of our series land in 5.18 could you at least submit
this patch in preparation? Should keep conflicts minimal that way.

Thanks!

--
Oliver

Powered by blists - more mailing lists