Re: [PATCH v2 08/10] x86/SVM: Add interrupt management code via AVIC

[George Dunlap <george.dunlap@citrix.com>]

On 02/01/17 17:45, Andrew Cooper wrote:
> On 31/12/2016 05:45, Suravee Suthikulpanit wrote:
>> Enabling AVIC implicitly disables the V_IRQ, V_INTR_PRIO, V_IGN_TPR,
>> and V_INTR_VECTOR fields in the VMCB Control Word. Therefore, this patch
>> introduces new interrupt injection code via AVIC backing page.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  xen/arch/x86/hvm/svm/avic.c        | 28 ++++++++++++++++++++++++++++
>>  xen/arch/x86/hvm/svm/intr.c        |  4 ++++
>>  xen/arch/x86/hvm/svm/svm.c         | 12 ++++++++++--
>>  xen/include/asm-x86/hvm/svm/avic.h |  1 +
>>  4 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
>> index 6351c8e..faa5e45 100644
>> --- a/xen/arch/x86/hvm/svm/avic.c
>> +++ b/xen/arch/x86/hvm/svm/avic.c
>> @@ -636,6 +636,34 @@ void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
>>      return;
>>  }
>>  
>> +void svm_avic_deliver_posted_intr(struct vcpu *v, u8 vec)
>> +{
>> +    struct vlapic *vlapic = vcpu_vlapic(v);
>> +
>> +    /* Fallback to use non-AVIC if vcpu is not enabled with AVIC. */
>> +    if ( !svm_avic_vcpu_enabled(v) )
>> +    {
>> +        if ( !vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
>> +            vcpu_kick(v);
>> +        return;
>> +    }
>> +
>> +    if ( !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
>> +        return;
> 
> Won't this discard the interrupt?
> 
>> +
>> +    if ( vlapic_test_and_set_vector(vec, &vlapic->regs->data[APIC_IRR]) )
>> +        return;
>> +
>> +    /*
>> +     * If vcpu is running on another cpu, hit the doorbell to signal
>> +     * it to process interrupt. Otherwise, kick it.
>> +     */
>> +    if ( v->is_running && (v != current) )
>> +        wrmsrl(AVIC_DOORBELL, cpu_data[v->processor].apicid);
> 
> Hmm - my gut feeling is that this is racy without holding the scheduler
> lock for the target pcpu.  Nothing (I am aware of) excludes ->is_running
> and ->processor changing behind our back.
> 
> CC'ing George and Dario for their input.

I'm not sure how AVIC_DOORBELL works (haven't looked at the whole
series) -- the vcpu_kick() path accesses both is_running and
v->processor without locks; but that's because any schedule event which
may change those values will also check to see whether there is a
pending event to be delivered.  In theory the same could apply to this
mechanism, but it would take some careful thinking (in particular,
understanding the "NB's" in vcpu_kick() to see if and how they apply).

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel