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

[Joe Jin <joe.jin@xxxxxxxxxx>]

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 )
+                vlapic_sync_pir_to_irr(d->vcpu[pirq_dpci->gmsi.dest_vcpu_id]);
         }
         /* Calculate dest_vcpu_id for MSI-type pirq migration. */
         dest = MASK_EXTR(pirq_dpci->gmsi.gflags,


Thanks,
Joe

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