Date: Mon, 11 Apr 2016 22:48:27 +0200
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: pbonzini@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org,
        alex.williamson@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com
Subject: Re: [PART1 RFC v4 06/11] KVM: x86: Detect and Initialize AVIC support
Message-ID: <20160411204823.GA1284@potion.brq.redhat.com>
References: <1460017232-17429-1-git-send-email-Suravee.Suthikulpanit@amd.com>
 <1460017232-17429-7-git-send-email-Suravee.Suthikulpanit@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1460017232-17429-7-git-send-email-Suravee.Suthikulpanit@amd.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3967
Lines: 131

2016-04-07 03:20-0500, Suravee Suthikulpanit:
> This patch introduces AVIC-related data structure, and AVIC
> initialization code.
> 
> There are three main data structures for AVIC:
>     * Virtual APIC (vAPIC) backing page (per-VCPU)
>     * Physical APIC ID table (per-VM)
>     * Logical APIC ID table (per-VM)
> 
> Currently, AVIC is disabled by default. Users can manually
> enable AVIC via kernel boot option kvm-amd.avic=1 or during
> kvm-amd module loading with parameter avic=1.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> @@ -14,6 +14,9 @@
>   * the COPYING file in the top-level directory.
>   *
>   */
> +
> +#define pr_fmt(fmt) "SVM: " fmt
> +
>  #include <linux/kvm_host.h>
>  
>  #include "irq.h"
> @@ -78,6 +81,11 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
>  #define TSC_RATIO_MIN		0x0000000000000001ULL
>  #define TSC_RATIO_MAX		0x000000ffffffffffULL
>  
> +#define AVIC_HPA_MASK	~((0xFFFULL << 52) || 0xFFF)
> +
> +/* NOTE: Current max index allowed for physical APIC ID table is 255 */
> +#define AVIC_PHYSICAL_ID_MAX	0xFF

0xff is broadcast, so shouldn't the actual last one be 0xfe?

> @@ -234,6 +257,18 @@ enum {
>  /* TPR and CR2 are always written before VMRUN */
>  #define VMCB_ALWAYS_DIRTY_MASK	((1U << VMCB_INTR) | (1U << VMCB_CR2))
>  
> +#define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
> +
> +static inline bool svm_vcpu_avic_enabled(struct vcpu_svm *svm)
> +{
> +	return (avic && (svm->vmcb->control.int_ctl & AVIC_ENABLE_MASK));
> +}

I think it'd be better to use kvm_vcpu_apicv_active() instead.
In any case, have just one function to tell whether avic is enabled.

> @@ -1000,6 +1042,24 @@ static void svm_adjust_tsc_offset_guest(struct kvm_vcpu *vcpu, s64 adjustment)
>  	mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
>  }
>  
> +static void avic_init_vmcb(struct vcpu_svm *svm)
> +{
> +	struct vmcb *vmcb = svm->vmcb;
> +	struct kvm_arch *vm_data = &svm->vcpu.kvm->arch;
> +	phys_addr_t bpa = page_to_phys(svm->avic_backing_page);
> +	phys_addr_t lpa = page_to_phys(vm_data->avic_logical_id_table_page);
> +	phys_addr_t ppa = page_to_phys(vm_data->avic_physical_id_table_page);
> +
> +	if (!vmcb)
> +		return;

We can call this function only from init_vmcb and therefore drop this
return.

> @@ -1113,6 +1173,142 @@ static void init_vmcb(struct vcpu_svm *svm)
> +static u64 *avic_get_physical_id_entry(struct kvm_vcpu *vcpu, int index)
> +{
> +	u64 *avic_physical_id_table;
> +	struct kvm_arch *vm_data = &vcpu->kvm->arch;
> +
> +	/* Note: APIC ID = 0xff is used for broadcast.
> +	 *       APIC ID > 0xff is reserved.
> +	 */
> +	if (index >= 0xff)

Isn't this AVIC_PHYSICAL_ID_MAX?

> +static int avic_init_backing_page(struct kvm_vcpu *vcpu)
> +{
> +	int ret;
> +	u64 *entry, new_entry;
> +	int id = vcpu->vcpu_id;
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	ret = avic_init_access_page(vcpu);
> +	if (ret)
> +		return ret;
> +
> +	if (id >= AVIC_PHYSICAL_ID_MAX)
> +		return -EINVAL;
> +
> +	if (!svm->vcpu.arch.apic->regs)
> +		return -EINVAL;
> +
> +	svm->avic_backing_page = virt_to_page(svm->vcpu.arch.apic->regs);
> +
> +	avic_init_vmcb(svm);

avic_init_vmcb() should be dropped from here.  It does nothing.

> +static void avic_vcpu_uninit(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	if (!avic)
> +		return;
> +
> +	if (svm->avic_physical_id_cache)
> +		svm->avic_physical_id_cache = NULL;
> +}

I think it's safe to drop avic_vcpu_uninit().  This function is only
called when we are freeing the vcpu, so the value is not used anymore.

>  static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
>  {
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb *vmcb = svm->vmcb;
> +
> +	if (!avic)
> +		return;
> +
> +	vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK;

(This function doesn't really "refresh" avic, it just disables.
 VMX isn't much better, so it's bearable to do so ...)