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