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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi Oliver,

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Friday, October 25, 2024 2:25 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: kvmarm@lists.linux.dev; maz@kernel.org; catalin.marinas@arm.com;
> will@kernel.org; mark.rutland@arm.com; cohuck@redhat.com;
> eric.auger@redhat.com; yuzenghui <yuzenghui@huawei.com>; Wangzhou
> (B) <wangzhou1@hisilicon.com>; jiangkunkun <jiangkunkun@huawei.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Anthony Jebson
> <anthony.jebson@huawei.com>; linux-arm-kernel@lists.infradead.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH v2 1/3] KVM: arm64: Add hypercall support for
> retrieving migration targets
> 
> 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.

Ok. I had  a look at PV_TIME_ST/ARM_SMCCC_VENDOR_HYP_KVM_PTP_FUNC_ID
and got that idea. Separate registers make sense though.

Do we really need to skip the version number? The idea was to use that as a
future proof for data format in case we realize that MIDR/REVIDR is not good
enough for errata later.

> 
> > +``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...

Ok. I had the same feedback from Jonathan in an internal discussion as well
and on re-reading Marc's comments on v1, he also had the same suggestion
that I missed. Will change 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.

Ok.

> 
> > +|                     | (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.

Ok. I will rework based on these and will sent out something soon.

Thanks,
Shameer