Re: [PATCH v4 2/3] KVM: arm64: Introduce hypercall support for retrieving target implementations

[Marc Zyngier <maz@kernel.org>]

Hi Shameer,

On Wed, 18 Dec 2024 10:53:44 +0000,
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> If the Guest requires migration to multiple targets, this hypercall
> will provide a way to retrieve the target CPU implementations from
> the user space VMM.
> 
> Subsequent patch will use this to enable the associated errata.
> 
> Suggested-by: Oliver Upton <oliver.upton@linux.dev>
> Reviewed-by: Sebastian Ott <sebott@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  Documentation/virt/kvm/arm/hypercalls.rst | 30 +++++++++++++++++++++++
>  include/linux/arm-smccc.h                 |  7 ++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst
> index af7bc2c2e0cb..16b4c02cf9d9 100644
> --- a/Documentation/virt/kvm/arm/hypercalls.rst
> +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> @@ -142,3 +142,33 @@ region is equal to the memory protection granule advertised by
>  |                     |          |    +---------------------------------------------+
>  |                     |          |    | ``INVALID_PARAMETER (-3)``                  |
>  +---------------------+----------+----+---------------------------------------------+
> +
> +``ARM_SMCCC_VENDOR_HYP_KVM_DISCOVER_IMPL_CPUS_FUNC_ID``
> +-------------------------------------------------------
> +
> +Request the target CPU implementation information for the Guest VM. This hypercall
> +must be handled by the userspace VMM. The Guest kernel will use this information to
> +enable the associated errata. A maximum of 64 implementations is supported.

In the interest of being able to support this long term, I really
think there should be a separate function returning:

- the version of the extension, modelled after the PSCI versioning

- how many different implementations the extension supports

This would avoid hardcoding this 'maximum of 64 implementations' in
the spec, and allow us to add new ID registers if they are added in
the future.

> +
> ++---------------------+-------------------------------------------------------------+
> +| Presence:           | Optional;  KVM/ARM64 Guests only                            |
> ++---------------------+-------------------------------------------------------------+
> +| Calling convention: | HVC64                                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Function ID:        | (uint32) | 0xC600007E                                       |
> ++---------------------+----------+----+---------------------------------------------+
> +| Arguments:          | (uint64) | R1 | selected implementation index               |
> +|                     +----------+----+---------------------------------------------+
> +|                     | (uint64) | R2 | Reserved / Must be zero                     |
> +|                     +----------+----+---------------------------------------------+
> +|                     | (uint64) | R3 | Reserved / Must be zero                     |
> ++---------------------+----------+----+---------------------------------------------+
> +| Return Values:      | (int64)  | R0 | -1 on error else the maximum required CPU   |
> +|                     |          |    | implementation index.                       |

With the above introduced, this can be simplified to always return
"SUCCESS (0)" or "INVALID_PARAMETERS (-3)". Please use the SMCCC names
to describe standard return values (see section 7.1 in the spec).

> +|                     +----------+----+---------------------------------------------+
> +|                     | (uint64) | R1 | MIDR_EL1 of the selected implementation     |
> +|                     +----------+----+---------------------------------------------+
> +|                     | (uint64) | R2 | REVIDR_EL1 of the selected implementation   |
> +|                     +----------+----+---------------------------------------------+
> +|                     | (uint64) | R3 | Reserved / Must be zero                     |
> ++---------------------+----------+----+---------------------------------------------+

This is missing AIDR_EL1. Although Linux never had to use it, its
purpose is to describe additional implementation details that we may
find relevant in the future.

> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 67f6fdf2e7cd..6c080fa9d345 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -179,6 +179,7 @@
>  #define ARM_SMCCC_KVM_FUNC_PKVM_RESV_62		62
>  #define ARM_SMCCC_KVM_FUNC_PKVM_RESV_63		63
>  /* End of pKVM hypercall range */
> +#define ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS	126

No need to stay at the back of the room, you can happily take slot 64!

>  #define ARM_SMCCC_KVM_FUNC_FEATURES_2		127
>  #define ARM_SMCCC_KVM_NUM_FUNCS			128
>  
> @@ -225,6 +226,12 @@
>  			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
>  			   ARM_SMCCC_KVM_FUNC_MMIO_GUARD)
>  
> +#define ARM_SMCCC_VENDOR_HYP_KVM_DISCOVER_IMPL_CPUS_FUNC_ID		\
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,				\
> +			   ARM_SMCCC_SMC_64,				\
> +			   ARM_SMCCC_OWNER_VENDOR_HYP,			\
> +			   ARM_SMCCC_KVM_FUNC_DISCOVER_IMPL_CPUS)
> +
>  /* ptp_kvm counter type ID */
>  #define KVM_PTP_VIRT_COUNTER			0
>  #define KVM_PTP_PHYS_COUNTER			1

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.