| Message ID | 5758314A02000078000F30A2@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 06:52:58AM -0600, Jan Beulich wrote: > There's no point in registering the internal MSI-X table intercept > functions on all domains - it is sufficient to do so once a domain gets > an MSI-X capable device assigned. I think this will break on SR-IOV devices that are created and we had not setup MCFG. There is this Intel board (which I have) where the MCFG is setup only via ACPI and in the past we had issues - where we never detect that the VF had MSI-X - b/c we could not access the configuration registers past 0xff. I am pretty sure that Linux now uploads the MCFG data during bootup from the ACPI AML code, but earlier kernels may not. And that would mean the device would try to use MSI-X while code would be !pdev->msix. Hmm, thought I wonder - with all those improvements of capturing the MSI-X and validating it in the hypervisor - would this even matter? Let me stash an 82576 card in that box and see what happens with Xen 4.7. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -644,8 +644,6 @@ int hvm_domain_initialise(struct domain > > rtc_init(d); > > - msixtbl_init(d); > - > register_portio_handler(d, 0xe9, 1, hvm_print_line); > > if ( hvm_tsc_scaling_supported ) > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -553,7 +553,8 @@ found: > > void msixtbl_init(struct domain *d) > { > - if ( !has_vlapic(d) ) > + if ( !has_hvm_container_domain(d) || !has_vlapic(d) || > + d->arch.hvm_domain.msixtbl_list.next ) > return; > > INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list); > @@ -567,7 +568,7 @@ void msixtbl_pt_cleanup(struct domain *d > struct msixtbl_entry *entry, *temp; > unsigned long flags; > > - if ( !has_vlapic(d) ) > + if ( !d->arch.hvm_domain.msixtbl_list.next ) > return; > > /* msixtbl_list_lock must be acquired with irq_disabled for check_lock() */ > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1380,6 +1380,9 @@ static int assign_device(struct domain * > goto done; > } > > + if ( pdev->msix ) > + msixtbl_init(d); > + > pdev->fault.count = 0; > > if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag)) ) > > > > x86/vMSI-X: defer intercept handler registration > > There's no point in registering the internal MSI-X table intercept > functions on all domains - it is sufficient to do so once a domain gets > an MSI-X capable device assigned. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -644,8 +644,6 @@ int hvm_domain_initialise(struct domain > > rtc_init(d); > > - msixtbl_init(d); > - > register_portio_handler(d, 0xe9, 1, hvm_print_line); > > if ( hvm_tsc_scaling_supported ) > --- a/xen/arch/x86/hvm/vmsi.c > +++ b/xen/arch/x86/hvm/vmsi.c > @@ -553,7 +553,8 @@ found: > > void msixtbl_init(struct domain *d) > { > - if ( !has_vlapic(d) ) > + if ( !has_hvm_container_domain(d) || !has_vlapic(d) || > + d->arch.hvm_domain.msixtbl_list.next ) > return; > > INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list); > @@ -567,7 +568,7 @@ void msixtbl_pt_cleanup(struct domain *d > struct msixtbl_entry *entry, *temp; > unsigned long flags; > > - if ( !has_vlapic(d) ) > + if ( !d->arch.hvm_domain.msixtbl_list.next ) > return; > > /* msixtbl_list_lock must be acquired with irq_disabled for check_lock() */ > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1380,6 +1380,9 @@ static int assign_device(struct domain * > goto done; > } > > + if ( pdev->msix ) > + msixtbl_init(d); > + > pdev->fault.count = 0; > > if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag)) ) > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 17.06.16 at 18:13, <konrad.wilk@oracle.com> wrote: > On Wed, Jun 08, 2016 at 06:52:58AM -0600, Jan Beulich wrote: >> There's no point in registering the internal MSI-X table intercept >> functions on all domains - it is sufficient to do so once a domain gets >> an MSI-X capable device assigned. > > I think this will break on SR-IOV devices that are created and we > had not setup MCFG. How does MCFG get into the picture here? (And anyway, this series is about vMSI-X, not host MSI-X.) > There is this Intel board (which I have) where the MCFG is setup > only via ACPI and in the past we had issues - where we never > detect that the VF had MSI-X - b/c we could not access the > configuration registers past 0xff. The MSI-X capability necessarily lives in the low 256 bytes. Do you mean the SR-IOV capability? > I am pretty sure that Linux now uploads the MCFG data during > bootup from the ACPI AML code, but earlier kernels may not. > > And that would mean the device would try to use MSI-X while > code would be !pdev->msix. > > Hmm, thought I wonder - with all those improvements of capturing > the MSI-X and validating it in the hypervisor - would this > even matter? > > Let me stash an 82576 card in that box and see what happens with > Xen 4.7. Thanks. Jan
On 08/06/16 13:52, Jan Beulich wrote: > There's no point in registering the internal MSI-X table intercept > functions on all domains - it is sufficient to do so once a domain gets > an MSI-X capable device assigned. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -644,8 +644,6 @@ int hvm_domain_initialise(struct domain rtc_init(d); - msixtbl_init(d); - register_portio_handler(d, 0xe9, 1, hvm_print_line); if ( hvm_tsc_scaling_supported ) --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -553,7 +553,8 @@ found: void msixtbl_init(struct domain *d) { - if ( !has_vlapic(d) ) + if ( !has_hvm_container_domain(d) || !has_vlapic(d) || + d->arch.hvm_domain.msixtbl_list.next ) return; INIT_LIST_HEAD(&d->arch.hvm_domain.msixtbl_list); @@ -567,7 +568,7 @@ void msixtbl_pt_cleanup(struct domain *d struct msixtbl_entry *entry, *temp; unsigned long flags; - if ( !has_vlapic(d) ) + if ( !d->arch.hvm_domain.msixtbl_list.next ) return; /* msixtbl_list_lock must be acquired with irq_disabled for check_lock() */ --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1380,6 +1380,9 @@ static int assign_device(struct domain * goto done; } + if ( pdev->msix ) + msixtbl_init(d); + pdev->fault.count = 0; if ( (rc = hd->platform_ops->assign_device(d, devfn, pci_to_dev(pdev), flag)) )