Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2019 12:56, Roger Pau Monné  wrote:
> On Wed, Nov 27, 2019 at 12:30:06PM +0100, Jan Beulich wrote:
>> On 27.11.2019 12:22, Roger Pau Monné  wrote:
>>> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
>>>> On 26.11.2019 14:26, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/hvm/irq.c
>>>>> +++ b/xen/arch/x86/hvm/irq.c
>>>>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t 
>>>>> via)
>>>>>  struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
>>>>>  {
>>>>>      struct hvm_domain *plat = &v->domain->arch.hvm;
>>>>> -    int vector;
>>>>> +    /*
>>>>> +     * Always call vlapic_has_pending_irq so that PIR is synced into IRR 
>>>>> when
>>>>> +     * using posted interrupts.
>>>>> +     */
>>>>> +    int vector = vlapic_has_pending_irq(v);
>>>>
>>>> Did you consider doing this conditionally either here ...
>>>>
>>>>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct 
>>>>> vcpu *v)
>>>>>      if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
>>>>>          return hvm_intack_pic(0);
>>>>>  
>>>>> -    vector = vlapic_has_pending_irq(v);
>>>>>      if ( vector != -1 )
>>>>>          return hvm_intack_lapic(vector);
>>>>
>>>> ... or here?
>>>
>>> I'm afraid I don't follow. The whole point of this change is to ensure
>>> vlapic_has_pending_irq is unconditionally called in
>>> hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
>>> conditionally...".
>>
>> Do it early when using interrupt posting, and keep it in its
>> current place otherwise.
>>
>>>> I ask not only because the function isn't exactly
>>>> cheap to call (as iirc you did also mention during the v2
>>>> discussion), but also because of its interaction with Viridian
>>>> and nested mode. In case of problems there, avoiding the use
>>>> of interrupt posting would be a workaround in such cases then.
>>>
>>> Would you like me to export something like vlapic_sync_pir_to_irr and
>>> call it unconditionally instead of calling vlapic_has_pending_irq?
>>
>> This looks to be another option, yes. Albeit instead of making
>> non-static (which I assume is what you mean by "export"), maybe
>> simply make this a static inline in vlapic.h then.
> 
> Yes, that would work and IMO is better than moving the call to
> vlapic_has_pending_irq around. Are you OK with this approach?

I think so, yes.

Jan

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