Re: [PATCH v2 1/6] KVM: arm64: Document PV-lock interface

[yezengruan]

Hi Steve,

On 2020/1/9 22:53, Steven Price wrote:
> On 26/12/2019 13:58, Zengruan Ye wrote:
>> Introduce a paravirtualization interface for KVM/arm64 to obtain the VCPU
>> is currently running or not.
>>
>> The PV lock structure of the guest is allocated by user space.
>>
>> 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: Zengruan Ye <yezengruan@xxxxxxxxxx>
>> ---
>> Â Documentation/virt/kvm/arm/pvlock.rstÂÂ | 63 +++++++++++++++++++++++++
>> Â Documentation/virt/kvm/devices/vcpu.txt | 14 ++++++
>> Â 2 files changed, 77 insertions(+)
>> Â create mode 100644 Documentation/virt/kvm/arm/pvlock.rst
>>
>> diff --git a/Documentation/virt/kvm/arm/pvlock.rst b/Documentation/virt/kvm/arm/pvlock.rst
>> new file mode 100644
>> index 000000000000..58b3b8ee7537
>> --- /dev/null
>> +++ b/Documentation/virt/kvm/arm/pvlock.rst
>> @@ -0,0 +1,63 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Paravirtualized lock support for arm64
>> +======================================
>> +
>> +KVM/arm64 provides some hypervisor service calls to support a paravirtualized
>> +guest obtaining the VCPU is currently running or not.
> NIT:ÂÂÂÂÂÂÂÂÂÂÂÂÂ ^ whether

Thanks for posting this.

> 
>> +
>> +Two new SMCCC compatible hypercalls are defined:
>> +
>> +* PV_LOCK_FEATURES:ÂÂ 0xC6000020
>> +* PV_LOCK_PREEMPTED:Â 0xC6000021
>> +
>> +The existence of the PV_LOCK hypercall should be probed using the SMCCC 1.1
>> +ARCH_FEATURES mechanism before calling it.
> 
> Since these are within the "vendor specific" SMCCC region ideally you should also check that you are talking to KVM. (Other hypervisors could allocate SMCCC IDs differently within this block). Will has a patch on a branch which gives an example of how this could work [1]
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git/commit/?h=kvm/hvc&id=464f5a1741e5959c3e4d2be1966ae0093b4dce06

OK, I will add "vendor specific" check next version.

> 
>> +
>> +PV_LOCK_FEATURES
>> +ÂÂÂ ============= ========ÂÂÂ ==========
>> +ÂÂÂ Function ID:Â (uint32)ÂÂÂ 0xC6000020
>> +ÂÂÂ PV_call_id:ÂÂ (uint32)ÂÂÂ The function to query for support.
>> +ÂÂÂ Return value: (int64)ÂÂÂÂ NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ PV-lock feature is supported by the hypervisor.
>> +ÂÂÂ ============= ========ÂÂÂ ==========
>> +
>> +PV_LOCK_PREEMPTED
>> +ÂÂÂ ============= ========ÂÂÂ ==========
>> +ÂÂÂ Function ID:Â (uint32)ÂÂÂ 0xC6000021
>> +ÂÂÂ Return value: (int64)ÂÂÂÂ NOT_SUPPORTED (-1) or SUCCESS (0) if the IPA of
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ this VCPU's pv data structure is configured by
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ the hypervisor.
>> +ÂÂÂ ============= ========ÂÂÂ ==========
> 
> PV_LOCK_PREEMPTED also needs to return the address of this data structure. Either by returning this in another register, or by e.g. treating a positive return as an address and a negative value as an error.

This is somewhat embarrassing. The code does what you say, but the doc doesn't. Thanks for pointing it out to me! I'll update the doc to match.

> 
>> +
>> +The IPA returned by PV_LOCK_PREEMPTED should be mapped by the guest as normal
>> +memory with inner and outer write back caching attributes, in the inner
>> +shareable domain.
>> +
>> +PV_LOCK_PREEMPTED returns the structure for the calling VCPU.
>> +
>> +PV lock state
>> +-------------
>> +
>> +The structure pointed to by the PV_LOCK_PREEMPTED hypercall is as follows:
>> +
>> ++-----------+-------------+-------------+---------------------------------+
>> +| FieldÂÂÂÂ | Byte Length | Byte Offset | DescriptionÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |
>> ++===========+=============+=============+=================================+
>> +| preempted |ÂÂÂÂÂ 8ÂÂÂÂÂ |ÂÂÂÂÂ 0ÂÂÂÂÂ | Indicate the VCPU who owns this |
> 
> NIT: s/Indicate/Indicates that/. Also more common English would be "the VCPU *that* owns"

Will update.

> 
>> +|ÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ | struct is running or not.ÂÂÂÂÂÂ |
>> +|ÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ | Non-zero values mean the VCPUÂÂ |
>> +| | | | has been preempted. Zero means |
>> +|ÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂÂÂÂÂÂÂÂÂÂÂ | the VCPU is not preempted.ÂÂÂÂÂ |
>> ++-----------+-------------+-------------+---------------------------------+
>> +
>> +The preempted field will be updated to 1 by the hypervisor prior to scheduling
>> +a VCPU. When the VCPU is scheduled out, the preempted field will be updated
>> +to 0 by the hypervisor.
>> +
>> +The structure will be present within a reserved region of the normal memory
>> +given to the guest. The guest should not attempt to write into this memory.
>> +There is a structure per VCPU of the guest.
> 
> I think it would be worth mentioning in this document that the structure is guaranteed to be 64-byte aligned.

Good point, I'll update the doc.

> 
> Steve
> 
>> +
>> +For the user space interface see Documentation/virt/kvm/devices/vcpu.txt
>> +section "4. GROUP: KVM_ARM_VCPU_PVLOCK_CTRL".
>> diff --git a/Documentation/virt/kvm/devices/vcpu.txt b/Documentation/virt/kvm/devices/vcpu.txt
>> index 6f3bd64a05b0..c10a5945075b 100644
>> --- a/Documentation/virt/kvm/devices/vcpu.txt
>> +++ b/Documentation/virt/kvm/devices/vcpu.txt
>> @@ -74,3 +74,17 @@ Specifies the base address of the stolen time structure for this VCPU. The
>> Â base address must be 64 byte aligned and exist within a valid guest memory
>> Â region. See Documentation/virt/kvm/arm/pvtime.txt for more information
>> Â including the layout of the stolen time structure.
>> +
>> +4. GROUP: KVM_ARM_VCPU_PVLOCK_CTRL
>> +Architectures: ARM64
>> +
>> +4.1 ATTRIBUTE: KVM_ARM_VCPU_PVLOCK_IPA
>> +Parameters: 64-bit base address
>> +Returns: -ENXIO:Â PV lock not implemented
>> +ÂÂÂÂÂÂÂÂ -EEXIST: Base address already set for this VCPU
>> +ÂÂÂÂÂÂÂÂ -EINVAL: Base address not 64 byte aligned
>> +
>> +Specifies the base address of the PV lock structure for this VCPU. The
>> +base address must be 64 byte aligned and exist within a valid guest memory
>> +region. See Documentation/virt/kvm/arm/pvlock.rst for more information
>> +including the layout of the pv lock structure.
>>
> 
> 
> .

Thanks,

Zengruan