Re: [PATCH v10 5/6] passthrough/io: don't migrate pirq when it is delivered through VT-d PI

[Chao Gao <chao.gao@intel.com>]

On Fri, Mar 17, 2017 at 04:43:08AM -0600, Jan Beulich wrote:
>>>> On 15.03.17 at 06:11, <chao.gao@intel.com> wrote:
>> @@ -441,6 +442,15 @@ int pt_irq_create_bind(
>>  
>>          dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>>          pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>> +
>> +        pirq_dpci->gmsi.via_pi = 0;
>
>false
>
>> +        if ( iommu_intpost )
>> +        {
>> +            vcpu = pi_find_dest_vcpu(d, dest, dest_mode, delivery_mode,
>> +                                     pirq_dpci->gmsi.gvec);
>
>This is now outside of the event_lock-ed region - is this safe?

do you mean it is __inside__ the event_lock-ed region? I think it is safe
for the functions called by pi_find_dest_vcpu() are almost same with
hvm_girq_dest_2_vcpu_id.

>
>Furthermore, looking at this, why do we need to call this function
>at all in other than the LowestPrio case? It looks as if we could
>easily use what hvm_girq_dest_2_vcpu_id() provides in the Fixed
>case.

Agree. we can delete pi_find_dest_vcpu() and for LowestPrio case 
when VT-d PI enabled, we call vector_hashing_dest() directly.

>
>> +            if ( vcpu )
>> +                pirq_dpci->gmsi.via_pi = 1;
>
>true
>
>> +        }
>>          spin_unlock(&d->event_lock);
>>          if ( dest_vcpu_id >= 0 )
>>              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
>
>(continuing from above) This could then use vcpu too.

I don't understand. In this patch, vcpu is always null when VT-d PI is not
enabled. Do you mean something like below: 

if ( dest_vcpu_id >= 0 )
    vcpu = d->vcpu[dest_vcpu_id];
if ( iommu_intpost && (!vcpu) && (delivery_mode == dest_LowestPrio) )
{
    vcpu = vector_hashing_dest(d, dest, dest_mode,pirq_dpci->gmsi.gvec);
...
}
spin_unlock(&d->event_lock);
if ( vcpu )
    hvm_migrate_pirqs(vcpu);

Thank,
Chao

>
>> --- a/xen/include/xen/hvm/irq.h
>> +++ b/xen/include/xen/hvm/irq.h
>> @@ -63,6 +63,7 @@ struct hvm_gmsi_info {
>>      uint32_t gvec;
>>      uint32_t gflags;
>>      int dest_vcpu_id; /* -1 :multi-dest, non-negative: dest_vcpu_id */
>> +    bool via_pi; /* directly deliver to guest via VT-d PI? */
>
>Wouldn't the field name perhaps better be (or include) "posted"?
>
>Jan
>

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