RE: [PATCH v5 2/4] KVM: arm64: Introduce hypercall support for retrieving target implementations

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

> -----Original Message-----
> From: Oliver Upton <oliver.upton@linux.dev>
> Sent: Monday, January 27, 2025 5:26 PM
> 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; sebott@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: [PATCH v5 2/4] KVM: arm64: Introduce hypercall support for
> retrieving target implementations
> 
> On Fri, Jan 24, 2025 at 03:17:30PM +0000, Shameer Kolothum wrote:
> > If the Guest requires migration to multiple targets, these hypercalls
> > 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>
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  Documentation/virt/kvm/arm/hypercalls.rst | 59
> +++++++++++++++++++++++
> >  include/linux/arm-smccc.h                 | 15 ++++++
> >  2 files changed, 74 insertions(+)
> >
> > diff --git a/Documentation/virt/kvm/arm/hypercalls.rst
> b/Documentation/virt/kvm/arm/hypercalls.rst
> > index af7bc2c2e0cb..718725cbf6c7 100644
> > --- a/Documentation/virt/kvm/arm/hypercalls.rst
> > +++ b/Documentation/virt/kvm/arm/hypercalls.rst
> > @@ -142,3 +142,62 @@ region is equal to the memory protection granule
> advertised by
> >  |                     |          |    +---------------------------------------------+
> >  |                     |          |    | ``INVALID_PARAMETER (-3)``                  |
> >  +---------------------+----------+----+---------------------------------------------+
> > +
> > +``ARM_SMCCC_VENDOR_HYP_KVM_DISCOVER_IMPL_VER_FUNC_ID``
> > +-------------------------------------------------------
> > +Request the target CPU implementation version information and the
> number of target
> > +implementations for the Guest VM.
> > +
> > ++---------------------+-------------------------------------------------------------+
> > +| Presence:           | Optional;  KVM/ARM64 Guests only                            |
> > ++---------------------+-------------------------------------------------------------+
> > +| Calling convention: | HVC64                                                       |
> > ++---------------------+----------+--------------------------------------------------+
> > +| Function ID:        | (uint32) | 0xC6000040                                       |
> > ++---------------------+----------+--------------------------------------------------+
> > +| Arguments:          | None                                                        |
> > ++---------------------+----------+----+---------------------------------------------+
> > +| Return Values:      | (int64)  | R0 | ``SUCCESS (0)``                             |
> > +|                     |          |    +---------------------------------------------+
> > +|                     |          |    | ``NOT_SUPPORTED (-1)``                      |
> > +|                     +----------+----+---------------------------------------------+
> > +|                     | (uint64) | R1 | Bit[63] Must be zero                        |
> > +|                     |          |    +---------------------------------------------+
> 
> Why? R0 is sufficient to determine if the hypercall was successful, and
> this is already defined as an unsigned quantity to the guest.

Hmm..I was trying to imitate(badly ofcourse) the SMCCC_VERSION where
bit 31 must be zero but again in that case it is Int32 so it makes sense there.

> 
> > +|                     |          |    | Bits [62:32] Major version                  |
> > +|                     |          |    +---------------------------------------------+
> > +|                     |          |    | Bits [31:0] Minor version                   |
> > +|                     +----------+----+---------------------------------------------+
> 
> Do we really need a full u64 of version information? If you collapse
> this down to a u32 it'd align with the versioning scheme for PSCI /
> SMCCC.
 
Ok. How about this,

Bits [63:32]  Reserved/] Must be zero
Bits [31:16]  Major Rev (Should we leave the bit 31 to zero here to match
                      SMCCC_VERSION?)
Bits [15:0]    Minor Rev

Thanks,
Shameer