Re: [PATCH v3] i386: docs: Briefly describe KVM PV features

[Vitaly Kuznetsov]

Igor Mammedov <imammedo@redhat.com> writes:

> On Mon,  4 Oct 2021 16:04:45 +0200
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>

Thanks for the review! As I can see, the patch already made it to
'master':

commit 7f7c8d0ce3630849a4df3d627b11de354fcb3bb0
Author: Vitaly Kuznetsov <vkuznets@redhat.com>
Date:   Mon Oct 4 16:04:45 2021 +0200

    i386: docs: Briefly describe KVM PV features

we can send follow-ups, of course. 

>> KVM PV features don't seem to be documented anywhere, in particular, the
>> fact that some of the features are enabled by default and some are not can
>> only be figured out from the code.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> Changes since "[PATCH v2 0/8] i386: Assorted KVM PV and Hyper-V feature
>>  improvements" [Paolo Bonzini]:
>> - Convert to 'rst' and move to docs/system/i386/kvm-pv.rst.
>> - Add information about the version of Linux that introduced the particular
>>   PV feature.
>> ---
>>  docs/system/i386/kvm-pv.rst | 100 ++++++++++++++++++++++++++++++++++++
>>  docs/system/target-i386.rst |   1 +
>>  2 files changed, 101 insertions(+)
>>  create mode 100644 docs/system/i386/kvm-pv.rst
>> 
>> diff --git a/docs/system/i386/kvm-pv.rst b/docs/system/i386/kvm-pv.rst
>> new file mode 100644
>> index 000000000000..1e5a9923ef45
>> --- /dev/null
>> +++ b/docs/system/i386/kvm-pv.rst
>> @@ -0,0 +1,100 @@
>> +Paravirtualized KVM features
>> +============================
>> +
>> +Description
>> +-----------
>> +
>> +In some cases when implementing hardware interfaces in software is slow, 
>> ``KVM``
>> +implements its own paravirtualized interfaces.
>> +
>> +Setup
>> +-----
>> +
>> +Paravirtualized ``KVM`` features are represented as CPU flags. The following
>> +features are enabled by default for any CPU model when ``KVM`` acceleration 
>> is
>> +enabled:
>
> /if host kernel supports them
>

It does as QEMU requires linux >= 4.5. I'm not sure what happens if it
doesn't, maybe it won't start. 

>> +
>> +- ``kvmclock``
>> +- ``kvm-nopiodelay``
>
>> +- ``kvm-asyncpf``
>
> later you say it's not enabled by default since x.y and something else
> should be used instead

The situation is a bit weird. QEMU will still be enabling kvm-asyncpf by
default. This, however, has no effect currently as KVM dropped support
for this feature (in favor of kvm-asyncpf-int but this one is *not*
enabled by default)

>
> maybe add a kernel version for each item in this list aka: (since: ... 
> [,till])
>
>> +- ``kvm-steal-time``
>> +- ``kvm-pv-eoi``
>> +- ``kvmclock-stable-bit``
>> +
>> +``kvm-msi-ext-dest-id`` feature is enabled by default in x2apic mode with 
>> split
>> +irqchip (e.g. "-machine ...,kernel-irqchip=split -cpu ...,x2apic").
>
>
>> +Note: when CPU model ``host`` is used, QEMU passes through all supported
>> +paravirtualized ``KVM`` features to the guest.
>
> Is it true in case of kvm-pv-enforce-cpuid=on ?

Yes, I believe these two things are orthogonal: CPU model 'host' will
give you everything supported by the kernel, 'kvm-pv-enforce-cpuid' will
tell KVM to forbid using features, not exposed in guest visible CPUIDs:
but combined with 'host' this is going to be an empty set as all
features are enabled.

>
> Also I'd s/passes through/enables/
> on the grounds that host CPUID simply doesn't have such CPUIDs
> so it's a bit confusing.

I meant to say 'passes through' from KVM, not from pCPU but I see why
this is not clear.

