Re: [Xen-devel] [PATCH for-4.13 v3 1/2] x86/vmx: add ASSERT to prevent syncing PIR to IRR...

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2019 17:47, Roger Pau Monné  wrote:
> On Tue, Nov 26, 2019 at 05:32:04PM +0100, Jan Beulich wrote:
>> On 26.11.2019 14:26, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -2054,6 +2054,19 @@ static void vmx_sync_pir_to_irr(struct vcpu *v)
>>>      unsigned int group, i;
>>>      DECLARE_BITMAP(pending_intr, NR_VECTORS);
>>>  
>>> +    if ( v != current && !atomic_read(&v->pause_count) )
>>> +    {
>>> +        /*
>>> +         * Syncing PIR to IRR must not be done behind the back of the CPU,
>>> +         * since the IRR is controlled by the hardware when the vCPU is
>>> +         * executing. Only allow Xen to do such sync if the vCPU is the 
>>> current
>>> +         * one or if it's paused: that's required in order to sync the 
>>> lapic
>>> +         * state before saving it.
>>> +         */
>>
>> Is this stated this way by the SDM anywhere?
> 
> No, I think the SDM is not very clear on this, there's a paragraph
> about PIR:
> 
> "The logical processor performs a logical-OR of PIR into VIRR and
> clears PIR. No other agent can read or write a PIR bit (or group of
> bits) between the time it is read (to determine what to OR into VIRR)
> and when it is cleared."

Well, this is about PIR, but my question was rather towards the
effects on vIRR.

>> I ask because the
>> comment then really doesn't apply to just this function, but to
>> vlapic_{,test_and_}{set,clear}_vector() more generally. It's
>> not clear to me at all whether the CPU caches (in an incoherent
>> fashion) IRR (and maybe other APIC page elements), rather than
>> honoring the atomic updates these macros do.
> 
> IMO syncing PIR to IRR when the vCPU is running on a different pCPU is
> likely to at least defeat the purpose of posted interrupts:

I agree here.

> when the
> CPU receives the posted interrupt vector it won't see the
> outstanding-notification bit in the posted-interrupt descriptor
> because the sync done from a different pCPU would have cleared it, at
> which point it's not clear to me that the processor will check vIRR
> for pending interrupts. The description in section 29.6
> POSTED-INTERRUPT PROCESSING doesn't explicitly mention whether the
> value of the outstanding-notification bit affects the logic of posted
> interrupt processing.

But overall this again is all posted interrupt centric when my
question was about vIRR, in particular whether the asserting you
add may need to be even more rigid.

Anyway, let's see what the VMX maintainers have to say.

Jan

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