Re: [PATCH 1/4] x86/vMSI-X: defer intercept handler registration

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 1/4] x86/vMSI-X: defer intercept handler registration
Date: Fri, 17 Jun 2016 12:13:58 -0400	[thread overview]
Message-ID: <20160617161358.GM1340@char.us.oracle.com> (raw)
In-Reply-To: <5758314A02000078000F30A2@prv-mh.provo.novell.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel