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

[Joe Jin <joe.jin@xxxxxxxxxx>]

On 9/23/19 1:31 AM, Chao Gao wrote:
> 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.

Hi Chao,

Yes vt-d posted interrupt been enabled on booting:

(XEN) VMX: Supported advanced features:
(XEN)  - APIC MMIO access virtualisation
(XEN)  - APIC TPR shadow
(XEN)  - Extended Page Tables (EPT)
(XEN)  - Virtual-Processor Identifiers (VPID)
(XEN)  - Virtual NMI
(XEN)  - MSR direct-access bitmap
(XEN)  - Unrestricted Guest
(XEN)  - APIC Register Virtualization
(XEN)  - Virtual Interrupt Delivery
(XEN)  - Posted Interrupt Processing
(XEN)  - VMCS shadowing

Look at vlapic_set_irq(), and seems if posted interrupt been enabled, it set PIR
but not IRR?

 170     if ( hvm_funcs.deliver_posted_intr )
 171         alternative_vcall(hvm_funcs.deliver_posted_intr, target, vec);
 172     else if ( !vlapic_test_and_set_irr(vec, vlapic) )
 173         vcpu_kick(target);

Thanks,
Joe

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