From: Paolo Bonzini <pbonzini@redhat.com>
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>,
rkrcmar@redhat.com, joro@8bytes.org, bp@alien8.de,
gleb@kernel.org, alex.williamson@redhat.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
wei@redhat.com, sherry.hurwitz@amd.com
Subject: Re: [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC
Date: Mon, 7 Mar 2016 16:58:03 +0100 [thread overview]
Message-ID: <56DDA50B.7090609@redhat.com> (raw)
In-Reply-To: <1457124368-2025-8-git-send-email-Suravee.Suthikulpanit@amd.com>
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
next prev parent reply other threads:[~2016-03-07 15:58 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-04 20:45 [PART1 RFC v2 00/10] KVM: x86: Introduce SVM AVIC support Suravee Suthikulpanit
2016-03-04 20:45 ` [PART1 RFC v2 01/10] KVM: x86: Misc LAPIC changes to exposes helper functions Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 02/10] KVM: x86: Introducing kvm_x86_ops VCPU blocking/unblocking Suravee Suthikulpanit
2016-03-07 15:42 ` Paolo Bonzini
2016-03-14 6:19 ` Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 03/10] svm: Introduce new AVIC VMCB registers Suravee Suthikulpanit
2016-03-07 15:44 ` Paolo Bonzini
2016-03-14 7:41 ` Suravee Suthikulpanit
2016-03-14 12:25 ` Paolo Bonzini
2016-03-15 12:51 ` Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 04/10] svm: clean up V_TPR, V_IRQ, V_INTR_PRIO, and V_INTR_MASKING Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 05/10] KVM: x86: Detect and Initialize AVIC support Suravee Suthikulpanit
2016-03-07 16:41 ` Paolo Bonzini
2016-03-15 17:09 ` Suravee Suthikulpanit
2016-03-15 17:22 ` Paolo Bonzini
2016-03-16 6:22 ` Suravee Suthikulpanit
2016-03-16 7:20 ` Paolo Bonzini
2016-03-16 8:21 ` Suravee Suthikulpanit
2016-03-16 11:12 ` Paolo Bonzini
2016-03-04 20:46 ` [PART1 RFC v2 06/10] svm: Add interrupt injection via AVIC Suravee Suthikulpanit
2016-03-07 15:36 ` Paolo Bonzini
2016-03-08 21:54 ` Radim Krčmář
2016-03-09 11:10 ` Paolo Bonzini
2016-03-09 16:00 ` Radim Krčmář
2016-03-14 9:41 ` Suravee Suthikulpanit
2016-03-14 12:27 ` Paolo Bonzini
2016-03-14 9:50 ` Suravee Suthikulpanit
2016-03-14 5:25 ` Suravee Suthikulpanit
2016-03-14 8:54 ` Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 07/10] svm: Add VMEXIT handlers for AVIC Suravee Suthikulpanit
2016-03-07 15:58 ` Paolo Bonzini [this message]
2016-03-08 22:05 ` Radim Krčmář
2016-03-09 10:56 ` Paolo Bonzini
2016-03-09 20:55 ` Radim Krčmář
2016-03-10 19:34 ` Radim Krčmář
2016-03-10 19:54 ` Paolo Bonzini
2016-03-10 20:44 ` Radim Krčmář
2016-03-17 3:58 ` Suravee Suthikulpanit
2016-03-17 9:35 ` Paolo Bonzini
2016-03-17 19:44 ` Suravee Suthikulpanit
2016-03-17 20:27 ` [PATCH] KVM: split kvm_vcpu_wake_up from kvm_vcpu_kick Radim Krčmář
2016-03-18 5:13 ` Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 08/10] svm: Do not expose x2APIC when enable AVIC Suravee Suthikulpanit
2016-03-04 20:46 ` [PART1 RFC v2 09/10] svm: Do not intercept CR8 " Suravee Suthikulpanit
2016-03-07 15:39 ` Paolo Bonzini
2016-03-14 6:09 ` Suravee Suthikulpanit
2016-03-14 12:28 ` Paolo Bonzini
2016-03-04 20:46 ` [PART1 RFC v2 10/10] svm: Manage vcpu load/unload " Suravee Suthikulpanit
2016-03-09 21:46 ` Radim Krčmář
2016-03-10 14:01 ` Radim Krčmář
2016-03-14 11:58 ` Suravee Suthikulpanit
2016-03-14 16:54 ` Radim Krčmář
2016-03-14 11:48 ` Suravee Suthikulpanit
2016-03-14 16:40 ` Radim Krčmář
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56DDA50B.7090609@redhat.com \
--to=pbonzini@redhat.com \
--cc=Suravee.Suthikulpanit@amd.com \
--cc=alex.williamson@redhat.com \
--cc=bp@alien8.de \
--cc=gleb@kernel.org \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rkrcmar@redhat.com \
--cc=sherry.hurwitz@amd.com \
--cc=wei@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).