On Mon, Mar 14, 2022 at 11:41 PM Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> On Mon, Mar 14, 2022 at 05:30:15PM -0700, Raghavendra Rao Ananta wrote:
> > On Mon, Mar 14, 2022 at 12:59 PM Oliver Upton <oupton@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Feb 24, 2022 at 05:25:53PM +0000, Raghavendra Rao Ananta wrote:
> > > > Introduce the firmware register to hold the vendor specific
> > > > hypervisor service calls (owner value 6) as a bitmap. The
> > > > bitmap represents the features that'll be enabled for the
> > > > guest, as configured by the user-space. Currently, this
> > > > includes support only for Precision Time Protocol (PTP),
> > > > represented by bit-0.
> > > >
> > > > The register is also added to the kvm_arm_vm_scope_fw_regs[]
> > > > list as it maintains its state per-VM.
> > > >
> > > > Signed-off-by: Raghavendra Rao Ananta <rananta@xxxxxxxxxx>
> > > > ---
> > > > arch/arm64/include/asm/kvm_host.h | 2 ++
> > > > arch/arm64/include/uapi/asm/kvm.h | 4 ++++
> > > > arch/arm64/kvm/guest.c | 1 +
> > > > arch/arm64/kvm/hypercalls.c | 22 +++++++++++++++++++++-
> > > > include/kvm/arm_hypercalls.h | 3 +++
> > > > 5 files changed, 31 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index 318148b69279..d999456c4604 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -106,10 +106,12 @@ struct kvm_arch_memory_slot {
> > > > *
> > > > * @hvc_std_bmap: Bitmap of standard secure service calls
> > > > * @hvc_std_hyp_bmap: Bitmap of standard hypervisor service calls
> > > > + * @hvc_vendor_hyp_bmap: Bitmap of vendor specific hypervisor service calls
> > > > */
> > > > struct kvm_hvc_desc {
> > > > u64 hvc_std_bmap;
> > > > u64 hvc_std_hyp_bmap;
> > > > + u64 hvc_vendor_hyp_bmap;
> > > > };
> > > >
> > > > struct kvm_arch {
> > > > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > > > index 9a2caead7359..ed470bde13d8 100644
> > > > --- a/arch/arm64/include/uapi/asm/kvm.h
> > > > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > > > @@ -299,6 +299,10 @@ struct kvm_arm_copy_mte_tags {
> > > > #define KVM_REG_ARM_STD_HYP_BIT_PV_TIME BIT(0)
> > > > #define KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX 0 /* Last valid bit */
> > > >
> > > > +#define KVM_REG_ARM_VENDOR_HYP_BMAP KVM_REG_ARM_FW_BMAP_REG(2)
> > > > +#define KVM_REG_ARM_VENDOR_HYP_BIT_PTP BIT(0)
> > > > +#define KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX 0 /* Last valid bit */
> > > > +
> > > > /* SVE registers */
> > > > #define KVM_REG_ARM64_SVE (0x15 << KVM_REG_ARM_COPROC_SHIFT)
> > > >
> > > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > > > index c42426d6137e..fc3656f91aed 100644
> > > > --- a/arch/arm64/kvm/guest.c
> > > > +++ b/arch/arm64/kvm/guest.c
> > > > @@ -67,6 +67,7 @@ static const u64 kvm_arm_vm_scope_fw_regs[] = {
> > > > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
> > > > KVM_REG_ARM_STD_BMAP,
> > > > KVM_REG_ARM_STD_HYP_BMAP,
> > > > + KVM_REG_ARM_VENDOR_HYP_BMAP,
> > > > };
> > > >
> > > > /**
> > > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > > > index ebc0cc26cf2e..5c5098c8f1f9 100644
> > > > --- a/arch/arm64/kvm/hypercalls.c
> > > > +++ b/arch/arm64/kvm/hypercalls.c
> > > > @@ -79,6 +79,9 @@ static bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
> > > > case ARM_SMCCC_HV_PV_TIME_ST:
> > > > return kvm_arm_fw_reg_feat_enabled(hvc_desc->hvc_std_hyp_bmap,
> > > > KVM_REG_ARM_STD_HYP_BIT_PV_TIME);
> > > > + case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > > > + return kvm_arm_fw_reg_feat_enabled(hvc_desc->hvc_vendor_hyp_bmap,
> > > > + KVM_REG_ARM_VENDOR_HYP_BIT_PTP);
> > > > default:
> > > > /* By default, allow the services that aren't listed here */
> > > > return true;
> > > > @@ -162,7 +165,14 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> > > > break;
> > > > case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> > > > val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> > > > - val[0] |= BIT(ARM_SMCCC_KVM_FUNC_PTP);
> > > > +
> > > > + /*
> > > > + * The feature bits exposed to user-space doesn't include
> > > > + * ARM_SMCCC_KVM_FUNC_FEATURES. However, we expose this to
> > > > + * the guest as bit-0. Hence, left-shift the user-space
> > > > + * exposed bitmap by 1 to accommodate this.
> > > > + */
> > > > + val[0] |= hvc_desc->hvc_vendor_hyp_bmap << 1;
> > >
> > > Having an off-by-one difference between the userspace and guest
> > > representations of this bitmap seems like it could be a source of bugs
> > > in the future. Its also impossible for the guest to completely hide the
> > > vendor range if it so chooses.
> > >
> > > Why not tie ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID and
> > > ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID to BIT(0)? PTP would then
> > > become BIT(1).
> > >
> > I agree it's a little asymmetrical. But exposing a bit for the
> > func_ids that you mentioned means providing a capability to disable
> > them by the userspace. This would block the guests from even
> > discovering the space. If it's not too ugly, we can maintain certain
> > bits to always remain read-only to the user-space. On the other hand,
> > we can simply ignore what the userspace configure and simply treat it
> > as a userspace bug. What do you think?
>
> I think that assigning a bit to the aforementioned hypercalls would be
> best. If userspace decides to hide all the features enumerated in the
> subrange then there isn't much point to the guest knowing that the range
> even exists. It shouldn't amount to much for userspace, as it will
> likely just keep the default value and only worry about these registers
> when migrating.
>
Sure, I'll include a bit for these as well.
Thanks,
Raghavenadra
> Apologies if I'm being pedantic, but such a subtle implementation detail
> could be overlooked in future changes.
>
> --
> Oliver
>
> > > > break;
> > > > case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID:
> > > > kvm_ptp_get_time(vcpu, val);
> > > > @@ -188,6 +198,7 @@ static const u64 kvm_arm_fw_reg_ids[] = {
> > > > KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
> > > > KVM_REG_ARM_STD_BMAP,
> > > > KVM_REG_ARM_STD_HYP_BMAP,
> > > > + KVM_REG_ARM_VENDOR_HYP_BMAP,
> > > > };
> > > >
> > > > void kvm_arm_init_hypercalls(struct kvm *kvm)
> > > > @@ -196,6 +207,7 @@ void kvm_arm_init_hypercalls(struct kvm *kvm)
> > > >
> > > > hvc_desc->hvc_std_bmap = ARM_SMCCC_STD_FEATURES;
> > > > hvc_desc->hvc_std_hyp_bmap = ARM_SMCCC_STD_HYP_FEATURES;
> > > > + hvc_desc->hvc_vendor_hyp_bmap = ARM_SMCCC_VENDOR_HYP_FEATURES;
> > > > }
> > > >
> > > > int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
> > > > @@ -285,6 +297,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > case KVM_REG_ARM_STD_HYP_BMAP:
> > > > val = READ_ONCE(hvc_desc->hvc_std_hyp_bmap);
> > > > break;
> > > > + case KVM_REG_ARM_VENDOR_HYP_BMAP:
> > > > + val = READ_ONCE(hvc_desc->hvc_vendor_hyp_bmap);
> > > > + break;
> > > > default:
> > > > return -ENOENT;
> > > > }
> > > > @@ -311,6 +326,10 @@ static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
> > > > fw_reg_bmap = &hvc_desc->hvc_std_hyp_bmap;
> > > > fw_reg_features = ARM_SMCCC_STD_HYP_FEATURES;
> > > > break;
> > > > + case KVM_REG_ARM_VENDOR_HYP_BMAP:
> > > > + fw_reg_bmap = &hvc_desc->hvc_vendor_hyp_bmap;
> > > > + fw_reg_features = ARM_SMCCC_VENDOR_HYP_FEATURES;
> > > > + break;
> > > > default:
> > > > return -ENOENT;
> > > > }
> > > > @@ -416,6 +435,7 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
> > > > return 0;
> > > > case KVM_REG_ARM_STD_BMAP:
> > > > case KVM_REG_ARM_STD_HYP_BMAP:
> > > > + case KVM_REG_ARM_VENDOR_HYP_BMAP:
> > > > return kvm_arm_set_fw_reg_bmap(vcpu, reg_id, val);
> > > > default:
> > > > return -ENOENT;
> > > > diff --git a/include/kvm/arm_hypercalls.h b/include/kvm/arm_hypercalls.h
> > > > index a1cb6e839c74..91be758ca58e 100644
> > > > --- a/include/kvm/arm_hypercalls.h
> > > > +++ b/include/kvm/arm_hypercalls.h
> > > > @@ -12,6 +12,9 @@
> > > > #define ARM_SMCCC_STD_HYP_FEATURES \
> > > > GENMASK_ULL(KVM_REG_ARM_STD_HYP_BMAP_BIT_MAX, 0)
> > > >
> > > > +#define ARM_SMCCC_VENDOR_HYP_FEATURES \
> > > > + GENMASK_ULL(KVM_REG_ARM_VENDOR_HYP_BMAP_BIT_MAX, 0)
> > > > +
> > > > int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
> > > >
> > > > static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)
> > > > --
> > > > 2.35.1.473.g83b2b277ed-goog
> > > >
