Re: [PART1 RFC 5/9] svm: Add VMEXIT handlers for AVIC

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

Hi Paolo,

On 2/12/16 22:38, Paolo Bonzini wrote:
>
>
> On 12/02/2016 14:59, Suravee Suthikulpanit wrote:
>> +		 "icrh:icrl=%#010x:%08x, id=%u, index=%u\n",
>> +		 __func__, svm->vcpu.cpu, svm->vcpu.vcpu_id,
>> +		 icrh, icrl, id, index);
>> +
>> +	switch (id) {
>> +	case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
>> +		/*
>> +		 * AVIC hardware handles the generation 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.
>> +		 */
>> +		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
>> +		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
>> +		break;
>> +	case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
>> +		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
>> +		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
>
> Wouldn't this cause a double injection of the IPI if the following happens:
>
> 1) destination 1 is running, so the processor sets IRR and sends a
> doorbell message
>
> 2) destination 2 is not running, so the processor sets IRR and exits
>
> 3) destination 1 processes the interrupt, moving it from IRR to ISR
>
> 4) destination 1 sends an EOI
>
> 5) the source exits and reinjects the interrupt
>
> 6) destination 1 then receives the interrupt again.

Not sure if I am following your scenario here.  IIUC, your concern is 
regarding the dest2 that was not running at the time that the IPI 
message is sent to both dest1 and dest2?

In this case, since the HW cannot deliver due to one ore more target 
vcpus due to not running, I believe it would not set the IRR bit of 
dest1, and generate the AVIC_INCOMPLETE_IPI #vmexit above instead. I 
don't think it would progress to step 3 right away.

>
>
> Or alternatively:
>
> 1) destination 1 is not running, so the processor sets IRR and exits
>
> 2) another CPU executes VMRUN for destination 1, so the processor
> injects the interrupt
>
> 3) destination 1 sends an EOI
>
> 4) the source exits and reinjects the interrupt
>
> 5) destination 1 then receives the interrupt again.

Same here, I don't think the HW would set the IRR of the dest1. Instead, 
it would generate the #VMEXIT.

> The handling of races for IsRunning and incomplete IPIs has always been
> very confusing to me whenever I read the AVIC specification.  It would
> be great if you could clarify this.

I'll make sure to confirm with the HW designer again just to be sure.

Thanks,
Suravee

> Paolo
>
>> +		break;
>> +	case AVIC_INCMP_IPI_ERR_INV_TARGET:
>> +		pr_err("SVM: %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("SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
>> +		       __func__, icrh, icrl, index);
>> +		BUG();