Message-ID: <20191015175651.GF24604@lakrids.cambridge.arm.com>
Date:   Tue, 15 Oct 2019 18:56:51 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Steven Price <steven.price@....com>
Cc:     Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        James Morse <james.morse@....com>,
        Julien Thierry <julien.thierry.kdev@...il.com>,
        Suzuki K Pouloze <suzuki.poulose@....com>,
        kvm@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 01/10] KVM: arm64: Document PV-time interface

Hi Steven,

On Fri, Oct 11, 2019 at 01:59:21PM +0100, Steven Price wrote:
> Introduce a paravirtualization interface for KVM/arm64 based on the
> "Arm Paravirtualized Time for Arm-Base Systems" specification DEN 0057A.

I notice that as published, this is a BETA Draft, with the explicit
note:

| This document is for review purposes only and should not be used
| for any implementation as changes are likely.

... what's the plan for getting a finalised version published?

> This only adds the details about "Stolen Time" as the details of "Live
> Physical Time" have not been fully agreed.

... and what do we expect to happen on this front?

AFAICT, the spec hasn't changed since I called out issues in that area:

  https://lore.kernel.org/r/20181210114047.tifwh6ilwzphsbqy@lakrids.cambridge.arm.com

... and I'd feel much happier about supporting this if that were dropped
from the finalised spec.

> User space can specify a reserved area of memory for the guest and
> inform KVM to populate the memory with information on time that the host
> kernel has stolen from the guest.
> 
> A hypercall interface is provided for the guest to interrogate the
> hypervisor's support for this interface and the location of the shared
> memory structures.
> 
> Signed-off-by: Steven Price <steven.price@....com>
> ---
>  Documentation/virt/kvm/arm/pvtime.rst   | 77 +++++++++++++++++++++++++
>  Documentation/virt/kvm/devices/vcpu.txt | 14 +++++
>  2 files changed, 91 insertions(+)
>  create mode 100644 Documentation/virt/kvm/arm/pvtime.rst
> 
> diff --git a/Documentation/virt/kvm/arm/pvtime.rst b/Documentation/virt/kvm/arm/pvtime.rst
> new file mode 100644
> index 000000000000..de949933ec78
> --- /dev/null
> +++ b/Documentation/virt/kvm/arm/pvtime.rst
> @@ -0,0 +1,77 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Paravirtualized time support for arm64
> +======================================
> +
> +Arm specification DEN0057/A defines a standard for paravirtualised time
> +support for AArch64 guests:
> +
> +https://developer.arm.com/docs/den0057/a
> +
> +KVM/arm64 implements the stolen time part of this specification by providing
> +some hypervisor service calls to support a paravirtualized guest obtaining a
> +view of the amount of time stolen from its execution.
> +
> +Two new SMCCC compatible hypercalls are defined:
> +
> +* PV_TIME_FEATURES: 0xC5000020
> +* PV_TIME_ST:       0xC5000021
> +
> +These are only available in the SMC64/HVC64 calling convention as
> +paravirtualized time is not available to 32 bit Arm guests. The existence of
> +the PV_FEATURES hypercall should be probed using the SMCCC 1.1 ARCH_FEATURES
> +mechanism before calling it.
> +
> +PV_TIME_FEATURES
> +    ============= ========    ==========
> +    Function ID:  (uint32)    0xC5000020
> +    PV_call_id:   (uint32)    The function to query for support.
> +                              Currently only PV_TIME_ST is supported.
> +    Return value: (int64)     NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
> +                              PV-time feature is supported by the hypervisor.
> +    ============= ========    ==========
> +
> +PV_TIME_ST
> +    ============= ========    ==========
> +    Function ID:  (uint32)    0xC5000021
> +    Return value: (int64)     IPA of the stolen time data structure for this
> +                              VCPU. On failure:
> +                              NOT_SUPPORTED (-1)
> +    ============= ========    ==========
> +
> +The IPA returned by PV_TIME_ST should be mapped by the guest as normal memory
> +with inner and outer write back caching attributes, in the inner shareable
> +domain. A total of 16 bytes from the IPA returned are guaranteed to be
> +meaningfully filled by the hypervisor (see structure below).

At what granularity is this allowed to share IPA space with other
mappings? The spec doesn't provide any guidance here, and I strongly
suspect that it should.

To support a 64K guest, we must ensure that this doesn't share a 64K IPA
granule with any MMIO, and it probably only makes sense for an instance
of this structure to share that granule with another vCPU's structure.

We probably _also_ want to ensure that this doesn't share a 64K granule
with memory the guest sees as regular system RAM. Otherwise we're liable
to force it into having mismatched attributes for any of that RAM it
happens to map as part of mapping the PV_TIME_ST structure.

> +
> +PV_TIME_ST returns the structure for the calling VCPU.
> +
> +Stolen Time
> +-----------
> +
> +The structure pointed to by the PV_TIME_ST hypercall is as follows:
> +
> ++-------------+-------------+-------------+----------------------------+
> +| Field       | Byte Length | Byte Offset | Description                |
> ++=============+=============+=============+============================+
> +| Revision    |      4      |      0      | Must be 0 for version 1.0  |
> ++-------------+-------------+-------------+----------------------------+
> +| Attributes  |      4      |      4      | Must be 0                  |
> ++-------------+-------------+-------------+----------------------------+
> +| Stolen time |      8      |      8      | Stolen time in unsigned    |
> +|             |             |             | nanoseconds indicating how |
> +|             |             |             | much time this VCPU thread |
> +|             |             |             | was involuntarily not      |
> +|             |             |             | running on a physical CPU. |
> ++-------------+-------------+-------------+----------------------------+
> +
> +All values in the structure are stored little-endian.

Looking at the published DEN 0057A, endianness is never stated. Is this
going to be corrected in the next release?

Thanks,
Mark.

Powered by blists - more mailing lists