Re: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for retrieving migration targets

[Oliver Upton <oliver.upton@linux.dev>]

Hi,

On Thu, Oct 24, 2024 at 10:40:10AM +0100, Shameer Kolothum wrote:
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> +------------------------------------------------------
> +
> +Query total number of migration target CPUs the Guest VM will be running during its
> +lifetime and version information applicable to the data format used for
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``. The maximum number of targets
> +supported is 64. Also the version number supported currently is 1.0. This hypercall
> +must be handled by the userspace VMM.
> +
> ++---------------------+-------------------------------------------------------------+
> +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> ++---------------------+-------------------------------------------------------------+
> +| Calling convention: | HVC64                                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Function ID:        | (uint32) | 0xC600007D                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Arguments:          | None                                                        |
> ++---------------------+----------+----+---------------------------------------------+
> +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
> +|                     |          |    | [0-31] total migration targets              |
> +|                     |          |    | [32-63] version number                      |
> ++---------------------+----------+----+---------------------------------------------+

We can't treat a single register as both a signed quantity *and* a full
64 bits of bitfields. Maybe just scrap the version and have this thing
either return a negative error or positive quantity of implementations.

> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> +-------------------------------------------------------
> +
> +Request migration target CPU information for the Guest VM. The information must be
> +provided as per the format described by the version info in
> +``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``. At present, we only support
> +the below format which corresponds to version 1.0. This hypercall will always be
> +preceded by ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID`` and may be
> +invoked multiple times to retrieve the total number of target CPUs information
> +advertised. This hypercall must be handled by the userspace VMM.
> +
> +A typical userspace usage scenario will be like below:
> +
> +1. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_NUM_FUNC_ID``
> +
> +   * Returns total number of migration targets and version number
> +   * Reset current target index to zero
> +2. Receives ``ARM_SMCCC_VENDOR_HYP_KVM_MIGRN_TARGET_CPUS_FUNC_ID``
> +
> +   * Returns MIDR/REVIDR info in return register fields. Can return up to 4
> +   * Update current target index based on returned target info
> +   * If there are remaining register fields, return zero to indicate the end
> +   * Repeat step 2 until current target index == total number of migration targets

Hmm... I'd rather we make the guest supply the target index of the
implementation it wants to discover. Otherwise, the VMM implementation
of this hypercall interface is *stateful* and needs to remember to
migrate that...

> ++---------------------+-------------------------------------------------------------+
> +| Presence:           | Optional; KVM/ARM64 Guests only                             |
> ++---------------------+-------------------------------------------------------------+
> +| Calling convention: | HVC64                                                       |
> ++---------------------+----------+--------------------------------------------------+
> +| Function ID:        | (uint32) | 0xC600007E                                       |
> ++---------------------+----------+----+---------------------------------------------+
> +| Arguments:          | None                                                        |
> ++---------------------+----------+----+---------------------------------------------+
> +| Return Values:      | (int64)  | R0 | ``NOT_SUPPORTED (-1)`` on error, else       |
> +|                     |          |    | [0-31] MIDR, [31-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

Same deal here, x0 needs to always be treated as a signed quantity.

How about -1 on error, 0 on success?

Then, in the remaining registers:

> +|                     | (uint64) | R1 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

Both MIDR_EL1 and REVIDR_EL1 are 64 bit registers in AArch64. So we need
to transfer a full 64 bits, even if the top half is _presently_ RES0 in
MIDR_EL1.

> +|                     | (uint64) | R2 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+
> +|                     | (uint64) | R3 | [0-31] MIDR, [32-63] REVIDR, else           |
> +|                     |          |    | [0-63] Zero to mark end.                    |
> ++---------------------+----------+----+---------------------------------------------+

I think it's fine to have this hypercall return a single MIDR/REVIDR
pair at a time. This is not a performance sensitive interface and will
only be called a few times at boot.

Actually -- what if we crammed everything into a single hypercall?

DISCOVER_IMPLEMENTATION_FUNC_ID

 - Arguments:
   - arg0: selected implementation index

 - Return value:
   - r0: -1 on error, otherwise the maximum possible implementation index
   - r1: MIDR_EL1 of the selected implementation
   - r2: REVIDR_EL1 of the selected implementation

We're guaranteed at least one CPU implementation of course, so the guest
can just start w/ index 0 and iterate from there.

-- 
Thanks,
Oliver