xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Fixes to Xen pcifront and pciback (v1)
@ 2016-02-11 21:10 Konrad Rzeszutek Wilk
  2016-02-11 21:10 ` [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY Konrad Rzeszutek Wilk
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-11 21:10 UTC (permalink / raw)
  To: Tommi Airikka, linux-kernel, xen-devel, david.vrabel, boris.ostrovsky

Hey,

These are patches that were developed for the Debian bug
810379 which san Tommi had openned.

The issue around from the two XSA fixes - which introduced
this regression.

I am the person who developed them and my explanation for this
regression oversight is that I tested for the 'exploit use-case'
and also did not have the VF automatic testing for PV guests
working at that point (only for HVM guests). When testing it
with with an PF in PV mode it worked - as the device would
fallback to legacy interrupts so things looked fine (argh)
from the outside (could ping it).

Either way it is embarrassing and I am sorry for the trouble
this caused.

Now the patches:
 [PATCH 1/4] xen/pciback: Check PF instead of VF for

Fixes regression introduced by XSA-157 for VF guests that
use MSI-X.

 [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be

Fixes regression introduced by XSA-155 - where MSI-X vectors
would not be copied back to the guest.

 [PATCH 3/4] xen/pcifront: Report the errors better.

Makes it easier to troubleshoot in the future.
 [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality

This one I think has been in the driver since the first time
the PV driver was introduced.

 arch/x86/include/asm/xen/pci.h        |  4 ++--
 arch/x86/pci/xen.c                    |  5 ++++-
 drivers/pci/xen-pcifront.c            | 11 +++++++----
 drivers/xen/xen-pciback/pciback_ops.c | 12 +++++++++---
 4 files changed, 22 insertions(+), 10 deletions(-)

Konrad Rzeszutek Wilk (4):
      xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY
      xen/pciback: Save the number of MSI-X entries to be copied later.
      xen/pcifront: Report the errors better.
      xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY
  2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
@ 2016-02-11 21:10 ` Konrad Rzeszutek Wilk
  2016-02-12  9:19   ` Jan Beulich
  2016-02-11 21:10 ` [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later Konrad Rzeszutek Wilk
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-11 21:10 UTC (permalink / raw)
  To: Tommi Airikka, linux-kernel, xen-devel, david.vrabel, boris.ostrovsky
  Cc: stable, Konrad Rzeszutek Wilk

c/s 408fb0e5aa7fda0059db282ff58c3b2a4278baa0
"xen/pciback: Don't allow MSI-X ops if PCI_COMMAND_MEMORY is not set."
would check the device for PCI_COMMAND_MEMORY which is great.
Except that VF devices are unique - for example they have no
legacy interrupts, and also any writes to PCI_COMMAND_MEMORY
are silently ignored (by the hardware).

CC: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pciback_ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
index 73dafdc..8c86a53 100644
--- a/drivers/xen/xen-pciback/pciback_ops.c
+++ b/drivers/xen/xen-pciback/pciback_ops.c
@@ -213,6 +213,7 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
 	int i, result;
 	struct msix_entry *entries;
 	u16 cmd;
+	struct pci_dev *phys_dev;
 
 	if (unlikely(verbose_request))
 		printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n",
@@ -227,8 +228,10 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
 	/*
 	 * PCI_COMMAND_MEMORY must be enabled, otherwise we may not be able
 	 * to access the BARs where the MSI-X entries reside.
+	 * But VF devices are unique in which the PF needs to be checked.
 	 */
-	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	phys_dev = pci_physfn(dev);
+	pci_read_config_word(phys_dev, PCI_COMMAND, &cmd);
 	if (dev->msi_enabled || !(cmd & PCI_COMMAND_MEMORY))
 		return -ENXIO;
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later.
  2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
  2016-02-11 21:10 ` [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY Konrad Rzeszutek Wilk
@ 2016-02-11 21:10 ` Konrad Rzeszutek Wilk
  2016-02-12  9:25   ` Jan Beulich
  2016-02-11 21:10 ` [PATCH 3/4] xen/pcifront: Report the errors better Konrad Rzeszutek Wilk
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-11 21:10 UTC (permalink / raw)
  To: Tommi Airikka, linux-kernel, xen-devel, david.vrabel, boris.ostrovsky
  Cc: stable, Konrad Rzeszutek Wilk

c/s  8135cf8b092723dbfcc611fe6fdcb3a36c9951c5
"xen/pciback: Save xen_pci_op commands before processing it"
would copyback the processed values - which was great.

However it missed the case that xen_pcibk_enable_msix - when
completing would overwrite op->value (which had the number
of MSI-X vectors requested) with the return value (which for
success was zero). Hence the copy-back routine (which would use
op->value) would copy exactly zero MSI-X vectors back.

CC: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-pciback/pciback_ops.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
index 8c86a53..2fc5880 100644
--- a/drivers/xen/xen-pciback/pciback_ops.c
+++ b/drivers/xen/xen-pciback/pciback_ops.c
@@ -335,7 +335,9 @@ void xen_pcibk_do_op(struct work_struct *data)
 	struct xen_pcibk_dev_data *dev_data = NULL;
 	struct xen_pci_op *op = &pdev->op;
 	int test_intx = 0;
-
+#ifdef CONFIG_PCI_MSI
+	unsigned int nr = 0;
+#endif
 	*op = pdev->sh_info->op;
 	barrier();
 	dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn);
