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
