As raised in the review comments for the original patch the assert and
comment added in afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are
disabled in protected mode") are bogus. The comments says that we check
that we do not have SME enabled for a pKVM guest but the assert actually
checks to see if the host has anything set in SVCR which is unrelated to
the guest features or state, regardless of if those guests are protected
or not.
What I believe the check is actually intended to validate is that we do
not enter the pKVM hypervisor with SME enabled since the pKVM hypervisor
does not yet understand SME and is therefore unable to save or restore
host state with SME enabled, indeed attempting to save SVE state would
fault if streaming mode is enabled on a system without FA64 due to FFR.
Update the comment to reflect this.
Fixes: afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are disabled in protected mode")
Signed-off-by: Mark Brown <[email protected]>
---
arch/arm64/kvm/fpsimd.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 521b32868d0d..f720ba47b85c 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -92,8 +92,9 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
}
/*
- * If normal guests gain SME support, maintain this behavior for pKVM
- * guests, which don't support SME.
+ * The pKVM hypervisor does not yet understand how to save or
+ * restore SME state for the host so double check that if we
+ * are running with pKVM we have disabled SME.
*/
WARN_ON(is_protected_kvm_enabled() && system_supports_sme() &&
read_sysreg_s(SYS_SVCR));
---
base-commit: afb91f5f8ad7af172d993a34fde1947892408f53
change-id: 20240604-kvm-arm64-sme-assert-5ad755d4e8a6
Best regards,
--
Mark Brown <[email protected]>
Hi,
On Tue, Jun 04, 2024 at 07:47:01PM +0100, Mark Brown wrote:
> As raised in the review comments for the original patch the assert and
> comment added in afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are
> disabled in protected mode") are bogus. The comments says that we check
> that we do not have SME enabled for a pKVM guest but the assert actually
> checks to see if the host has anything set in SVCR which is unrelated to
> the guest features or state, regardless of if those guests are protected
> or not.
>
> What I believe the check is actually intended to validate is that we do
> not enter the pKVM hypervisor with SME enabled since the pKVM hypervisor
> does not yet understand SME and is therefore unable to save or restore
> host state with SME enabled, indeed attempting to save SVE state would
> fault if streaming mode is enabled on a system without FA64 due to FFR.
> Update the comment to reflect this.
The added context likely isn't necessary in what winds up getting
applied. Can this just directly state the WARN_ON() exists b/c the
protected mode hypervisor doesn't know how to manage SME state?
> Fixes: afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are disabled in protected mode")
> Signed-off-by: Mark Brown <[email protected]>
> ---
> arch/arm64/kvm/fpsimd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 521b32868d0d..f720ba47b85c 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -92,8 +92,9 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
> }
>
> /*
> - * If normal guests gain SME support, maintain this behavior for pKVM
> - * guests, which don't support SME.
> + * The pKVM hypervisor does not yet understand how to save or
> + * restore SME state for the host so double check that if we
> + * are running with pKVM we have disabled SME.
> */
> WARN_ON(is_protected_kvm_enabled() && system_supports_sme() &&
> read_sysreg_s(SYS_SVCR));
While we're here, this should be WARN_ON_ONCE() or WARN_RATELIMIT() if
we _really_ want some spam. But a single WARN ought to be enough.
It'd be a good idea to also document why we're testing for SME state
twice on the KVM_RUN path, as any WARN() in the hyp code is currently
fatal. I'm guessing Fuad meant to have a non-fatal way of getting some
debug information out.
--
Thanks,
Oliver
On Tue, Jun 04, 2024 at 12:17:41PM -0700, Oliver Upton wrote:
> On Tue, Jun 04, 2024 at 07:47:01PM +0100, Mark Brown wrote:
> > As raised in the review comments for the original patch the assert and
> > comment added in afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are
> > disabled in protected mode") are bogus. The comments says that we check
> > that we do not have SME enabled for a pKVM guest but the assert actually
> > checks to see if the host has anything set in SVCR which is unrelated to
> > the guest features or state, regardless of if those guests are protected
> > or not.
> > What I believe the check is actually intended to validate is that we do
> > not enter the pKVM hypervisor with SME enabled since the pKVM hypervisor
> > does not yet understand SME and is therefore unable to save or restore
> > host state with SME enabled, indeed attempting to save SVE state would
> > fault if streaming mode is enabled on a system without FA64 due to FFR.
> > Update the comment to reflect this.
> The added context likely isn't necessary in what winds up getting
> applied. Can this just directly state the WARN_ON() exists b/c the
> protected mode hypervisor doesn't know how to manage SME state?
It could definitely be briefer, I was being super detailed because I was
guessing between the intent being what the comment says or what the code
says, or if we want an assert here at all for that matter. It seemed
better to be verbose.
> > WARN_ON(is_protected_kvm_enabled() && system_supports_sme() &&
> > read_sysreg_s(SYS_SVCR));
> While we're here, this should be WARN_ON_ONCE() or WARN_RATELIMIT() if
> we _really_ want some spam. But a single WARN ought to be enough.
Good point, and I agree that WARN_ON_ONCE is the better option.
> It'd be a good idea to also document why we're testing for SME state
> twice on the KVM_RUN path, as any WARN() in the hyp code is currently
> fatal. I'm guessing Fuad meant to have a non-fatal way of getting some
> debug information out.
Yes, this was one of the reasons I was unclear if the check or the
comment was the intention. Possibly it's due to getting much better
diagnostics for a warning generated in the host kernel than an error
returned from the hypervisor when we try to run the guest?