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

[Chao Gao <chao.gao@xxxxxxxxx>]

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

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

Yes. the code may be wrong, other than complicated. The code above has changing
the way we choose the destination vcpu when VT-d PI is enabled. Even we
can get a single destination vcpu for LowestPrio, we should use
vector_hashing_dest() to calculate the destination vcpu like the original logic.

I think the code below is better:
if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )

>
>Jan
>

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