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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.17 at 03:38, <chao.gao@xxxxxxxxx> wrote:
> On Mon, Mar 20, 2017 at 03:18:18AM -0600, Jan Beulich wrote:
>>>>> On 20.03.17 at 02:59, <chao.gao@xxxxxxxxx> wrote:
>>> On Fri, Mar 17, 2017 at 04:43:08AM -0600, Jan Beulich wrote:
>>>>>>> On 15.03.17 at 06:11, <chao.gao@xxxxxxxxx> wrote:
>>>>> +        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?
>>
>>Oops, indeed.
>>
>>> I think it is safe
>>> for the functions called by pi_find_dest_vcpu() are almost same with
>>> hvm_girq_dest_2_vcpu_id.
>>
>>The question then needs to be put differently: Is this needed?
>>You shouldn't move into a locked region what doesn't need to
>>be there.
> 
> Yes. For we rely on the result to set @via_pi which needs to be 
> protected by the lock.

I don't follow: You set via_pi for hvm_migrate_pirqs() to consume,
yet the call to that function sits ...

>>>>> +        }
>>>>>          spin_unlock(&d->event_lock);
>>>>>          if ( dest_vcpu_id >= 0 )
>>>>>              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);

... right after the lock release.

>>>>(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);
>>
>>Yes, along these lines, albeit I think the first if() is more complicated
>>than it needs to be.
> 
> We can make it simple like this:
> 
> const struct *vcpu vcpu;
> ...
> 
> vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;

Ouch - there were three if()s, and I missed the first one, i.e. I
really meant the middle of them.

Jan


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