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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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