| Message ID | 20250205132222.55816-4-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Errata management for VM Live migration | expand |
On Wed, Feb 05, 2025 at 01:22:21PM +0000, Shameer Kolothum wrote: > Currently ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID returns the > bitmap corresponding to KVM_REG_ARM_VENDOR_HYP_BMAP and it only > returns _KVM_FEATURES_FUNC_ID and _KVM_PTP_FUNC_ID. Change that > to return all the KVM/arm64-specific hypercalls exposed by > KVM/arm64 to guest operating systems. > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > arch/arm64/kvm/hypercalls.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index 27ce4cb44904..5cef2590ffdf 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -359,7 +359,11 @@ int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) > val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3; > break; > case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: > - val[0] = smccc_feat->vendor_hyp_bmap; > + val[0] = GENMASK(ARM_SMCCC_KVM_FUNC_PTP, > + ARM_SMCCC_KVM_FUNC_FEATURES); > + /* Function numbers 2-63 are reserved for pKVM for now */ > + val[2] = GENMASK((ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS - 64), > + (ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_VER - 64)); > break; This isn't right, vendor_hyp_bmap is very much load bearing. We have a documented UAPI that allows userspace to control the hypercalls exposed to the guest. The idea being a user wants kernel rollback safety and doesn't expose hypercalls that could potentially be revoked. https://docs.kernel.org/virt/kvm/arm/fw-pseudo-registers.html#bitmap-feature-firmware-registers
On Fri, Feb 07, 2025 at 10:21:13AM -0800, Oliver Upton wrote: > On Wed, Feb 05, 2025 at 01:22:21PM +0000, Shameer Kolothum wrote: > > Currently ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID returns the > > bitmap corresponding to KVM_REG_ARM_VENDOR_HYP_BMAP and it only > > returns _KVM_FEATURES_FUNC_ID and _KVM_PTP_FUNC_ID. Change that > > to return all the KVM/arm64-specific hypercalls exposed by > > KVM/arm64 to guest operating systems. > > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > --- > > arch/arm64/kvm/hypercalls.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > > index 27ce4cb44904..5cef2590ffdf 100644 > > --- a/arch/arm64/kvm/hypercalls.c > > +++ b/arch/arm64/kvm/hypercalls.c > > @@ -359,7 +359,11 @@ int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) > > val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3; > > break; > > case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: > > - val[0] = smccc_feat->vendor_hyp_bmap; > > + val[0] = GENMASK(ARM_SMCCC_KVM_FUNC_PTP, > > + ARM_SMCCC_KVM_FUNC_FEATURES); > > + /* Function numbers 2-63 are reserved for pKVM for now */ > > + val[2] = GENMASK((ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS - 64), > > + (ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_VER - 64)); > > break; > > This isn't right, vendor_hyp_bmap is very much load bearing. We have a > documented UAPI that allows userspace to control the hypercalls exposed > to the guest. > > The idea being a user wants kernel rollback safety and doesn't expose > hypercalls that could potentially be revoked. > > https://docs.kernel.org/virt/kvm/arm/fw-pseudo-registers.html#bitmap-feature-firmware-registers To add: KVM cannot advertise the DISCOVER_IMPL* stuff unconditionally, since the expectation is that userspace implements these hypercalls. These bits may need to be writable from userspace but have a reset value of 0.
> -----Original Message----- > From: Oliver Upton <oliver.upton@linux.dev> > Sent: Friday, February 7, 2025 6:24 PM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: kvmarm@lists.linux.dev; maz@kernel.org; catalin.marinas@arm.com; > will@kernel.org; mark.rutland@arm.com; cohuck@redhat.com; > eric.auger@redhat.com; sebott@redhat.com; yuzenghui > <yuzenghui@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>; > jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron > <jonathan.cameron@huawei.com>; Anthony Jebson > <anthony.jebson@huawei.com>; linux-arm-kernel@lists.infradead.org; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH v6 3/4] KVM: arm64: Report all the KVM/arm64-specific > hypercalls > > On Fri, Feb 07, 2025 at 10:21:13AM -0800, Oliver Upton wrote: > > On Wed, Feb 05, 2025 at 01:22:21PM +0000, Shameer Kolothum wrote: > > > Currently ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID returns > the > > > bitmap corresponding to KVM_REG_ARM_VENDOR_HYP_BMAP and it > only > > > returns _KVM_FEATURES_FUNC_ID and _KVM_PTP_FUNC_ID. Change > that > > > to return all the KVM/arm64-specific hypercalls exposed by > > > KVM/arm64 to guest operating systems. > > > > > > Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > > > --- > > > arch/arm64/kvm/hypercalls.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > > > index 27ce4cb44904..5cef2590ffdf 100644 > > > --- a/arch/arm64/kvm/hypercalls.c > > > +++ b/arch/arm64/kvm/hypercalls.c > > > @@ -359,7 +359,11 @@ int kvm_smccc_call_handler(struct kvm_vcpu > *vcpu) > > > val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3; > > > break; > > > case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: > > > - val[0] = smccc_feat->vendor_hyp_bmap; > > > + val[0] = GENMASK(ARM_SMCCC_KVM_FUNC_PTP, > > > + ARM_SMCCC_KVM_FUNC_FEATURES); > > > + /* Function numbers 2-63 are reserved for pKVM for now */ > > > + val[2] = > GENMASK((ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS - 64), > > > + > (ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_VER - 64)); > > > break; > > > > This isn't right, vendor_hyp_bmap is very much load bearing. We have a > > documented UAPI that allows userspace to control the hypercalls exposed > > to the guest. > > > > The idea being a user wants kernel rollback safety and doesn't expose > > hypercalls that could potentially be revoked. > > > > https://docs.kernel.org/virt/kvm/arm/fw-pseudo-registers.html#bitmap- > feature-firmware-registers > > To add: > > KVM cannot advertise the DISCOVER_IMPL* stuff unconditionally, since the > expectation is that userspace implements these hypercalls. These bits > may need to be writable from userspace but have a reset value of 0. > Ok. So IIUC, vendor_hyp_bmap actually holds the information which Vendor Hyp services are available to the user space and can be get/set using GET/SET _ONE_REG interfaces. Currently this bitmap is a 64 bit one and if we have to have a one to one mapping between these bitmap and the hypercall function numbers, then that requires some changes. Because function numbers 2-63 are now reserved for pKVM and the new ones introduced in this series take 64 & 65. May be we can have KVM_REG_ARM_VENDOR_HYP_BMAP_2 which represents 64-127? Or can we take the next available bits(2 & 3) for KVM_REG_ARM_VENDOR_HYP_BMAP and then map it to the function number appropriately (64 & 65) when ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID is handled? Thoughts? Thanks, Shameer
> > > This isn't right, vendor_hyp_bmap is very much load bearing. We have a > > > documented UAPI that allows userspace to control the hypercalls exposed > > > to the guest. > > > > > > The idea being a user wants kernel rollback safety and doesn't expose > > > hypercalls that could potentially be revoked. > > > > > > https://docs.kernel.org/virt/kvm/arm/fw-pseudo-registers.html#bitmap- > > feature-firmware-registers > > > > To add: > > > > KVM cannot advertise the DISCOVER_IMPL* stuff unconditionally, since the > > expectation is that userspace implements these hypercalls. These bits > > may need to be writable from userspace but have a reset value of 0. > > > > Ok. So IIUC, vendor_hyp_bmap actually holds the information which Vendor Hyp > services are available to the user space and can be get/set using GET/SET _ONE_REG > interfaces. Right. > Currently this bitmap is a 64 bit one and if we have to have a one to one mapping > between these bitmap and the hypercall function numbers, then that requires > some changes. > > Because function numbers 2-63 are now reserved for pKVM and the new ones > introduced in this series take 64 & 65. > > May be we can have KVM_REG_ARM_VENDOR_HYP_BMAP_2 which represents > 64-127? > > Or can we take the next available bits(2 & 3) for KVM_REG_ARM_VENDOR_HYP_BMAP > and then map it to the function number appropriately (64 & 65) when > ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID is handled? > > Thoughts? Adding a second register seems reasonable so we can preserve the mapping of bit position / hypercall #.
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index 27ce4cb44904..5cef2590ffdf 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -359,7 +359,11 @@ int kvm_smccc_call_handler(struct kvm_vcpu *vcpu) val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3; break; case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: - val[0] = smccc_feat->vendor_hyp_bmap; + val[0] = GENMASK(ARM_SMCCC_KVM_FUNC_PTP, + ARM_SMCCC_KVM_FUNC_FEATURES); + /* Function numbers 2-63 are reserved for pKVM for now */ + val[2] = GENMASK((ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS - 64), + (ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_VER - 64)); break; case ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID: kvm_ptp_get_time(vcpu, val);
Currently ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID returns the bitmap corresponding to KVM_REG_ARM_VENDOR_HYP_BMAP and it only returns _KVM_FEATURES_FUNC_ID and _KVM_PTP_FUNC_ID. Change that to return all the KVM/arm64-specific hypercalls exposed by KVM/arm64 to guest operating systems. Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- arch/arm64/kvm/hypercalls.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)