Re: [Xen-devel] [PATCH RFC] pass-through: sync pir to irr after msix vector been updated

[Joe Jin <joe.jin@xxxxxxxxxx>]

On 9/19/19 3:24 AM, Jan Beulich wrote:
> On 18.09.2019 23:16, Joe Jin wrote:
>> On 9/16/19 11:48 PM, Jan Beulich wrote:
>>> On 17.09.2019 00:20, Joe Jin wrote:
>>>> On 9/16/19 1:01 AM, Jan Beulich wrote:
>>>>> On 13.09.2019 18:38, Joe Jin wrote:
>>>>>> On 9/13/19 12:14 AM, Jan Beulich wrote:
>>>>>>> On 12.09.2019 20:03, Joe Jin wrote:
>>>>>>>> --- a/xen/drivers/passthrough/io.c
>>>>>>>> +++ b/xen/drivers/passthrough/io.c
>>>>>>>> @@ -412,6 +412,9 @@ int pt_irq_create_bind(
>>>>>>>>                  pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
>>>>>>>>                  pirq_dpci->gmsi.gflags = gflags;
>>>>>>>>              }
>>>>>>>> +
>>>>>>>> +            if ( hvm_funcs.sync_pir_to_irr )
>>>>>>>> +                
>>>>>>>> hvm_funcs.sync_pir_to_irr(d->vcpu[pirq_dpci->gmsi.dest_vcpu_id]);
>>>>>>>
>>>>>>> If the need for this change can be properly explained, then it
>>>>>>> still wants converting to alternative_vcall() - the the other
>>>>>>> caller of this hook. Or perhaps even better move vlapic.c's
>>>>>>> wrapper (suitably renamed) into hvm.h, and use it here.
>>>>>>
>>>>>> Yes I agree, I'm not 100% sure, so I set it to RFC.
>>>>>
>>>>> And btw, please also attach a brief comment here, to clarify
>>>>> why the syncing is needed precisely at this point.
>>>>>
>>>>>>> Additionally, the code setting pirq_dpci->gmsi.dest_vcpu_id
>>>>>>> (right after your code insertion) allows for the field to be
>>>>>>> invalid, which I think you need to guard against.
>>>>>>
>>>>>> I think you means multiple destination, then it's -1?
>>>>>
>>>>> The reason for why it might be -1 are irrelevant here, I think.
>>>>> You need to handle the case both to avoid an out-of-bounds
>>>>> array access and to make sure an IRR bit wouldn't still get
>>>>> propagated too late in some special case.
>>>>
>>>> Add following checks?
>>>>             if ( dest_vcpu_id >= 0 && dest_vcpu_id < d->max_vcpus &&
>>>>                  d->vcpu[dest_vcpu_id]->runstate.state <= RUNSTATE_blocked 
>>>> )
>>>
>>> Just the >= part should suffice; without an explanation I don't
>>> see why you want the runstate check (which after all is racy
>>> anyway afaict).
>>>
>>>>> Also - what about the respective other path in the function,
>>>>> dealing with PT_IRQ_TYPE_PCI and PT_IRQ_TYPE_MSI_TRANSLATE? It
>>>>> seems to me that there's the same chance of deferring IRR
>>>>> propagation for too long?
>>>>
>>>> This is possible, can you please help on how to get which vcpu associate 
>>>> the IRQ?
>>>> I did not found any helper on current Xen.
>>>
>>> There's no such helper, I'm afraid. Looking at hvm_migrate_pirq()
>>> and hvm_girq_dest_2_vcpu_id() I notice that the former does nothing
>>> if pirq_dpci->gmsi.posted is set. Hence pirq_dpci->gmsi.dest_vcpu_id
>>> isn't really used in this case (please double check), and so you may
>>> want to update the field alongside setting pirq_dpci->gmsi.posted in
>>> pt_irq_create_bind(), covering the multi destination case.
>>>
>>> Your code addition still visible in context above may then want to
>>> be further conditionalized upon iommu_intpost or (perhaps better)
>>> pirq_dpci->gmsi.posted being set.
>>>
>>
>> Sorry this is new to me, and I have to study from code.
>> Do you think below check cover all conditions?
> 
> I does afaict; I don't think you need to check both iommu_intpost and
> pirq_dpci->gmsi.posted - just the latter ought to be enough. What's
> still missing is the further updating of pirq_dpci->gmsi.dest_vcpu_id
> (as explained before, still visible in context above).
> 

 422
 423         dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
 424         pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;

dest_vcpu_id updated later by above code, do I missed something?

Thanks,
Joe

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel