Re: [PATCH v4 05/13] KVM: arm64: Setup a framework for hypercall bitmap firmware registers

[Oliver Upton <oupton@google.com>]

On Tue, Mar 15, 2022 at 09:59:35AM -0700, Raghavendra Rao Ananta wrote:
> On Tue, Mar 15, 2022 at 12:25 AM Oliver Upton <oupton@google.com> wrote:
> >
> > On Mon, Mar 14, 2022 at 05:22:31PM -0700, Raghavendra Rao Ananta wrote:
> > > Hi Oliver,
> > >
> > > On Mon, Mar 14, 2022 at 12:41 PM Oliver Upton <oupton@google.com> wrote:
> > > >
> > > > On Thu, Feb 24, 2022 at 05:25:51PM +0000, Raghavendra Rao Ananta wrote:
> > > > > KVM regularly introduces new hypercall services to the guests without
> > > > > any consent from the userspace. This means, the guests can observe
> > > > > hypercall services in and out as they migrate across various host
> > > > > kernel versions. This could be a major problem if the guest
> > > > > discovered a hypercall, started using it, and after getting migrated
> > > > > to an older kernel realizes that it's no longer available. Depending
> > > > > on how the guest handles the change, there's a potential chance that
> > > > > the guest would just panic.
> > > > >
> > > > > As a result, there's a need for the userspace to elect the services
> > > > > that it wishes the guest to discover. It can elect these services
> > > > > based on the kernels spread across its (migration) fleet. To remedy
> > > > > this, extend the existing firmware psuedo-registers, such as
> > > > > KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.
> > > > >
> > > > > These firmware registers are categorized based on the service call
> > > > > owners, and unlike the existing firmware psuedo-registers, they hold
> > > > > the features supported in the form of a bitmap.
> > > > >
> > > > > During the VM initialization, the registers holds an upper-limit of
> > > > > the features supported by the corresponding registers. It's expected
> > > > > that the VMMs discover the features provided by each register via
> > > > > GET_ONE_REG, and writeback the desired values using SET_ONE_REG.
> > > > > KVM allows this modification only until the VM has started.
> > > > >
> > > > > Older userspace code can simply ignore the capability and the
> > > > > hypercall services will be exposed unconditionally to the guests, thus
> > > > > ensuring backward compatibility.
> > > > >
> > > > > In this patch, the framework adds the register only for ARM's standard
> > > > > secure services (owner value 4). Currently, this includes support only
> > > > > for ARM True Random Number Generator (TRNG) service, with bit-0 of the
> > > > > register representing mandatory features of v1.0. The register is also
> > > > > added to the kvm_arm_vm_scope_fw_regs[] list as it maintains its state
> > > > > per-VM. Other services are momentarily added in the upcoming patches.
> > > > >
> > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
> > > > > ---
> > > > >  arch/arm64/include/asm/kvm_host.h | 12 +++++
> > > > >  arch/arm64/include/uapi/asm/kvm.h |  8 ++++
> > > > >  arch/arm64/kvm/arm.c              |  8 ++++
> > > > >  arch/arm64/kvm/guest.c            |  1 +
> > > > >  arch/arm64/kvm/hypercalls.c       | 78 +++++++++++++++++++++++++++++++
> > > > >  include/kvm/arm_hypercalls.h      |  4 ++
> > > > >  6 files changed, 111 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > > index e823571e50cc..1909ced3208f 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -101,6 +101,15 @@ struct kvm_s2_mmu {
> > > > >  struct kvm_arch_memory_slot {
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * struct kvm_hvc_desc: KVM ARM64 hypercall descriptor
> > > > > + *
> > > > > + * @hvc_std_bmap: Bitmap of standard secure service calls
> > > > > + */
> > > > > +struct kvm_hvc_desc {
> > > >
> > > > nit: maybe call this structure kvm_hypercall_features? When nested comes
> > > > along guests will need to use the SVC conduit as HVC traps are always
> > > > taken to EL2. Same will need to be true for virtual EL2.
> > > >
> > > Sure, I can rename it to be more generic.
> > >
> > > > > +     u64 hvc_std_bmap;
> > > > > +};
> > > > > +
> > > > >  struct kvm_arch {
> > > > >       struct kvm_s2_mmu mmu;
> > > > >
> > > > > @@ -142,6 +151,9 @@ struct kvm_arch {
> > > > >
> > > > >       /* Capture first run of the VM */
> > > > >       bool has_run_once;
> > > > > +
> > > > > +     /* Hypercall firmware register' descriptor */
> > > > > +     struct kvm_hvc_desc hvc_desc;
> > > > >  };
> > > > >
> > > > >  struct kvm_vcpu_fault_info {
> > > > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > > > > index c35447cc0e0c..2decc30d6b84 100644
> > > > > --- a/arch/arm64/include/uapi/asm/kvm.h
> > > > > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > > > > @@ -287,6 +287,14 @@ struct kvm_arm_copy_mte_tags {
> > > > >  #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED     3
> > > > >  #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED          (1U << 4)
> > > > >
> > > > > +/* Bitmap firmware registers, extension to the existing psuedo-register space */
> > > > > +#define KVM_REG_ARM_FW_BMAP                  KVM_REG_ARM_FW_REG(0xff00)
> > > >
> > > > What is the motivation for moving the bitmap register indices so far
> > > > away from the rest of the firmware regs?
> > > >
> > > The original motivation to create a sub-space came from Reiji's
> > > comment on v3 [1] so that user-space can distinguish between bitmapped
> > > and regular fw registers.
> > > As with the spacing, I thought a 50/50 split would do a good job of
> > > avoiding collisions. Do you have any recommendations here?
> > >
> >
> > I see. This is for the sake of ABI stability with future expansion,
> > right? A new register could be added in the future that controls more
> > SMCCC features, and we expect userspace to zero them if it cares about
> > ABI stability.
> >
> > If that is all true, we probably need some strong supporting
> > documentation. Additionally, using a new COPROC value for the register
> > range might be better than partitioning the existing FW reg range.
> >
> I assumed the 50/50 split could be fine even for future expansion, but
> I can go for a new COPROC value. However, wouldn't the same problem
> exist even with that? We could never have enough space :)

Of course, but I think the UAPI is consistent if you use a new COPROC
value for the bitmaps. That way, you can add documentation that covers
the entire COPROC value you've selected, and doesn't require any further
twiddling with an existing register range. It seems that we have plenty
of COPROC values that are available as well.

> > > > > +#define KVM_REG_ARM_FW_BMAP_REG(r)           (KVM_REG_ARM_FW_BMAP | (r))
> > > >
> > > > If you are still going to use the index offset, just pass 'r' through to
> > > > the other macro:
> > > >
> > > >   #define KVM_REG_ARM_FW_BMAP_REG(r)            KVM_REG_ARM_FW_REG(0xff00 + r)
> > > >
> > > I'm sorry, what's the advantage of doing this?
> > >

Just a style nit :)

--
Thanks,
Oliver