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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan

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