Re: [PATCH v2] KVM: arm64: Fix confusion in documentation for pKVM SME assert

[Fuad Tabba]

Hi Mark,

On Wed, Jun 5, 2024 at 7:11 PM Mark Brown <broonie@xxxxxxxxxx> 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. This check is also made in the hypervisor, it will refuse to run
> a guest if the check fails, so it appears that the assert here is
> intended to improve diagnostics.
>
> Update the comment to reflect the check in the code, and to clarify that
> we do actually enforce this in the hypervisor. While we're here also
> update to use a WARN_ON_ONCE() to avoid log spam if this triggers.

Reviewed-by: Fuad Tabba <tabba@xxxxxxxxxx

Cheers,
/fuad
>
> Fixes: afb91f5f8ad7 ("KVM: arm64: Ensure that SME controls are disabled in protected mode")
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> ---
> Changes in v2:
> - Commit message tweaks.
> - Change the assert to WARN_ON_ONCE().
> - Link to v1: https://lore.kernel.org/r/20240604-kvm-arm64-sme-assert-v1-1-5d98348d00f8@xxxxxxxxxx
> ---
>  arch/arm64/kvm/fpsimd.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 521b32868d0d..820769567080 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -92,11 +92,14 @@ 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.  The hypervisor
> +        * enforces this when the guest is run, this check is for
> +        * clearer diagnostics.
>          */
> -       WARN_ON(is_protected_kvm_enabled() && system_supports_sme() &&
> -               read_sysreg_s(SYS_SVCR));
> +       WARN_ON_ONCE(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 <broonie@xxxxxxxxxx>
>