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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Sep 18, 2019 at 02:16:13PM -0700, 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?
>
>diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
>index 4290c7c710..90c3da441d 100644
>--- a/xen/drivers/passthrough/io.c
>+++ b/xen/drivers/passthrough/io.c
>@@ -412,6 +412,10 @@ int pt_irq_create_bind(
>                 pirq_dpci->gmsi.gvec = pt_irq_bind->u.msi.gvec;
>                 pirq_dpci->gmsi.gflags = gflags;
>             }
>+
>+            /* Notify guest of pending interrupts if necessary */
>+            if ( dest_vcpu_id >= 0 && iommu_intpost && pirq_dpci->gmsi.posted 
>)

Hi Joe,

Do you enable vt-d posted interrupt in Xen boot options? I don't see
why it is specific to vt-d posted interrupt. If only CPU side posted
interrupt is enabled, it is also possible that interrupts are not
propagated from PIR to IRR in time.

Thanks
Chao

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