| Message ID | 20200401165816.530281-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PSCI fixes | expand |
On Wed, Apr 01, 2020 at 05:58:16PM +0100, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: > -- > 2.25.0 > Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>
Hello, On 4/1/20 5:58 PM, Marc Zyngier wrote: > Implementing (and even advertising) 64bit PSCI functions to 32bit > guests is at least a bit odd, if not altogether violating the > spec which says ("5.2.1 Register usage in arguments and return values"): > > "Adherence to the SMC Calling Conventions implies that any AArch32 > caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). > This matches the NOT_SUPPORTED error code used in PSCI" > > Tighten the implementation by pretending these functions are not > there for 32bit guests. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c > index 69ff4a51ceb5..122795cdd984 100644 > --- a/virt/kvm/arm/psci.c > +++ b/virt/kvm/arm/psci.c > @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) > vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); > } > > +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) > +{ > + switch(fn) { > + case PSCI_0_2_FN64_CPU_SUSPEND: > + case PSCI_0_2_FN64_CPU_ON: > + case PSCI_0_2_FN64_AFFINITY_INFO: I checked in ARM DEN 0022D, those are indeed the only 3 functions that KVM implements and have a different function ID based on the calling convention. > + /* Disallow these functions for 32bit guests */ > + if (vcpu_mode_is_32bit(vcpu)) > + return PSCI_RET_NOT_SUPPORTED; > + break; > + } > + > + return 0; > +} > + > static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > { > struct kvm *kvm = vcpu->kvm; > @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > unsigned long val; > int ret = 1; > > + val = kvm_psci_check_allowed_function(vcpu, psci_fn); > + if (val) > + goto out; > + > switch (psci_fn) { > case PSCI_0_2_FN_PSCI_VERSION: > /* > @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) > break; > } > > +out: > smccc_set_retval(vcpu, val, 0, 0, 0); > return ret; > } > @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) > break; > case PSCI_1_0_FN_PSCI_FEATURES: > feature = smccc_get_arg1(vcpu); > + val = kvm_psci_check_allowed_function(vcpu, feature); > + if (val) > + break; > + > switch(feature) { > case PSCI_0_2_FN_PSCI_VERSION: > case PSCI_0_2_FN_CPU_SUSPEND: The patch makes sense to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex
diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 69ff4a51ceb5..122795cdd984 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c @@ -199,6 +199,21 @@ static void kvm_psci_narrow_to_32bit(struct kvm_vcpu *vcpu) vcpu_set_reg(vcpu, i, (u32)vcpu_get_reg(vcpu, i)); } +static unsigned long kvm_psci_check_allowed_function(struct kvm_vcpu *vcpu, u32 fn) +{ + switch(fn) { + case PSCI_0_2_FN64_CPU_SUSPEND: + case PSCI_0_2_FN64_CPU_ON: + case PSCI_0_2_FN64_AFFINITY_INFO: + /* Disallow these functions for 32bit guests */ + if (vcpu_mode_is_32bit(vcpu)) + return PSCI_RET_NOT_SUPPORTED; + break; + } + + return 0; +} + static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) { struct kvm *kvm = vcpu->kvm; @@ -206,6 +221,10 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) unsigned long val; int ret = 1; + val = kvm_psci_check_allowed_function(vcpu, psci_fn); + if (val) + goto out; + switch (psci_fn) { case PSCI_0_2_FN_PSCI_VERSION: /* @@ -273,6 +292,7 @@ static int kvm_psci_0_2_call(struct kvm_vcpu *vcpu) break; } +out: smccc_set_retval(vcpu, val, 0, 0, 0); return ret; } @@ -290,6 +310,10 @@ static int kvm_psci_1_0_call(struct kvm_vcpu *vcpu) break; case PSCI_1_0_FN_PSCI_FEATURES: feature = smccc_get_arg1(vcpu); + val = kvm_psci_check_allowed_function(vcpu, feature); + if (val) + break; + switch(feature) { case PSCI_0_2_FN_PSCI_VERSION: case PSCI_0_2_FN_CPU_SUSPEND:
Implementing (and even advertising) 64bit PSCI functions to 32bit guests is at least a bit odd, if not altogether violating the spec which says ("5.2.1 Register usage in arguments and return values"): "Adherence to the SMC Calling Conventions implies that any AArch32 caller of an SMC64 function will get a return code of 0xFFFFFFFF(int32). This matches the NOT_SUPPORTED error code used in PSCI" Tighten the implementation by pretending these functions are not there for 32bit guests. Signed-off-by: Marc Zyngier <maz@kernel.org> --- virt/kvm/arm/psci.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)