@@ -363,6 +365,7 @@ void xen_pcibk_do_op(struct work_struct *data)
 			op->err = xen_pcibk_disable_msi(pdev, dev, op);
 			break;
 		case XEN_PCI_OP_enable_msix:
+			nr = op->value;
 			op->err = xen_pcibk_enable_msix(pdev, dev, op);
 			break;
 		case XEN_PCI_OP_disable_msix:
@@ -385,7 +388,7 @@ void xen_pcibk_do_op(struct work_struct *data)
 	if (op->cmd == XEN_PCI_OP_enable_msix && op->err == 0) {
 		unsigned int i;
 
-		for (i = 0; i < op->value; i++)
+		for (i = 0; i < nr; i++)
 			pdev->sh_info->op.msix_entries[i].vector =
 				op->msix_entries[i].vector;
 	}
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] xen/pcifront: Report the errors better.
  2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
  2016-02-11 21:10 ` [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY Konrad Rzeszutek Wilk
  2016-02-11 21:10 ` [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later Konrad Rzeszutek Wilk
@ 2016-02-11 21:10 ` Konrad Rzeszutek Wilk
  2016-02-11 21:10 ` [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted Konrad Rzeszutek Wilk
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-11 21:10 UTC (permalink / raw)
  To: Tommi Airikka, linux-kernel, xen-devel, david.vrabel, boris.ostrovsky
  Cc: Konrad Rzeszutek Wilk

The messages should be different depending on the type of error.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/xen/pci.h | 4 ++--
 arch/x86/pci/xen.c             | 5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
index 968d57d..f320ee3 100644
--- a/arch/x86/include/asm/xen/pci.h
+++ b/arch/x86/include/asm/xen/pci.h
@@ -57,7 +57,7 @@ static inline int xen_pci_frontend_enable_msi(struct pci_dev *dev,
 {
 	if (xen_pci_frontend && xen_pci_frontend->enable_msi)
 		return xen_pci_frontend->enable_msi(dev, vectors);
-	return -ENODEV;
+	return -ENOSYS;
 }
 static inline void xen_pci_frontend_disable_msi(struct pci_dev *dev)
 {
@@ -69,7 +69,7 @@ static inline int xen_pci_frontend_enable_msix(struct pci_dev *dev,
 {
 	if (xen_pci_frontend && xen_pci_frontend->enable_msix)
 		return xen_pci_frontend->enable_msix(dev, vectors, nvec);
-	return -ENODEV;
+	return -ENOSYS;
 }
 static inline void xen_pci_frontend_disable_msix(struct pci_dev *dev)
 {
diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index ff31ab4..beac4df 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -196,7 +196,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 	return 0;
 
 error:
-	dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n");
+	if (ret == -ENOSYS)
+		dev_err(&dev->dev, "Xen PCI frontend has not registered MSI/MSI-X support!\n");
+	else if (ret)
+		dev_err(&dev->dev, "Xen PCI frontend error: %d!\n", ret);
 free:
 	kfree(v);
 	return ret;
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.
  2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
                   ` (2 preceding siblings ...)
  2016-02-11 21:10 ` [PATCH 3/4] xen/pcifront: Report the errors better Konrad Rzeszutek Wilk
@ 2016-02-11 21:10 ` Konrad Rzeszutek Wilk
  2016-02-14  1:23   ` Boris Ostrovsky
                     ` (2 more replies)
  2016-02-15 14:35 ` [PATCH] Fixes to Xen pcifront and pciback (v1) David Vrabel
       [not found] ` <56C1E24C.5040704@citrix.com>
  5 siblings, 3 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-11 21:10 UTC (permalink / raw)
  To: Tommi Airikka, linux-kernel, xen-devel, david.vrabel, boris.ostrovsky
  Cc: stable, Konrad Rzeszutek Wilk

Occasionaly PV guests would crash with:

pciback 0000:00:00.1: Xen PCI mapped GSI0 to IRQ16
BUG: unable to handle kernel paging request at 0000000d1a8c0be0
.. snip..
  <ffffffff8139ce1b>] find_next_bit+0xb/0x10
  [<ffffffff81387f22>] cpumask_next_and+0x22/0x40
  [<ffffffff813c1ef8>] pci_device_probe+0xb8/0x120
  [<ffffffff81529097>] ? driver_sysfs_add+0x77/0xa0
  [<ffffffff815293e4>] driver_probe_device+0x1a4/0x2d0
  [<ffffffff813c1ddd>] ? pci_match_device+0xdd/0x110
  [<ffffffff81529657>] __device_attach_driver+0xa7/0xb0
  [<ffffffff815295b0>] ? __driver_attach+0xa0/0xa0
  [<ffffffff81527622>] bus_for_each_drv+0x62/0x90
  [<ffffffff8152978d>] __device_attach+0xbd/0x110
  [<ffffffff815297fb>] device_attach+0xb/0x10
  [<ffffffff813b75ac>] pci_bus_add_device+0x3c/0x70
  [<ffffffff813b7618>] pci_bus_add_devices+0x38/0x80
  [<ffffffff813dc34e>] pcifront_scan_root+0x13e/0x1a0
  [<ffffffff817a0692>] pcifront_backend_changed+0x262/0x60b
  [<ffffffff814644c6>] ? xenbus_gather+0xd6/0x160
  [<ffffffff8120900f>] ? put_object+0x2f/0x50
  [<ffffffff81465c1d>] xenbus_otherend_changed+0x9d/0xa0
  [<ffffffff814678ee>] backend_changed+0xe/0x10
  [<ffffffff81463a28>] xenwatch_thread+0xc8/0x190
  [<ffffffff810f22f0>] ? woken_wake_function+0x10/0x10

which was the result of two things:

When we call pci_scan_root_bus we would pass in 'sd' (sysdata)
pointer which was an 'pcifront_sd' structure. However in the
pci_device_add it expects that the 'sd' is 'struct sysdata' and
sets the dev->node to what is in sd->node (offset 4):

set_dev_node(&dev->dev, pcibus_to_node(bus));

 __pcibus_to_node(const struct pci_bus *bus)
{
        const struct pci_sysdata *sd = bus->sysdata;

        return sd->node;
}

However our structure was pcifront_sd which had nothing at that
offset:

struct pcifront_sd {
        int                        domain;    /*     0     4 */
        /* XXX 4 bytes hole, try to pack */
        struct pcifront_device *   pdev;      /*     8     8 */
}

That is an hole - filled with garbage as we used kmalloc instead of
kzalloc (the second problem).

This patch fixes the issue by:
 1) Use kzalloc to initialize to a well known state.
 2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
    way access to the 'node' will access the right offset.

CC: stable@vger.kernel.org
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/pci/xen-pcifront.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index c777b97..66d197d 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -53,7 +53,7 @@ struct pcifront_device {
 };
 
 struct pcifront_sd {
-	int domain;
+	struct pci_sysdata sd;
 	struct pcifront_device *pdev;
 };
 
@@ -67,7 +67,9 @@ static inline void pcifront_init_sd(struct pcifront_sd *sd,
 				    unsigned int domain, unsigned int bus,
 				    struct pcifront_device *pdev)
 {
-	sd->domain = domain;
+	/* Because we do not expose that information via XenBus. */
+	sd->sd.node = first_online_node;
+	sd->sd.domain = domain;
 	sd->pdev = pdev;
 }
 
@@ -468,8 +470,8 @@ static int pcifront_scan_root(struct pcifront_device *pdev,
 	dev_info(&pdev->xdev->dev, "Creating PCI Frontend Bus %04x:%02x\n",
 		 domain, bus);
 
-	bus_entry = kmalloc(sizeof(*bus_entry), GFP_KERNEL);
-	sd = kmalloc(sizeof(*sd), GFP_KERNEL);
+	bus_entry = kzalloc(sizeof(*bus_entry), GFP_KERNEL);
+	sd = kzalloc(sizeof(*sd), GFP_KERNEL);
 	if (!bus_entry || !sd) {
 		err = -ENOMEM;
 		goto err_out;
@@ -576,6 +578,7 @@ static void pcifront_free_roots(struct pcifront_device *pdev)
 		free_root_bus_devs(bus_entry->bus);
 
 		kfree(bus_entry->bus->sysdata);
+		bus_entry->bus->sysdata = NULL;
 
 		device_unregister(bus_entry->bus->bridge);
 		pci_remove_bus(bus_entry->bus);
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY
  2016-02-11 21:10 ` [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY Konrad Rzeszutek Wilk
@ 2016-02-12  9:19   ` Jan Beulich
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2016-02-12  9:19 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: linux-kernel, stable, Tommi Airikka, david.vrabel, xen-devel,
	boris.ostrovsky

>>> On 11.02.16 at 22:10, <konrad.wilk@oracle.com> wrote:
> c/s 408fb0e5aa7fda0059db282ff58c3b2a4278baa0
> "xen/pciback: Don't allow MSI-X ops if PCI_COMMAND_MEMORY is not set."
> would check the device for PCI_COMMAND_MEMORY which is great.
> Except that VF devices are unique - for example they have no
> legacy interrupts, and also any writes to PCI_COMMAND_MEMORY
> are silently ignored (by the hardware).
> 
> CC: stable@vger.kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit ...

> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -213,6 +213,7 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
>  	int i, result;
>  	struct msix_entry *entries;
>  	u16 cmd;
> +	struct pci_dev *phys_dev;
>  
>  	if (unlikely(verbose_request))
>  		printk(KERN_DEBUG DRV_NAME ": %s: enable MSI-X\n",
> @@ -227,8 +228,10 @@ int xen_pcibk_enable_msix(struct xen_pcibk_device *pdev,
>  	/*
>  	 * PCI_COMMAND_MEMORY must be enabled, otherwise we may not be able
>  	 * to access the BARs where the MSI-X entries reside.
> +	 * But VF devices are unique in which the PF needs to be checked.
>  	 */
> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> +	phys_dev = pci_physfn(dev);
> +	pci_read_config_word(phys_dev, PCI_COMMAND, &cmd);

... I don't think the use of a local variable here is really needed.

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later.
  2016-02-11 21:10 ` [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later Konrad Rzeszutek Wilk
@ 2016-02-12  9:25   ` Jan Beulich
  0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2016-02-12  9:25 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: linux-kernel, stable, Tommi Airikka, david.vrabel, xen-devel,
	boris.ostrovsky

>>> On 11.02.16 at 22:10, <konrad.wilk@oracle.com> wrote:
> c/s  8135cf8b092723dbfcc611fe6fdcb3a36c9951c5
> "xen/pciback: Save xen_pci_op commands before processing it"
> would copyback the processed values - which was great.
> 
> However it missed the case that xen_pcibk_enable_msix - when
> completing would overwrite op->value (which had the number
> of MSI-X vectors requested) with the return value (which for
> success was zero). Hence the copy-back routine (which would use
> op->value) would copy exactly zero MSI-X vectors back.
> 
> CC: stable@vger.kernel.org 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

but I'd prefer to see ...

> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -335,7 +335,9 @@ void xen_pcibk_do_op(struct work_struct *data)
>  	struct xen_pcibk_dev_data *dev_data = NULL;
>  	struct xen_pci_op *op = &pdev->op;
>  	int test_intx = 0;
> -
> +#ifdef CONFIG_PCI_MSI
> +	unsigned int nr = 0;
> +#endif
>  	*op = pdev->sh_info->op;
>  	barrier();
>  	dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn);

... the blank line separating declarations from statements to stay.

Jan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.
  2016-02-11 21:10 ` [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted Konrad Rzeszutek Wilk
@ 2016-02-14  1:23   ` Boris Ostrovsky
       [not found]   ` <56BFD702.9010708@oracle.com>
  2016-02-15 14:27   ` David Vrabel
  2 siblings, 0 replies; 14+ messages in thread
From: Boris Ostrovsky @ 2016-02-14  1:23 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Tommi Airikka, linux-kernel, xen-devel,
	david.vrabel
  Cc: stable

On 02/11/2016 04:10 PM, Konrad Rzeszutek Wilk wrote:
>
> This patch fixes the issue by:
>   1) Use kzalloc to initialize to a well known state.
>   2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
>      way access to the 'node' will access the right offset.
>
> CC: stable@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

(This, btw, is the second time we got bitten by pcifront_sd bit not 
being pci_sysdata. dc4fdaf0e48 was a workaround for a similar problem 
and we should have fixed it then).

-boris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.
       [not found]   ` <56BFD702.9010708@oracle.com>
@ 2016-02-15 14:05     ` Konrad Rzeszutek Wilk
  2016-02-15 14:37       ` Boris Ostrovsky
  0 siblings, 1 reply; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-15 14:05 UTC (permalink / raw)
  To: Boris Ostrovsky
  Cc: Tommi Airikka, xen-devel, linux-kernel, stable, david.vrabel

On Sat, Feb 13, 2016 at 08:23:14PM -0500, Boris Ostrovsky wrote:
> On 02/11/2016 04:10 PM, Konrad Rzeszutek Wilk wrote:
> >
> >This patch fixes the issue by:
> >  1) Use kzalloc to initialize to a well known state.
> >  2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
> >     way access to the 'node' will access the right offset.
> >
> >CC: stable@vger.kernel.org
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> (This, btw, is the second time we got bitten by pcifront_sd bit not being
> pci_sysdata. dc4fdaf0e48 was a workaround for a similar problem and we
> should have fixed it then).

I think that the dc4fdaf0e4839109169d8261814813816951c75f commit can be
reverted then?

Ah no, b/c:
"    Fixes: 97badf873ab6 (device property: Make it possible to use secondary firmware nodes)"

> 
> -boris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.
  2016-02-11 21:10 ` [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted Konrad Rzeszutek Wilk
  2016-02-14  1:23   ` Boris Ostrovsky
       [not found]   ` <56BFD702.9010708@oracle.com>
@ 2016-02-15 14:27   ` David Vrabel
  2 siblings, 0 replies; 14+ messages in thread
From: David Vrabel @ 2016-02-15 14:27 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Tommi Airikka, linux-kernel, xen-devel,
	david.vrabel, boris.ostrovsky
  Cc: stable

On 11/02/16 21:10, Konrad Rzeszutek Wilk wrote:
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c

> @@ -576,6 +578,7 @@ static void pcifront_free_roots(struct pcifront_device *pdev)
>  		free_root_bus_devs(bus_entry->bus);
>  
>  		kfree(bus_entry->bus->sysdata);
> +		bus_entry->bus->sysdata = NULL;

I dropped this change, as it was unrelated.

I note that we free sysdata before removing the bus which looks racy to me.

David

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Fixes to Xen pcifront and pciback (v1)
  2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
                   ` (3 preceding siblings ...)
  2016-02-11 21:10 ` [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted Konrad Rzeszutek Wilk
@ 2016-02-15 14:35 ` David Vrabel
       [not found] ` <56C1E24C.5040704@citrix.com>
  5 siblings, 0 replies; 14+ messages in thread
From: David Vrabel @ 2016-02-15 14:35 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Tommi Airikka, linux-kernel, xen-devel,
	david.vrabel, boris.ostrovsky

On 11/02/16 21:10, Konrad Rzeszutek Wilk wrote:
> Hey,
> 
> These are patches that were developed for the Debian bug
> 810379 which san Tommi had openned.
> 
> The issue around from the two XSA fixes - which introduced
> this regression.

Applied to for-linus-4.6, thanks.

I rewrote some of the commit messages for clarity.  Please only put
relevant information in them and for regression fixes, make sure it's
clear what functionality regressed.

e.g. #1 now reads:

    Commit 408fb0e5aa7fda0059db282ff58c3b2a4278baa0 (xen/pciback: Don't
    allow MSI-X ops if PCI_COMMAND_MEMORY is not set) prevented enabling
    MSI-X on passed-through virtual functions, because it checked the VF
    for PCI_COMMAND_MEMORY but this is not a valid bit for VFs.

    Instead, check the physical function for PCI_COMMAND_MEMORY.

David

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted.
  2016-02-15 14:05     ` Konrad Rzeszutek Wilk
@ 2016-02-15 14:37       ` Boris Ostrovsky
  0 siblings, 0 replies; 14+ messages in thread
From: Boris Ostrovsky @ 2016-02-15 14:37 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Tommi Airikka, xen-devel, linux-kernel, stable, david.vrabel

On 02/15/2016 09:05 AM, Konrad Rzeszutek Wilk wrote:
> On Sat, Feb 13, 2016 at 08:23:14PM -0500, Boris Ostrovsky wrote:
>> On 02/11/2016 04:10 PM, Konrad Rzeszutek Wilk wrote:
>>> This patch fixes the issue by:
>>>   1) Use kzalloc to initialize to a well known state.
>>>   2) Put 'struct pci_sysdata' at the start of 'pcifront_sd'. That
>>>      way access to the 'node' will access the right offset.
>>>
>>> CC: stable@vger.kernel.org
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> (This, btw, is the second time we got bitten by pcifront_sd bit not being
>> pci_sysdata. dc4fdaf0e48 was a workaround for a similar problem and we
>> should have fixed it then).
> I think that the dc4fdaf0e4839109169d8261814813816951c75f commit can be
> reverted then?
>
> Ah no, b/c:
> "    Fixes: 97badf873ab6 (device property: Make it possible to use secondary firmware nodes)"

Right --- the problem which that commit fixed was not specific to Xen 
but could be observed on ia64 as well.

-boris

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Fixes to Xen pcifront and pciback (v1)
       [not found] ` <56C1E24C.5040704@citrix.com>
@ 2016-02-15 14:38   ` David Vrabel
       [not found]   ` <56C1E2DE.7030507@citrix.com>
  1 sibling, 0 replies; 14+ messages in thread
From: David Vrabel @ 2016-02-15 14:38 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk, Tommi Airikka, linux-kernel, xen-devel,
	boris.ostrovsky

On 15/02/16 14:35, David Vrabel wrote:
> On 11/02/16 21:10, Konrad Rzeszutek Wilk wrote:
>> Hey,
>>
>> These are patches that were developed for the Debian bug
>> 810379 which san Tommi had openned.
>>
>> The issue around from the two XSA fixes - which introduced
>> this regression.
> 
> Applied to for-linus-4.6, thanks.

I mean for-linus-4.5.

I also fixed the minor style quibbles.

David

> I rewrote some of the commit messages for clarity.  Please only put
> relevant information in them and for regression fixes, make sure it's
> clear what functionality regressed.
> 
> e.g. #1 now reads:
> 
>     Commit 408fb0e5aa7fda0059db282ff58c3b2a4278baa0 (xen/pciback: Don't
>     allow MSI-X ops if PCI_COMMAND_MEMORY is not set) prevented enabling
>     MSI-X on passed-through virtual functions, because it checked the VF
>     for PCI_COMMAND_MEMORY but this is not a valid bit for VFs.
> 
>     Instead, check the physical function for PCI_COMMAND_MEMORY.
> 
> David
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Fixes to Xen pcifront and pciback (v1)
       [not found]   ` <56C1E2DE.7030507@citrix.com>
@ 2016-02-15 15:35     ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-02-15 15:35 UTC (permalink / raw)
  To: David Vrabel; +Cc: Tommi Airikka, xen-devel, boris.ostrovsky, linux-kernel

On Mon, Feb 15, 2016 at 02:38:22PM +0000, David Vrabel wrote:
> On 15/02/16 14:35, David Vrabel wrote:
> > On 11/02/16 21:10, Konrad Rzeszutek Wilk wrote:
> >> Hey,
> >>
> >> These are patches that were developed for the Debian bug
> >> 810379 which san Tommi had openned.
> >>
> >> The issue around from the two XSA fixes - which introduced
> >> this regression.
> > 
> > Applied to for-linus-4.6, thanks.
> 
> I mean for-linus-4.5.

Thanks!
> 
> I also fixed the minor style quibbles.

Thank you. It was written rather hurriedly.
> 
> David
> 
> > I rewrote some of the commit messages for clarity.  Please only put
> > relevant information in them and for regression fixes, make sure it's
> > clear what functionality regressed.
> > 
> > e.g. #1 now reads:
> > 
> >     Commit 408fb0e5aa7fda0059db282ff58c3b2a4278baa0 (xen/pciback: Don't
> >     allow MSI-X ops if PCI_COMMAND_MEMORY is not set) prevented enabling
> >     MSI-X on passed-through virtual functions, because it checked the VF
> >     for PCI_COMMAND_MEMORY but this is not a valid bit for VFs.
> > 
> >     Instead, check the physical function for PCI_COMMAND_MEMORY.
> > 
> > David
> > 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-02-15 15:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-11 21:10 [PATCH] Fixes to Xen pcifront and pciback (v1) Konrad Rzeszutek Wilk
2016-02-11 21:10 ` [PATCH 1/4] xen/pciback: Check PF instead of VF for PCI_COMMAND_MEMORY Konrad Rzeszutek Wilk
2016-02-12  9:19   ` Jan Beulich
2016-02-11 21:10 ` [PATCH 2/4] xen/pciback: Save the number of MSI-X entries to be copied later Konrad Rzeszutek Wilk
2016-02-12  9:25   ` Jan Beulich
2016-02-11 21:10 ` [PATCH 3/4] xen/pcifront: Report the errors better Konrad Rzeszutek Wilk
2016-02-11 21:10 ` [PATCH 4/4] xen/pcifront: Fix mysterious crashes when NUMA locality information was extracted Konrad Rzeszutek Wilk
2016-02-14  1:23   ` Boris Ostrovsky
     [not found]   ` <56BFD702.9010708@oracle.com>
2016-02-15 14:05     ` Konrad Rzeszutek Wilk
2016-02-15 14:37       ` Boris Ostrovsky
2016-02-15 14:27   ` David Vrabel
2016-02-15 14:35 ` [PATCH] Fixes to Xen pcifront and pciback (v1) David Vrabel
     [not found] ` <56C1E24C.5040704@citrix.com>
2016-02-15 14:38   ` David Vrabel
     [not found]   ` <56C1E2DE.7030507@citrix.com>
2016-02-15 15:35     ` Konrad Rzeszutek Wilk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).