Re: [External] Re: [RFC 1/6] KVM: arm64: Document PV-lock interface

[Usama Arif <usama.arif@xxxxxxxxxxxxx>]

Hi,

Thanks for the review. I will include the changes in the next version I 
send for pvlock. I have sent a patch to fix pvtime here 
https://lore.kernel.org/all/20221103131210.3603385-1-usama.arif@xxxxxxxxxxxxx/.

Usama

On 03/11/2022 03:50, Bagas Sanjaya wrote:
> On Wed, Nov 02, 2022 at 04:13:35PM +0000, Usama Arif wrote:
> > +    ============= ========    ==========
> > +    Function ID:  (uint32)    0xC6000020
> > +    PV_call_id:   (uint32)    The function to query for support.
> > +                              Currently only PV_LOCK_PREEMPTED is supported.
> > +    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)     IPA of the pv lock data structure for this
> > +                              VCPU. On failure:
> > +                              NOT_SUPPORTED (-1)
> > +    ============= ========    ==========
> > +
>
> You need to fix up these tables above:
>
> ---- >8 ----
>
> diff --git a/Documentation/virt/kvm/arm/pvlock.rst b/Documentation/virt/kvm/arm/pvlock.rst
> index 766aeef50b2d31..940a1cb221bc90 100644
> --- a/Documentation/virt/kvm/arm/pvlock.rst
> +++ b/Documentation/virt/kvm/arm/pvlock.rst
> @@ -15,21 +15,23 @@ The existence of the PV_LOCK hypercall should be probed using the SMCCC 1.1
>   ARCH_FEATURES mechanism before calling it.
>   
>   PV_LOCK_FEATURES
> -    ============= ========    ==========
> +
> +    ============= ========    =================================================
>       Function ID:  (uint32)    0xC6000020
>       PV_call_id:   (uint32)    The function to query for support.
>                                 Currently only PV_LOCK_PREEMPTED is supported.
>       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)     IPA of the pv lock data structure for this
>                                 VCPU. On failure:
>                                 NOT_SUPPORTED (-1)
> -    ============= ========    ==========
> +    ============= ========    ==========================================
>   
>   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
>
> The similar fixup should also be made to the tables in
> Documentation/virt/kvm/arm/pvtime.rst, though.
>
> > +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 if the VCPU that owns  |
> > +|           |             |             | this 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.
> > +
> > +For the user space interface see Documentation/virt/kvm/devices/vcpu.rst
> > +section "4. GROUP: KVM_ARM_VCPU_PVLOCK_CTRL".
>
> Use reST labels for cross-referencing to the documentation section:
>
> ---- >8 ----
>
> diff --git a/Documentation/virt/kvm/arm/pvlock.rst b/Documentation/virt/kvm/arm/pvlock.rst
> index 940a1cb221bc90..4e9d09b76ef033 100644
> --- a/Documentation/virt/kvm/arm/pvlock.rst
> +++ b/Documentation/virt/kvm/arm/pvlock.rst
> @@ -62,5 +62,5 @@ 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.
>   
> -For the user space interface see Documentation/virt/kvm/devices/vcpu.rst
> -section "4. GROUP: KVM_ARM_VCPU_PVLOCK_CTRL".
> +For the user space interface see :ref:`KVM_VCPU_TSC_CTRL in Generic vcpu
> +interface documentation <kvm-vcpu-tsc-ctrl>`.
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 223ac2fe62f01f..6532f61073a39c 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -194,6 +194,8 @@ base address must be 64 byte aligned and exist within a valid guest memory
>   region. See Documentation/virt/kvm/arm/pvtime.rst for more information
>   including the layout of the stolen time structure.
>   
> +.. _kvm-vcpu-tsc-ctrl:
> +
>   4. GROUP: KVM_VCPU_TSC_CTRL
>   ===========================
>   
>
> Also, you need to add the documentation to table of contents (index):
>
> ---- >8 ----
>
> diff --git a/Documentation/virt/kvm/arm/index.rst b/Documentation/virt/kvm/arm/index.rst
> index e8484843215808..b8499dc00a6a96 100644
> --- a/Documentation/virt/kvm/arm/index.rst
> +++ b/Documentation/virt/kvm/arm/index.rst
> @@ -10,4 +10,5 @@ ARM
>      hyp-abi
>      hypercalls
>      pvtime
> +   pvlock
>      ptp_kvm
>
> Thanks.