>
>
>> +Existing features
>> +-----------------
>> +
>> +``kvmclock``
>> +  Expose a ``KVM`` specific paravirtualized clocksource to the guest. 
>> Supported
>> +  since Linux v2.6.26.
>> +
>> +``kvm-nopiodelay``
>> +  The guest doesn't need to perform delays on PIO operations. Supported 
>> since
>> +  Linux v2.6.26.
>> +
>> +``kvm-mmu``
>> +  This feature is deprecated.
>> +
>> +``kvm-asyncpf``
>> +  Enable asynchronous page fault mechanism. Supported since Linux v2.6.38.
>> +  Note: since Linux v5.10 the feature is deprecated and not enabled by 
>> ``KVM``.
>
>> +  Use ``kvm-asyncpf-int`` instead.
> 'Use' or 'Used' by default?
>

'kvm-asyncpf' is a dead feature now so in case users want to get
Asynchronouse Page Faults they need to enable 'kvm-asyncpf-int'
manually, thus 'use'.

>
>> +``kvm-steal-time``
>> +  Enable stolen (when guest vCPU is not running) time accounting. Supported
>> +  since Linux v3.1.
>> +
>> +``kvm-pv-eoi``
>> +  Enable paravirtualized end-of-interrupt signaling. Supported since Linux
>> +  v3.10.
>> +
>> +``kvm-pv-unhalt``
>> +  Enable paravirtualized spinlocks support. Supported since Linux v3.12.
>> +
>> +``kvm-pv-tlb-flush``
>> +  Enable paravirtualized TLB flush mechanism. Supported since Linux v4.16.
>> +
>> +``kvm-pv-ipi``
>> +  Enable paravirtualized IPI mechanism. Supported since Linux v4.19.
>> +
>> +``kvm-poll-control``
>> +  Enable host-side polling on HLT control from the guest. Supported since 
>> Linux
>> +  v5.10.
>> +
>> +``kvm-pv-sched-yield``
>> +  Enable paravirtualized sched yield feature. Supported since Linux v5.10.
>> +
>> +``kvm-asyncpf-int``
>> +  Enable interrupt based asynchronous page fault mechanism. Supported since 
>> Linux
>> +  v5.10.
>> +
>> +``kvm-msi-ext-dest-id``
>> +  Support 'Extended Destination ID' for external interrupts. The feature 
>> allows
>> +  to use up to 32768 CPUs without IRQ remapping (but other limits may apply 
>> making
>
> maybe add a footnote pointing to what 'other limits' may exist.

A footnote would also do. The main point here is that this feature by
itself doesn't guarantee that the limit for the number of vCPUs is
'32768', hope this is clear.

>
>> +  the number of supported vCPUs for a given configuration lower). Supported 
>> since
>> +  Linux v5.10.
>> +
>> +``kvmclock-stable-bit``
>> +  Tell the guest that guest visible TSC value can be fully trusted for 
>> kvmclock
>> +  computations and no warps are expected. Supported since Linux v2.6.35.
>> +
>> +Supplementary features
>> +----------------------
>> +
>> +``kvm-pv-enforce-cpuid``
>> +  Limit the supported paravirtualized feature set to the exposed features 
>> only.
>
> Does 'the exposed features' mean feature flags explicitly set for CPU
> on command line?

Explicitly enabled + features, enabled by default OR '-cpu host' (all
features). 'Exposed' here is an implementation detail: KVM doesn't know
how guest visible CPUIDs were built, the only thing it does is every
time a feature is used (e.g. a PV MSR is being written to) it checks if
the feature bit is set. I'd appreciate a suggestion on how to word this
better from QEMU's point of view.

>
>> +  Note, by default, ``KVM`` allows the guest to use all currently supported
>> +  paravirtualized features even when they were not announced in guest 
>> visible
>> +  CPUIDs. Supported since Linux v5.10.
>> +
>> +
>> +Useful links
>> +------------
>> +
>> +Please refer to Documentation/virt/kvm in Linux for additional details.
>> diff --git a/docs/system/target-i386.rst b/docs/system/target-i386.rst
>> index 6a86d638633a..4daa53c35d8f 100644
>> --- a/docs/system/target-i386.rst
>> +++ b/docs/system/target-i386.rst
>> @@ -26,6 +26,7 @@ Architectural features
>>     :maxdepth: 1
>>  
>>     i386/cpu
>> +   i386/kvm-pv
>>     i386/sgx
>>  
>>  .. _pcsys_005freq:
>

-- 
Vitaly