Re: [PATCH v6 01/10] KVM: arm64: Document PV-time interface

[Steven Price]

On 18/10/2019 18:10, Mark Rutland wrote:
> On Tue, Oct 15, 2019 at 06:56:51PM +0100, Mark Rutland wrote:
[...]
>>> +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.
> 
> I guess we can say that it's userspace's responsibiltiy to set this up
> with sufficient alignment, but I do think we want to make a
> recommendataion here.

I can add something like this to the kernel's documentation:

    It is advisable that one or more 64k pages are set aside for the
    purpose of these structures and not used for other purposes, this
    enables the guest to map the region using 64k pages and avoids
    conflicting attributes with other memory.

> [...]
> 
>>> +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?
> 
> I'm assuming that this has been communicated internally, and we can
> assume the next rev of the spec will state so.

Yes I've fed that back, so hopefully it should be in the next rev of the
spec.

> Assuming so, this looks good to me.

Great, thanks for the review.

Steve