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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 27, 2019 at 03:09:51AM +0000, Tian, Kevin wrote:
> > From: Jan Beulich <jbeulich@xxxxxxxx>
> > Sent: Wednesday, November 27, 2019 12:59 AM
> > 
> > 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.
> 
> I think the outstanding-notification is one-off checked for triggering 
> interrupt posting process. Once the process starts, there is no need to 
> look at it again. The step 3 of posting process in 29.6 clearly says:
> 
> "The processor clears the outstanding-notification bit in the posted-
> interrupt descriptor. This is done atomically so as to leave the remainder 
> of the descriptor unmodified (e.g., with a locked AND operation)."

Yes, my question would be what happens if the outstanding-notification
bit is 0, does the processor jump to step 6 then?

Does it just ignore the value of the outstanding-notification bit and
continue to step 4?

> But regardless of the hardware behavior, I think it's safe to restrict
> sync_pir_to_irr as this patch does.
> 
> > 
> > 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.
> > 
> 
> There is one paragraph in 29.6:
> 
> "Use of the posted-interrupt descriptor differs from that of other data 
> structures that are referenced by pointers in a VMCS. There is a general 
> requirement that software ensure that each such data structure is 
> modified only when no logical processor with a current VMCS that 
> references it is in VMX non-root operation. That requirement does
> not apply to the posted-interrupt descriptor. There is a requirement, 
> however, that such modifications be done using locked read-modify-write 
> instructions."
> 
> virtual-APIC page is pointer-referenced by VMCS, thus it falls into above
> general requirement. But I suppose there should be some exception with
> this page too, otherwise the point of posted interrupt is killed (if we have
> to kick the dest vcpu into root to update the vIRR). Let me confirm
> internally.

Ack, thanks. I think we can can hold off this improvement/restriction
until we get confirmation of the intended software behavior here.

Thanks, Roger.

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