Re: [Xen-devel] [PATCH v2] x86/passthrough: fix migration of MSI when using posted interrupts

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monné [mailto:roger.pau@xxxxxxxxxx]
> Sent: Friday, November 8, 2019 6:20 PM
> To: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Cc: Joe Jin <joe.jin@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>;
> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Juergen Gross <jgross@xxxxxxxx>; Wei Liu
> <wl@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2] x86/passthrough: fix migration of MSI
> when using posted interrupts
> 
> On Fri, Nov 08, 2019 at 02:25:05AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monné [mailto:roger.pau@xxxxxxxxxx]
> > > Sent: Monday, November 4, 2019 5:47 PM
> > >
> > > On Sat, Nov 02, 2019 at 07:48:06AM +0000, Tian, Kevin wrote:
> > > > > From: Roger Pau Monné [mailto:roger.pau@xxxxxxxxxx]
> > > > > Sent: Thursday, October 31, 2019 11:23 PM
> > > > >
> > > > > On Thu, Oct 31, 2019 at 07:52:23AM -0700, Joe Jin wrote:
> > > > > > On 10/31/19 1:01 AM, Jan Beulich wrote:
> > > > > > > On 30.10.2019 19:01, Joe Jin wrote:
> > > > > > >> On 10/30/19 10:23 AM, Roger Pau Monné wrote:
> > > > > > >>> On Wed, Oct 30, 2019 at 09:38:16AM -0700, Joe Jin wrote:
> > > > > > >>>> On 10/30/19 1:24 AM, Roger Pau Monné wrote:
> > > > > > >>>>> Can you try to add the following debug patch on top of the
> > > existing
> > > > > > >>>>> one and report the output that you get on the Xen console?
> > > > > > >>>>
> > > > > > >>>> Applied debug patch and run the test again, not of any log
> > > printed,
> > > > > > >>>> attached Xen log on serial console, seems pi_update_irte()
> not
> > > been
> > > > > > >>>> called for iommu_intpost was false.
> > > > > > >>>
> > > > > > >>> I have to admit I'm lost at this point. Does it mean the 
> > > > > > >>> original
> > > > > > >>> issue had nothing to do with posted interrupts?
> > > > > > >>
> > > > > > >> Looks when inject irq by vlapic_set_irq(), it checked by
> > > > > > >> hvm_funcs.deliver_posted_intr rather than iommu_intpost:
> > > > > > >>
> > > > > > >>  176     if ( hvm_funcs.deliver_posted_intr )
> > > > > > >>  177         hvm_funcs.deliver_posted_intr(target, vec);
> > > > > > >>
> > > > > > >> And deliver_posted_intr() would be there, when vmx enabled:
> > > > > > >>
> > > > > > >> (XEN) HVM: VMX enabled
> > > > > > >> (XEN) HVM: Hardware Assisted Paging (HAP) detected
> > > > > > >> (XEN) HVM: HAP page sizes: 4kB, 2MB, 1GB
> > > > > > >
> > > > > > > I can't see the connection. start_vmx() has
> > > > > > >
> > > > > > >     if ( cpu_has_vmx_posted_intr_processing )
> > > > > > >     {
> > > > > > >         alloc_direct_apic_vector(&posted_intr_vector,
> > > > > pi_notification_interrupt);
> > > > > > >         if ( iommu_intpost )
> > > > > > >             alloc_direct_apic_vector(&pi_wakeup_vector,
> > > > > pi_wakeup_interrupt);
> > > > > > >
> > > > > > >         vmx_function_table.deliver_posted_intr =
> > > vmx_deliver_posted_intr;
> > > > > > >         vmx_function_table.sync_pir_to_irr     = 
> > > > > > > vmx_sync_pir_to_irr;
> > > > > > >         vmx_function_table.test_pir            = vmx_test_pir;
> > > > > > >     }
> > > > > > >
> > > > > > > i.e. the hook is present only when posted interrupts are
> > > > > > > available in general. I.e. also with just CPU-side posted
> > > > > > > interrupts, yes, which gets confirmed by your "apicv=0"
> > > > > > > test. Yet with just CPU-side posted interrupts I'm
> > > > > > > struggling again to understand your original problem
> > > > > > > description, and the need to fiddle with IOMMU side code.
> > > > > >
> > > > > > Yes, on my test env, cpu_has_vmx_posted_intr_processing == true
> &&
> > > > > iommu_intpost == false,
> > > > > > with this, posted interrupts been enabled.
> > > > >
> > > > > I'm still quite lost. My reading of the Intel VT-d spec is that the
> > > > > posted interrupt descriptor (which contains the PIRR) is used in
> > > > > conjunction with a posted interrupt remapping entry in the iommu,
> so
> > > > > that interrupts get recorded in the PIRR and later synced by the
> > > > > hypervisor into the vlapic IRR when resuming the virtual CPU.
> > > >
> > > > there are two parts. Intel first implements CPU posted interrupt,
> > > > which allows one CPU to post IPI into non-root context in another
> > > > CPU through posted interrupt descriptor. Later VT-d posted
> > > > interrupt comes, which use interrupt remapping entry and the
> > > > same posted interrupt descriptor (using more fields) to convert
> > > > a device interrupt into a posted interrupt. The posting process is
> > > > same on the dest CPU, regardless of whether it's from another CPU
> > > > or a device.
> > >
> > > Thanks for the description.
> > >
> > > So the problem reported by Jin happens when using CPU posted
> > > interrupts but not VT-d posted interrupts, in which case there
> > > shouldn't be a need to sync PIRR with IRR when interrupts from a
> > > passthrough device are reconfigured, because interrupts from that
> > > device shouldn't end up signaled in PIRR because VT-d posted
> > > interrupts is not being used.
> > >
> > > Do interrupts from passthrough devices end up signaled in the posted
> > > interrupt descriptor PIRR field when not using VT-d posted
> > > interrupts but using CPU posted interrupts?
> >
> > No. If VT-d posted interrupt is disabled, interrupts from passthrough
> > devices don't go through posted interrupt descriptor. But after hypervisor
> > serves the interrupt and when it decides to inject a virtual interrupt into
> > the guest, PIRR will be updated if CPU posted interrupt is enabled.
> 
> Oh, I see. vmx_deliver_posted_intr which is called regardless of
> whether VT-d posted interrupts are enabled or not does set the vector
> in the PIRR, so we do need to sync the PIRR with the IRR even when CPU
> only posted interrupts are used.
> 
> May I ask why this is done that way? When VT-d posted interrupts are
> not used wouldn't it be simpler to just set the vector in the IRR
> directly instead of setting it in the PIRR and later syncing the PIRR
> with IRR?
> 

because PIRR allows direct virtual interrupt posting when the dest
vcpu is in non-root mode (you save physical IPI). It benefits generic 
interrupt virtualization.

Thanks
Kevin

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