From: Will Deacon <[email protected]>
We can advertise ourselves to guests as KVM and provide a basic features
bitmap for discoverability of future hypervisor services.
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Will Deacon <[email protected]>
Signed-off-by: Jianyong Wu <[email protected]>
---
arch/arm64/kvm/hypercalls.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 550dfa3e53cd..db6dce3d0e23 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -12,13 +12,13 @@
int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
{
u32 func_id = smccc_get_function(vcpu);
- long val = SMCCC_RET_NOT_SUPPORTED;
+ u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
u32 feature;
gpa_t gpa;
switch (func_id) {
case ARM_SMCCC_VERSION_FUNC_ID:
- val = ARM_SMCCC_VERSION_1_1;
+ val[0] = ARM_SMCCC_VERSION_1_1;
break;
case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
feature = smccc_get_arg1(vcpu);
@@ -28,10 +28,10 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
case KVM_BP_HARDEN_UNKNOWN:
break;
case KVM_BP_HARDEN_WA_NEEDED:
- val = SMCCC_RET_SUCCESS;
+ val[0] = SMCCC_RET_SUCCESS;
break;
case KVM_BP_HARDEN_NOT_REQUIRED:
- val = SMCCC_RET_NOT_REQUIRED;
+ val[0] = SMCCC_RET_NOT_REQUIRED;
break;
}
break;
@@ -41,31 +41,40 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
case KVM_SSBD_UNKNOWN:
break;
case KVM_SSBD_KERNEL:
- val = SMCCC_RET_SUCCESS;
+ val[0] = SMCCC_RET_SUCCESS;
break;
case KVM_SSBD_FORCE_ENABLE:
case KVM_SSBD_MITIGATED:
- val = SMCCC_RET_NOT_REQUIRED;
+ val[0] = SMCCC_RET_NOT_REQUIRED;
break;
}
break;
case ARM_SMCCC_HV_PV_TIME_FEATURES:
- val = SMCCC_RET_SUCCESS;
+ val[0] = SMCCC_RET_SUCCESS;
break;
}
break;
case ARM_SMCCC_HV_PV_TIME_FEATURES:
- val = kvm_hypercall_pv_features(vcpu);
+ val[0] = kvm_hypercall_pv_features(vcpu);
break;
case ARM_SMCCC_HV_PV_TIME_ST:
gpa = kvm_init_stolen_time(vcpu);
if (gpa != GPA_INVALID)
- val = gpa;
+ val[0] = gpa;
+ break;
+ case ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID:
+ val[0] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0;
+ val[1] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1;
+ val[2] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2;
+ val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3;
+ break;
+ case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
+ val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
break;
default:
return kvm_psci_call(vcpu);
}
- smccc_set_retval(vcpu, val, 0, 0, 0);
+ smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
return 1;
}
--
2.17.1
Hi Will,
> -----Original Message-----
> From: Jianyong Wu <[email protected]>
> Sent: Friday, June 19, 2020 9:01 PM
> To: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; [email protected]; Suzuki Poulose
> <[email protected]>; Steven Price <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Steve Capper
> <[email protected]>; Kaly Xin <[email protected]>; Justin He
> <[email protected]>; Wei Chen <[email protected]>; Jianyong Wu
> <[email protected]>; nd <[email protected]>
> Subject: [PATCH v13 2/9] arm/arm64: KVM: Advertise KVM UID to guests via
> SMCCC
>
> From: Will Deacon <[email protected]>
>
> We can advertise ourselves to guests as KVM and provide a basic features
> bitmap for discoverability of future hypervisor services.
>
> Cc: Marc Zyngier <[email protected]>
> Signed-off-by: Will Deacon <[email protected]>
> Signed-off-by: Jianyong Wu <[email protected]>
> ---
> arch/arm64/kvm/hypercalls.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 550dfa3e53cd..db6dce3d0e23 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -12,13 +12,13 @@
> int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) {
> u32 func_id = smccc_get_function(vcpu);
> - long val = SMCCC_RET_NOT_SUPPORTED;
> + u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
There is a risk as this u32 value will return here and a u64 value will be obtained in guest. For example,
The val[0] is initialized as -1 of 0xffffffff and the guest get 0xffffffff then it will be compared with -1 of 0xffffffffffffffff
Also this problem exists for the transfer of address in u64 type. So the following assignment to "val" should be split into
two u32 value and assign to val[0] and val[1] respectively.
WDYT?
Thanks
Jianyong
> u32 feature;
> gpa_t gpa;
>
> switch (func_id) {
> case ARM_SMCCC_VERSION_FUNC_ID:
> - val = ARM_SMCCC_VERSION_1_1;
> + val[0] = ARM_SMCCC_VERSION_1_1;
> break;
> case ARM_SMCCC_ARCH_FEATURES_FUNC_ID:
> feature = smccc_get_arg1(vcpu);
> @@ -28,10 +28,10 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> case KVM_BP_HARDEN_UNKNOWN:
> break;
> case KVM_BP_HARDEN_WA_NEEDED:
> - val = SMCCC_RET_SUCCESS;
> + val[0] = SMCCC_RET_SUCCESS;
> break;
> case KVM_BP_HARDEN_NOT_REQUIRED:
> - val = SMCCC_RET_NOT_REQUIRED;
> + val[0] = SMCCC_RET_NOT_REQUIRED;
> break;
> }
> break;
> @@ -41,31 +41,40 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
> case KVM_SSBD_UNKNOWN:
> break;
> case KVM_SSBD_KERNEL:
> - val = SMCCC_RET_SUCCESS;
> + val[0] = SMCCC_RET_SUCCESS;
> break;
> case KVM_SSBD_FORCE_ENABLE:
> case KVM_SSBD_MITIGATED:
> - val = SMCCC_RET_NOT_REQUIRED;
> + val[0] = SMCCC_RET_NOT_REQUIRED;
> break;
> }
> break;
> case ARM_SMCCC_HV_PV_TIME_FEATURES:
> - val = SMCCC_RET_SUCCESS;
> + val[0] = SMCCC_RET_SUCCESS;
> break;
> }
> break;
> case ARM_SMCCC_HV_PV_TIME_FEATURES:
> - val = kvm_hypercall_pv_features(vcpu);
> + val[0] = kvm_hypercall_pv_features(vcpu);
> break;
> case ARM_SMCCC_HV_PV_TIME_ST:
> gpa = kvm_init_stolen_time(vcpu);
> if (gpa != GPA_INVALID)
> - val = gpa;
> + val[0] = gpa;
> + break;
> + case ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID:
> + val[0] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_0;
> + val[1] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_1;
> + val[2] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2;
> + val[3] = ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_3;
> + break;
> + case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID:
> + val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES);
> break;
> default:
> return kvm_psci_call(vcpu);
> }
>
> - smccc_set_retval(vcpu, val, 0, 0, 0);
> + smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
> return 1;
> }
> --
> 2.17.1
On Mon, Jul 27, 2020 at 03:45:37AM +0000, Jianyong Wu wrote:
> > From: Will Deacon <[email protected]>
> >
> > We can advertise ourselves to guests as KVM and provide a basic features
> > bitmap for discoverability of future hypervisor services.
> >
> > Cc: Marc Zyngier <[email protected]>
> > Signed-off-by: Will Deacon <[email protected]>
> > Signed-off-by: Jianyong Wu <[email protected]>
> > ---
> > arch/arm64/kvm/hypercalls.c | 29 +++++++++++++++++++----------
> > 1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> > index 550dfa3e53cd..db6dce3d0e23 100644
> > --- a/arch/arm64/kvm/hypercalls.c
> > +++ b/arch/arm64/kvm/hypercalls.c
> > @@ -12,13 +12,13 @@
> > int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) {
> > u32 func_id = smccc_get_function(vcpu);
> > - long val = SMCCC_RET_NOT_SUPPORTED;
> > + u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
>
> There is a risk as this u32 value will return here and a u64 value will be
> obtained in guest. For example, The val[0] is initialized as -1 of
> 0xffffffff and the guest get 0xffffffff then it will be compared with -1
> of 0xffffffffffffffff Also this problem exists for the transfer of address
> in u64 type. So the following assignment to "val" should be split into two
> u32 value and assign to val[0] and val[1] respectively.
> WDYT?
Yes, I think you're right that this is a bug, but isn't the solution just
to make that an array of 'long'?
long val [4];
That will sign-extend the negative error codes as required, while leaving
the explicitly unsigned UID constants alone.
Will
> -----Original Message-----
> From: Will Deacon <[email protected]>
> Sent: Monday, July 27, 2020 7:38 PM
> To: Jianyong Wu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Mark Rutland
> <[email protected]>; Suzuki Poulose <[email protected]>;
> Steven Price <[email protected]>; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; Steve Capper <[email protected]>; Kaly Xin
> <[email protected]>; Justin He <[email protected]>; Wei Chen
> <[email protected]>; nd <[email protected]>
> Subject: Re: [PATCH v13 2/9] arm/arm64: KVM: Advertise KVM UID to guests
> via SMCCC
>
> On Mon, Jul 27, 2020 at 03:45:37AM +0000, Jianyong Wu wrote:
> > > From: Will Deacon <[email protected]>
> > >
> > > We can advertise ourselves to guests as KVM and provide a basic
> > > features bitmap for discoverability of future hypervisor services.
> > >
> > > Cc: Marc Zyngier <[email protected]>
> > > Signed-off-by: Will Deacon <[email protected]>
> > > Signed-off-by: Jianyong Wu <[email protected]>
> > > ---
> > > arch/arm64/kvm/hypercalls.c | 29 +++++++++++++++++++----------
> > > 1 file changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/hypercalls.c
> > > b/arch/arm64/kvm/hypercalls.c index 550dfa3e53cd..db6dce3d0e23
> > > 100644
> > > --- a/arch/arm64/kvm/hypercalls.c
> > > +++ b/arch/arm64/kvm/hypercalls.c
> > > @@ -12,13 +12,13 @@
> > > int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) {
> > > u32 func_id = smccc_get_function(vcpu);
> > > - long val = SMCCC_RET_NOT_SUPPORTED;
> > > + u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
> >
> > There is a risk as this u32 value will return here and a u64 value
> > will be obtained in guest. For example, The val[0] is initialized as
> > -1 of 0xffffffff and the guest get 0xffffffff then it will be compared
> > with -1 of 0xffffffffffffffff Also this problem exists for the
> > transfer of address in u64 type. So the following assignment to "val"
> > should be split into two
> > u32 value and assign to val[0] and val[1] respectively.
> > WDYT?
>
> Yes, I think you're right that this is a bug, but isn't the solution just to make
> that an array of 'long'?
>
> long val [4];
>
> That will sign-extend the negative error codes as required, while leaving the
> explicitly unsigned UID constants alone.
Ok, that's much better. I will fix it at next version.
By the way, I wonder when will you update this patch set. I see someone like me
adopt this patch set as code base and need rebase it every time, so expect your update.
Thanks
Jianyong
>
> Will
On Tue, Jul 28, 2020 at 01:07:14AM +0000, Jianyong Wu wrote:
>
>
> > -----Original Message-----
> > From: Will Deacon <[email protected]>
> > Sent: Monday, July 27, 2020 7:38 PM
> > To: Jianyong Wu <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Mark Rutland
> > <[email protected]>; Suzuki Poulose <[email protected]>;
> > Steven Price <[email protected]>; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]; Steve Capper <[email protected]>; Kaly Xin
> > <[email protected]>; Justin He <[email protected]>; Wei Chen
> > <[email protected]>; nd <[email protected]>
> > Subject: Re: [PATCH v13 2/9] arm/arm64: KVM: Advertise KVM UID to guests
> > via SMCCC
> >
> > On Mon, Jul 27, 2020 at 03:45:37AM +0000, Jianyong Wu wrote:
> > > > From: Will Deacon <[email protected]>
> > > >
> > > > We can advertise ourselves to guests as KVM and provide a basic
> > > > features bitmap for discoverability of future hypervisor services.
> > > >
> > > > Cc: Marc Zyngier <[email protected]>
> > > > Signed-off-by: Will Deacon <[email protected]>
> > > > Signed-off-by: Jianyong Wu <[email protected]>
> > > > ---
> > > > arch/arm64/kvm/hypercalls.c | 29 +++++++++++++++++++----------
> > > > 1 file changed, 19 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/kvm/hypercalls.c
> > > > b/arch/arm64/kvm/hypercalls.c index 550dfa3e53cd..db6dce3d0e23
> > > > 100644
> > > > --- a/arch/arm64/kvm/hypercalls.c
> > > > +++ b/arch/arm64/kvm/hypercalls.c
> > > > @@ -12,13 +12,13 @@
> > > > int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) {
> > > > u32 func_id = smccc_get_function(vcpu);
> > > > - long val = SMCCC_RET_NOT_SUPPORTED;
> > > > + u32 val[4] = {SMCCC_RET_NOT_SUPPORTED};
> > >
> > > There is a risk as this u32 value will return here and a u64 value
> > > will be obtained in guest. For example, The val[0] is initialized as
> > > -1 of 0xffffffff and the guest get 0xffffffff then it will be compared
> > > with -1 of 0xffffffffffffffff Also this problem exists for the
> > > transfer of address in u64 type. So the following assignment to "val"
> > > should be split into two
> > > u32 value and assign to val[0] and val[1] respectively.
> > > WDYT?
> >
> > Yes, I think you're right that this is a bug, but isn't the solution just to make
> > that an array of 'long'?
> >
> > long val [4];
> >
> > That will sign-extend the negative error codes as required, while leaving the
> > explicitly unsigned UID constants alone.
>
> Ok, that's much better. I will fix it at next version.
>
> By the way, I wonder when will you update this patch set. I see someone like me
> adopt this patch set as code base and need rebase it every time, so expect your update.
I'm not working on it, so please feel free to include it along with the
patches that add an upstream user.
Will