Subject: Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>, rkrcmar@redhat.com,
        joro@8bytes.org, bp@alien8.de, gleb@kernel.org,
        alex.williamson@redhat.com
References: <1457124368-2025-1-git-send-email-Suravee.Suthikulpanit@amd.com>
 <1457124368-2025-8-git-send-email-Suravee.Suthikulpanit@amd.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com,
        sherry.hurwitz@amd.com
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56DDA50B.7090609@redhat.com>
Date: Mon, 7 Mar 2016 16:58:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <1457124368-2025-8-git-send-email-Suravee.Suthikulpanit@amd.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3942
Lines: 138


On 04/03/2016 21:46, Suravee Suthikulpanit wrote:
> +#define SVM_EXIT_AVIC_INCMP_IPI 0x401
> +#define SVM_EXIT_AVIC_NOACCEL  0x402
> 
> +enum avic_incmp_ipi_err_code {
> +	AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE,
> +	AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN,
> +	AVIC_INCMP_IPI_ERR_INV_TARGET,
> +	AVIC_INCMP_IPI_ERR_INV_BK_PAGE,
> +};
> +

Please do not abbreviate names and use the same definition as the
manual; these are "IPI delivery failure causes", which we can shorten to
"IPI failure causes".  Putting it together gives:

#define SVM_EXIT_AVIC_INCOMPLETE_IPI 0x401
#define SVM_EXIT_AVIC_UNACCELERATED_ACCESS 0x402

enum avic_ipi_failure_cause {
	AVIC_IPI_FAILURE_INVALID_INT_TYPE,
	AVIC_IPI_FAILURE_TARGET_NOT_RUNNING,
	AVIC_IPI_FAILURE_INVALID_TARGET,
	AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
};

Likewise, do not abbreviate function names.

> +#define APIC_SHORT_MASK			0xc0000
> +#define APIC_DEST_MASK			0x800

Please move these to lapic.h too in patch 1.

> +	case AVIC_INCMP_IPI_ERR_INV_TARGET:
> +		pr_err("%s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
> +		       __func__, icrh, icrl, index);
> +		BUG();
> +		break;
> +	case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
> +		pr_err("%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> +		       __func__, icrh, icrl, index);
> +		BUG();
> +		break;

Please use WARN(1, "%s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
__func__, icrh, icrl, index) (and likewise for invalid target) instead
of BUG().

> 
> +	pr_debug("%s: offset=%#x, val=%#x, (cpu=%x) (vcpu_id=%x)\n",
> +		 __func__, offset, reg, svm->vcpu.cpu, svm->vcpu.vcpu_id);
> +
> +	switch (offset) {
> +	case APIC_ID: {
> +		u32 aid = (reg >> 24) & 0xff;
> +		struct svm_avic_phy_ait_entry *o_ent =
> +				avic_get_phy_ait_entry(&svm->vcpu, svm->vcpu.vcpu_id);
> +		struct svm_avic_phy_ait_entry *n_ent =
> +				avic_get_phy_ait_entry(&svm->vcpu, aid);
> +
> +		if (!n_ent || !o_ent)
> +			return 0;
> +
> +		pr_debug("%s: APIC_ID=%#x (id=%x)\n", __func__, reg, aid);
> +
> +		/* We need to move phy_apic_entry to new offset */
> +		*n_ent = *o_ent;
> +		*((u64 *)o_ent) = 0ULL;
> +		break;
> +	}
> +	case APIC_LDR: {
> +		int ret, lid;
> +		int dlid = (reg >> 24) & 0xff;
> +
> +		if (!dlid)
> +			return 0;
> +
> +		lid = ffs(dlid) - 1;
> +		pr_debug("%s: LDR=%0#10x (lid=%x)\n", __func__, reg, lid);
> +		ret = avic_init_log_apic_entry(&svm->vcpu, svm->vcpu.vcpu_id,
> +					       lid);
> +		if (ret)
> +			return 0;
> +
> +		break;
> +	}
> +	case APIC_DFR: {
> +		u32 mod = (*avic_get_bk_page_entry(svm, offset) >> 28) & 0xf;
> +
> +		pr_debug("%s: DFR=%#x (%s)\n", __func__,
> +			 mod, (mod == 0xf) ? "flat" : "cluster");
> +
> +		/*
> +		 * We assume that all local APICs are using the same type.
> +		 * If this changes, we need to rebuild the AVIC logical
> +		 * APID id table with subsequent write to APIC_LDR.
> +		 */
> +		if (vm_data->ldr_mode != mod) {
> +			clear_page(page_address(vm_data->avic_log_ait_page));
> +			vm_data->ldr_mode = mod;
> +		}
> +		break;
> +	}
> +	case APIC_TMICT: {
> +		u32 val = kvm_apic_get_reg(apic, APIC_TMICT);
> +
> +		pr_debug("%s: TMICT=%#x,%#x\n", __func__, val, reg);
> +		break;
> +	}
> +	case APIC_ESR: {
> +		u32 val = kvm_apic_get_reg(apic, APIC_ESR);
> +
> +		pr_debug("%s: ESR=%#x,%#x\n", __func__, val, reg);
> +		break;
> +	}
> +	case APIC_LVTERR: {
> +		u32 val = kvm_apic_get_reg(apic, APIC_LVTERR);
> +
> +		pr_debug("%s: LVTERR=%#x,%#x\n", __func__, val, reg);
> +		break;
> +	}
> +	default:
> +		break;

Please use a single tracepoint instead of all these pr_debug statements.
 The tracepoint can convert APIC register offsets to APIC register
names, like the existing kvm_apic tracepoint.  Also please remove the
TMICT/ESR/LVTERR cases, since they do nothing but debugging.

Notice how a single tracepoint can be used for multiple functions, the
example being kvm_avic as well.

Existing tracepoints in fact make most of the pr_debug statements
unnecessary.

Paolo