On 07.05.18 at 23:07, <Janakarajan.Natarajan@xxxxxxx> wrote:
@@ -180,6 +185,293 @@ int svm_avic_init_vmcb(struct vcpu *v)
}
/*
+ * Note:
+ * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
+ * The hardware generates this fault when an IPI could not be delivered
+ * to all targeted guest virtual processors because at least one guest
+ * virtual processor was not allocated to a physical core at the time.
+ */
+void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
+{
+ struct vcpu *curr = current;
+ struct domain *currd = curr->domain;
+ struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+ u32 icrh = vmcb->exitinfo1 >> 32;
+ u32 icrl = vmcb->exitinfo1;
+ u32 id = vmcb->exitinfo2 >> 32;
+ u32 index = vmcb->exitinfo2 && 0xFF;
+
+ switch ( id )
+ {
+ case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
+ /*
+ * AVIC hardware handles the delivery of
+ * IPIs when the specified Message Type is Fixed
+ * (also known as fixed delivery mode) and
+ * the Trigger Mode is edge-triggered. The hardware
+ * also supports self and broadcast delivery modes
+ * specified via the Destination Shorthand(DSH)
+ * field of the ICRL. Logical and physical APIC ID
+ * formats are supported. All other IPI types cause
+ * a #VMEXIT, which needs to emulated.
Please utilize the permitted line length (also elsewhere).
+ */
+ vlapic_reg_write(curr, APIC_ICR2, icrh);
+ vlapic_reg_write(curr, APIC_ICR, icrl);
+ break;
+
+ case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
+ {
+ /*
+ * At this point, we expect that the AVIC HW has already
+ * set the appropriate IRR bits on the valid target
+ * vcpus. So, we just need to kick the appropriate vcpu.
+ */
+ struct vcpu *v;
+ uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
+ uint32_t short_hand = icrl & APIC_SHORT_MASK;
+ bool dest_mode = icrl & APIC_DEST_MASK;
+
+ for_each_vcpu ( currd, v )
+ {
+ if ( v != curr &&
+ vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
+ short_hand, dest, dest_mode) )
+ {
+ vcpu_kick(v);
+ break;
Why do you break out of the loop here? With a shorthand more than
one vCPU might be the target.
+ }
+ }
+ break;
+ }
+
+ case AVIC_INCMP_IPI_ERR_INV_TARGET:
+ gprintk(XENLOG_ERR,
+ "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
%#08x produces something like 0x012345, rather than a full eight digits.
Preferably drop the #, or if you really think it's needed replace it be an
explicit 0x.
+ __func__, icrh, icrl, index);
Please use __func__ only when a log message really can't be disambiguated
another way.
For both of these - same further down.
+static avic_logical_id_entry_t *
+avic_get_logical_id_entry(struct svm_domain *d, u32 ldr, bool flat)
+{
+ unsigned int index;
+ unsigned int dest_id = GET_xAPIC_LOGICAL_ID(ldr);
+
+ if ( !dest_id )
+ return NULL;
+
+ if ( flat )
+ {
+ index = ffs(dest_id) - 1;
+ if ( index > 7 )
+ return NULL;
+ }
+ else
+ {
+ unsigned int cluster = (dest_id & 0xf0) >> 4;
+ int apic = ffs(dest_id & 0x0f) - 1;
+
+ if ( (apic < 0) || (apic > 7) || (cluster >= 0xf) )
I can't see a way for apic to be larger than 3 with the calculation above.
+ return NULL;
+ index = (cluster << 2) + apic;
+ }
+
+ ASSERT(index <= 255);
Which of the many possible meanings of 255 is this?
+static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
+{
+ avic_logical_id_entry_t *entry, new_entry;
+ u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);
Just to give another example - looks like this too could be vlapic_get_reg().
+static int avic_handle_ldr_update(struct vcpu *v)
+{
+ int ret = 0;
Pointless initializer.
+ u32 ldr = vlapic_reg_read(vcpu_vlapic(v), APIC_LDR);
+ u32 apic_id = vlapic_reg_read(vcpu_vlapic(v), APIC_ID);
+
+ if ( !ldr )
+ return -EINVAL;
+
+ ret = avic_ldr_write(v, GET_xAPIC_ID(apic_id), ldr, true);
+ if ( ret && v->arch.hvm_svm.avic_last_ldr )
+ {
+ /*
+ * Note:
+ * In case of failure to update LDR register,
+ * we set the guest physical APIC ID to 0,
+ * and set the entry logical APID ID entry
+ * to invalid (false).
+ */
+ avic_ldr_write(v, 0, v->arch.hvm_svm.avic_last_ldr, false);
+ v->arch.hvm_svm.avic_last_ldr = 0;
+ }
+ else
+ {
+ /*
+ * Note:
+ * This saves the last valid LDR so that we
+ * know which entry in the local APIC ID
+ * to clean up when the LDR is updated.
+ */
+ v->arch.hvm_svm.avic_last_ldr = ldr;
The comment says "last valid", but you may get here also when the
first avic_ldr_write() failed. I think you mean
if ( !ret )
...
else if ( v->arch.hvm_svm.avic_last_ldr )
...
+static int avic_unaccel_trap_write(struct vcpu *v)
+{
+ struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
+ u32 offset = vmcb->exitinfo1 & AVIC_UNACCEL_ACCESS_OFFSET_MASK;
+ u32 reg = vlapic_reg_read(vcpu_vlapic(v), offset);
+
+ switch ( offset )
+ {
+ case APIC_ID:
+ /*
+ * Currently, we do not support APIC_ID update while
+ * the vcpus are running, which might require updating
+ * AVIC max APIC ID in all VMCBs. This would require
+ * synchronize update on all running VCPUs.
+ */
+ return X86EMUL_UNHANDLEABLE;
+
+ case APIC_LDR:
+ if ( avic_handle_ldr_update(v) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ case APIC_DFR:
+ if ( avic_handle_dfr_update(v) )
+ return X86EMUL_UNHANDLEABLE;
+ break;
+
+ default:
+ break;
This default case is unnecessary.
+void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
+{
+ struct vcpu *curr = current;
+ struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
+ u32 offset = vmcb->exitinfo1 & 0xFF0;
+ u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;
bool?
+ if ( avic_is_trap(offset) )
+ {
+ /* Handling AVIC Trap (intercept right after the access). */
+ if ( !rw )
+ {
+ /*
+ * If a read trap happens, the CPU microcode does not
+ * implement the spec.
+ */
+ gprintk(XENLOG_ERR, "%s: Invalid #VMEXIT due to trap read (%#x)\n",
+ __func__, offset);
+ domain_crash(curr->domain);
+ }
+
+ if ( avic_unaccel_trap_write(curr) != X86EMUL_OKAY )
ITYM "else if" here.
+ {
+ gprintk(XENLOG_ERR, "%s: Failed to handle trap write (%#x)\n",
+ __func__, offset);
+ domain_crash(curr->domain);
+ }
+ }
+ else
+ /* Handling AVIC Fault (intercept before the access). */
+ hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
+ X86_EVENT_NO_EC);
What's the rationale behind having chosen this function? I don't think it is
supposed to be called from outside the VM event code.
