linux-kernel - Re: [PART1 RFC v4 06/11] KVM: x86: Detect and Initialize AVIC support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160411204823.GA1284@potion.brq.redhat.com>
Date:	Mon, 11 Apr 2016 22:48:27 +0200
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
Cc:	pbonzini@...hat.com, joro@...tes.org, bp@...en8.de,
	gleb@...nel.org, alex.williamson@...hat.com, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, wei@...hat.com,
	sherry.hurwitz@....com
Subject: Re: [PART1 RFC v4 06/11] KVM: x86: Detect and Initialize AVIC support

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@....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 ...)