Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: 6i/DvY34nhiow8PBjqidsyf4LC1He06SSXVlt6Vu6BwS32Tw2KBRn1oRwdhJfaMN/807d7Gm5g
 lg5pF+/QWtYg==
IronPort-SDR: KTmbzwYRqU1143COmUSMc9+OGMtwL3lsPG0+NcNGGyVs3xfGObUzjSMCKihL7MdI/OF0qBWxY8
 ZaKijs/d9awA==
Subject: Re: [PATCH][v6] KVM: X86: support APERF/MPERF registers
To:     Li RongQing <lirongqing@baidu.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, x86@kernel.org, hpa@zytor.com, bp@alien8.de,
        mingo@redhat.com, tglx@linutronix.de, jmattson@google.com,
        wanpengli@tencent.com, vkuznets@redhat.com,
        sean.j.christopherson@intel.com, pbonzini@redhat.com,
        wei.huang2@amd.com
References: <1591321466-2046-1-git-send-email-lirongqing@baidu.com>
From:   Xiaoyao Li <xiaoyao.li@intel.com>
Message-ID: <b70d03dd-947f-dee5-5499-3b381372497d@intel.com>
Date:   Fri, 5 Jun 2020 13:00:53 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.8.1
MIME-Version: 1.0
In-Reply-To: <1591321466-2046-1-git-send-email-lirongqing@baidu.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 6/5/2020 9:44 AM, Li RongQing wrote:
> Guest kernel reports a fixed cpu frequency in /proc/cpuinfo,
> this is confused to user when turbo is enable, and aperf/mperf
> can be used to show current cpu frequency after 7d5905dc14a
> "(x86 / CPU: Always show current CPU frequency in /proc/cpuinfo)"
> so guest should support aperf/mperf capability
> 
> This patch implements aperf/mperf by three mode: none, software
> emulation, and pass-through
> 
> None: default mode, guest does not support aperf/mperf
> 
> Software emulation: the period of aperf/mperf in guest mode are
> accumulated as emulated value
> 
> Pass-though: it is only suitable for KVM_HINTS_REALTIME, Because
> that hint guarantees we have a 1:1 vCPU:CPU binding and guaranteed
> no over-commit.
> 
> And a per-VM capability is added to configure aperfmperf mode
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> Signed-off-by: Chai Wen <chaiwen@baidu.com>
> Signed-off-by: Jia Lina <jialina01@baidu.com>
> ---
> diff v5:
> return error if guest is configured with mperf/aperf, but host cpu has not
> 
> diff v4:
> fix maybe-uninitialized warning
> 
> diff v3:
> fix interception of MSR_IA32_MPERF/APERF in svm
> 
> diff v2:
> support aperfmperf pass though
> move common codes to kvm_get_msr_common
> 
> diff v1:
> 1. support AMD, but not test
> 2. support per-vm capability to enable
> 
> 
>   Documentation/virt/kvm/api.rst  | 10 ++++++++++
>   arch/x86/include/asm/kvm_host.h | 11 +++++++++++
>   arch/x86/kvm/cpuid.c            | 15 ++++++++++++++-
>   arch/x86/kvm/svm/svm.c          |  8 ++++++++
>   arch/x86/kvm/vmx/vmx.c          |  6 ++++++
>   arch/x86/kvm/x86.c              | 42 +++++++++++++++++++++++++++++++++++++++++
>   arch/x86/kvm/x86.h              | 15 +++++++++++++++
>   include/uapi/linux/kvm.h        |  1 +
>   8 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index d871dacb984e..f854f4da6fd8 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6126,3 +6126,13 @@ KVM can therefore start protected VMs.
>   This capability governs the KVM_S390_PV_COMMAND ioctl and the
>   KVM_MP_STATE_LOAD MP_STATE. KVM_SET_MP_STATE can fail for protected
>   guests when the state change is invalid.
> +
> +8.23 KVM_CAP_APERFMPERF
> +----------------------------
> +
> +:Architectures: x86
> +:Parameters: args[0] is aperfmperf mode;
> +             0 for not support, 1 for software emulation, 2 for pass-through
> +:Returns: 0 on success; -1 on error
> +
> +This capability indicates that KVM supports APERF and MPERF MSR registers
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fd78bd44b2d6..14643f8af9c4 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -824,6 +824,9 @@ struct kvm_vcpu_arch {
>   
>   	/* AMD MSRC001_0015 Hardware Configuration */
>   	u64 msr_hwcr;
> +
> +	u64 v_mperf;
> +	u64 v_aperf;
>   };
>   
>   struct kvm_lpage_info {
> @@ -889,6 +892,12 @@ enum kvm_irqchip_mode {
>   	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
>   };
>   
> +enum kvm_aperfmperf_mode {
> +	KVM_APERFMPERF_NONE,
> +	KVM_APERFMPERF_SOFT,      /* software emulate aperfmperf */
> +	KVM_APERFMPERF_PT,        /* pass-through aperfmperf to guest */
> +};
> +
>   #define APICV_INHIBIT_REASON_DISABLE    0
>   #define APICV_INHIBIT_REASON_HYPERV     1
>   #define APICV_INHIBIT_REASON_NESTED     2
> @@ -986,6 +995,8 @@ struct kvm_arch {
>   
>   	struct kvm_pmu_event_filter *pmu_event_filter;
>   	struct task_struct *nx_lpage_recovery_thread;
> +
> +	enum kvm_aperfmperf_mode aperfmperf_mode;
>   };
>   
>   struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index cd708b0b460a..80f18b29a845 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -122,6 +122,16 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>   					   MSR_IA32_MISC_ENABLE_MWAIT);
>   	}
>   
> +	best = kvm_find_cpuid_entry(vcpu, 6, 0);
> +	if (best) {
> +		if (guest_has_aperfmperf(vcpu->kvm)) {
> +			if (!boot_cpu_has(X86_FEATURE_APERFMPERF))
> +				return -EINVAL;

kvm_vm_ioctl_enable_cap() ensures that guest_has_aperfmperf() always 
aligns with boot_cpu_has(X86_FEATURE_APERFMPERF). So above is unnecessary.

> +			best->ecx |= 1;
> +		} else {
> +			best->ecx &= ~1;
> +		}
> +	}

you could do

	bool guest_cpuid_aperfmperf = false;
	if (best)
		guest_cpuid_aperfmperf = !!(best->ecx & BIT(0));

	if (guest_cpuid_aperfmerf != guest_has_aperfmperf(vcpu->kvm))
		return -EINVAL;


In fact, I think we can do nothing here. Leave it as what usersapce 
wants just like how KVM treats other CPUID bits.

Paolo,

What's your point?

>   	/* Note, maxphyaddr must be updated before tdp_level. */
>   	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
>   	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);

[...]

> @@ -4930,6 +4939,11 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>   		kvm->arch.exception_payload_enabled = cap->args[0];
>   		r = 0;
>   		break;
> +	case KVM_CAP_APERFMPERF:
> +		kvm->arch.aperfmperf_mode =
> +			boot_cpu_has(X86_FEATURE_APERFMPERF) ? cap->args[0] : 0;

Shouldn't check whether cap->args[0] is a valid value?

> +		r = 0;
> +		break;
>   	default:
>   		r = -EINVAL;
>   		break;