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

[Paolo Bonzini]

On 16/02/2016 07:29, Suravee Suthikulpanit wrote:
> 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.

The documentation doesn't say that setting the IRR bit is atomic across
all CPUs (and from a hardware perspective that would be extremely
unlikely).  Another hint in my opinion is that the vmexit is called
"incomplete" IPI, not something like "aborted" IPI.  "abort" might
suggest atomicity, "incomplete" definitely suggests *non*atomicity to me.

Wei, what do you think/recall?

I am afraid that this could be a showstopper for AVIC support in KVM.
The only solution I see is to have a different page for each CPU, so
that only self-IPIs are virtualized.  Then you'd only support
virtualization of self-IPIs, similar to Intel's APICv.

>> 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.

Please do, thanks!

Paolo