Re: [PATCH v4 07/43] arm64: RME: Check for RME support at KVM init

[Steven Price <steven.price@arm.com>]

Hi Gavin,

On 12/09/2024 09:49, Gavin Shan wrote:
> On 8/22/24 1:38 AM, Steven Price wrote:
>> Query the RMI version number and check if it is a compatible version. A
>> static key is also provided to signal that a supported RMM is available.
>>
>> Functions are provided to query if a VM or VCPU is a realm (or rec)
>> which currently will always return false.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v2:
>>   * Drop return value from kvm_init_rme(), it was always 0.
>>   * Rely on the RMM return value to identify whether the RSI ABI is
>>     compatible.
>> ---
>>   arch/arm64/include/asm/kvm_emulate.h | 17 +++++++++
>>   arch/arm64/include/asm/kvm_host.h    |  4 ++
>>   arch/arm64/include/asm/kvm_rme.h     | 56 ++++++++++++++++++++++++++++
>>   arch/arm64/include/asm/virt.h        |  1 +
>>   arch/arm64/kvm/Makefile              |  3 +-
>>   arch/arm64/kvm/arm.c                 |  6 +++
>>   arch/arm64/kvm/rme.c                 | 50 +++++++++++++++++++++++++
>>   7 files changed, 136 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/arm64/include/asm/kvm_rme.h
>>   create mode 100644 arch/arm64/kvm/rme.c
>>
> 
> [...]
> 
>> diff --git a/arch/arm64/kvm/rme.c b/arch/arm64/kvm/rme.c
>> new file mode 100644
>> index 000000000000..418685fbf6ed
>> --- /dev/null
>> +++ b/arch/arm64/kvm/rme.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2023 ARM Ltd.
>> + */
>> +
>> +#include <linux/kvm_host.h>
>> +
>> +#include <asm/rmi_cmds.h>
>> +#include <asm/virt.h>
>> +
>> +static int rmi_check_version(void)
>> +{
>> +    struct arm_smccc_res res;
>> +    int version_major, version_minor;
>> +    unsigned long host_version = RMI_ABI_VERSION(RMI_ABI_MAJOR_VERSION,
>> +                             RMI_ABI_MINOR_VERSION);
>> +
>> +    arm_smccc_1_1_invoke(SMC_RMI_VERSION, host_version, &res);
>> +
>> +    if (res.a0 == SMCCC_RET_NOT_SUPPORTED)
>> +        return -ENXIO;
>> +
>> +    version_major = RMI_ABI_VERSION_GET_MAJOR(res.a1);
>> +    version_minor = RMI_ABI_VERSION_GET_MINOR(res.a1);
>> +
>> +    if (res.a0 != RMI_SUCCESS) {
>> +        kvm_err("Unsupported RMI ABI (v%d.%d) host supports v%d.%d\n",
>> +            version_major, version_minor,
>> +            RMI_ABI_MAJOR_VERSION,
>> +            RMI_ABI_MINOR_VERSION);
> 
> This message is perhaps something like below since a range of versions
> can be
> supported by one particular RMM release.
> 
>     kvm_err("Unsupported RMI ABI v%d.%d. Host supports v%ld.%ld -
> v%ld.%ld\n",
>             RMI_ABI_MAJOR_VERSION, RMI_ABI_MINOR_VERSION,
>             RMI_ABI_VERSION_GET_MAJOR(res.a1),
> RMI_ABI_VERSION_GET_MINOR(res.a1),
>             RMI_ABI_VERSION_GET_MAJOR(res.a2),
> RMI_ABI_VERSION_GET_MINOR(res.a2));
> 
>> +        return -ENXIO;
>> +    }
>> +
>> +    kvm_info("RMI ABI version %d.%d\n", version_major, version_minor);
>> +
> 
> We probably need to print the requested version, instead of the lower
> implemented
> version, if I'm correct. At present, both of them have been fixed to
> v1.0 and we
> don't have a problem though.

The RSI_VERSION command is somewhat complex. The RMM returns both a
"higher revision" and a "lower revision". The higher revision is the
highest interface revision supported by the RMM - and not especially
useful at least for the moment when Linux is only aiming for v1.0. So
we're currently just reporting the "lower revision" in the outputs.

There are three possibilities explained in the spec:

a) The RMM is compatible (status code is RSI_SUCCESS). From the spec
"The lower revision is equal to the requested revision". So this last
print will indeed output the requested revision.

b) The RMM doesn't support the requested revision, it supports an older
revision. In this case "The lower revision is the highest interface
revision which is both less than the requested revision and
supported by the RMM". Of course when we're requesting v1.0 this
situation should never occur, but generally we'd expect to negotiate the
lower revision if possible so this is the useful information to output.

c) The RMM does not support the requested revision, it supports a newer
revision. From the spec "The lower revision is equal to the higher
revision". So there's no point outputting both.

So situation (b) is the only case where the higher revision is
interesting. But it's only useful in a situation like:

 * Linux supports v1.1 and v2.0 (and maybe v1.0).
 * Linux prefers v1.1 over v2.0 (and v2.0 over v1.0).
 * Linux therefore requests v1.1.
 * The RMM supports v1.0 and v2.0 (but not v1.1) and so returns failure.
 * lower revision: v1.0
 * higher revision: v2.0

Linux can then use the higher revision field to detect that it can try
again with v2.0.

My expectation is that newer revisions will be preferred and so Linux
will always start by requesting the newest revision it supports (so
"higher revision" will be irrelevant). But it's there so we can support
a scenario like above.

We could output the higher revision just for information, a form of
"hey, your RMM is newer than Linux" - but I'm not sure I see the point.

Steve

>         kvm_info("RMI ABI version v%d.%d\n", RMI_ABI_MAJOR_VERSION,
> RMI_ABI_MINOR_VERSION);
> 
>> +    return 0;
>> +}
>> +
> 
> Thanks,
> Gavin
>