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 06:22, <chao.gao@xxxxxxxxx> wrote:
> On Mon, Mar 20, 2017 at 04:26:10AM -0600, Jan Beulich wrote:
>>>>> 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.
> 
> @via_pi is also consumed during vCPU migration.

But the event lock isn't being held around the checking of the
field, so putting the setting of the field under lock is of no use.

> I just think the event_lock protects R/W operations on struct hvm_pirq_dpci.
> To prohibit potential race(we can't use VT-d PI in 1st binding, but we can use
> in the 2nd binding. But somehow the first update to via_pi overrides the 
> second one),
> and don't complicate the fields event_lock protects,
> I'm inclined to put it in event_lock-ed region as long as it doesn't 
> introduce other issues.

I certainly don't object to properly synchronizing things here,
but then both producing and consuming side need to hold
respective locks. Otherwise the best you can hope for is to
reduce timing windows; you won't eliminate them though.

Jan


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