[prev in list] [next in list] [prev in thread] [next in thread] 

List:       qemu-devel
Subject:    Re: [RFC PATCH] target/i386: kvm: Enable KVM_FEATURE_PCI_GO_MMCONFIG CPUID bit
From:       Eduardo Habkost <ehabkost () redhat ! com>
Date:       2020-07-31 19:48:40
Message-ID: 20200731194840.GI225270 () habkost ! net
[Download RAW message or body]

On Fri, Jul 31, 2020 at 08:49:38PM +0200, Julia Suvorova wrote:
> This feature allows MMCONFIG to be used even to access the base PCI
> config space [1]. This means increased performance: one access to
> MMCONFIG instead of two conventional accesses to I/O ports.
> 
> Q35 makes no distinction in base or extended PCI config access to
> MMCONFIG, MMCONFIG is always on, and in case it is is not initialized,
> probing of PCI devices will fall back to normal process and use type1
> access.
> 
> Enable the feature unconditionally.
> 
> [1]: https://lore.kernel.org/kvm/20200730193510.578309-1-jusual@redhat.com/
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
> The feature is in the review phase.
> 
>  include/standard-headers/asm-x86/kvm_para.h | 1 +
>  target/i386/cpu.c                           | 3 ++-
>  target/i386/kvm.c                           | 1 +
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/include/standard-headers/asm-x86/kvm_para.h b/include/standard-headers/asm-x86/kvm_para.h
> index 07877d3295..52eeba9067 100644
> --- a/include/standard-headers/asm-x86/kvm_para.h
> +++ b/include/standard-headers/asm-x86/kvm_para.h
> @@ -32,6 +32,7 @@
>  #define KVM_FEATURE_POLL_CONTROL	12
>  #define KVM_FEATURE_PV_SCHED_YIELD	13
>  #define KVM_FEATURE_ASYNC_PF_INT	14
> +#define KVM_FEATURE_PCI_GO_MMCONFIG	15
>  
>  #define KVM_HINTS_REALTIME      0
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 588f32e136..5509523bb3 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -810,7 +810,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
>              "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
>              "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
>              NULL, "kvm-pv-tlb-flush", NULL, "kvm-pv-ipi",
> -            "kvm-poll-control", "kvm-pv-sched-yield", NULL, NULL,
> +            "kvm-poll-control", "kvm-pv-sched-yield", NULL, "kvm-pci-go-mmconfig",
>              NULL, NULL, NULL, NULL,
>              NULL, NULL, NULL, NULL,
>              "kvmclock-stable-bit", NULL, NULL, NULL,
> @@ -4141,6 +4141,7 @@ static PropValue kvm_default_props[] = {
>      { "acpi", "off" },
>      { "monitor", "off" },
>      { "svm", "off" },
> +    { "kvm-pci-go-mmconfig", "on" },

You'll need a TYPE_X86_CPU.kvm-pci-go-mmconfig=off entry in
pc_compat_5_1 to keep guest ABI compatibility on older machine
types.

pc_compat_5_1 is introduced by
https://lore.kernel.org/qemu-devel/20200728094645.272149-1-cohuck@redhat.com/
("hw: add compat machines for 5.2").

>      { NULL, NULL },
>  };
>  
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 6f18d940a5..0069e945e6 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -440,6 +440,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
>          if (!kvm_irqchip_in_kernel()) {
>              ret &= ~(1U << KVM_FEATURE_PV_UNHALT);
>          }
> +        ret |= 1U << KVM_FEATURE_PCI_GO_MMCONFIG;

On most cases, enabling a feature unconditionally on
kvm_arch_get_supported_cpuid() would be a mistake, but this flag
seems to be an exception to the rule.

A comment here explaining why it is really safe to enable it
unconditionally would be welcome.

>      } else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
>          ret |= 1U << KVM_HINTS_REALTIME;
>          found = 1;
> -- 
> 2.25.4
> 

-- 
Eduardo


[prev in list] [next in list] [prev in thread] [next in thread]