Re: [BUG] pci-passthrough generates "xen:events: Failed to obtain physical IRQ" for some devices

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Feb 08, 2016 at 06:39:17PM +0100, Marek Marczykowski-Górecki wrote:
> On Wed, Feb 03, 2016 at 10:26:58AM -0500, Konrad Rzeszutek Wilk wrote:
> > On Wed, Feb 03, 2016 at 03:22:30PM +0100, Marek Marczykowski-Górecki wrote:
> > > On Mon, Feb 01, 2016 at 09:50:53AM -0500, Konrad Rzeszutek Wilk wrote:
> > > > > The second bullet looks at first pretty interesting from this PoV,
> > > > > see http://xenbits.xen.org/xsa/advisory-157.html for info on the XSA and
> > > > > the various patches. Konrad is on the CC already so hopefully he has some
> > > > > ideas.
> > > > 
> > > > Thanks. I will try to reproduce this with the upstream kernel first as
> > > > those patches are there.
> > > 
> > > According to one Qubes OS user report[1], the bug was introduced between
> > > version, which differs only by XSA-155 patches (including one for
> > > pciback), especially not XSA-157. 
> > > Maybe on some code path, some value is not copied back to pdev->sh_info->op?
> > 
> > I found two bugs (attached the draft not-compiled patches). Upstream
> > wise I seem to be tripping over another issue.
> > 
> > There is also some more work required in there to fix the MSI-x enable op.
> 
> What exactly do you have in mind here? That four patches in your next
> email? Or something not yet fixed?

I posted it at some point. It was that the MSI-X enable op stashes the
error value in op->value. But 'op->value' is an unsigned int so the
value ends up being 0xfffffe or such. And the other PV frontends only
check for !0 - and manufacture their own value (-EINVAL).

Hence I want to update the pciff.h .. Oh here is the patch:
Oh man. A year?!

Anyhow this can be posted as a cleanup patch seperately of the
bug-fixes.

commit 393be47782bca7a24d3e365448d4d3d1a303abfe
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Wed Apr 1 17:01:26 2015 -0400

    xen/pcifront/pciback: Update pciif.h with ->err and ->result values.
    
    The '->err' should contain only the XEN_PCI_ERR_* type values.
    The '->result' may contain -EXX values or any other value
    that the XEN_PCI_OP_* deems appropiate.
    
    As such update the header and also the implementations.
    
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    
    Conflicts:
    	drivers/xen/xen-pciback/pciback_ops.c
    
    Conflicts:
    	drivers/xen/xen-pciback/pciback_ops.c

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index b1ffebe..353c8a2 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -297,7 +297,7 @@ static int pci_frontend_enable_msix(struct pci_dev *dev,
 	} else {
 		dev_err(&dev->dev, "enable msix get err %x\n", err);
 	}
-	return err;
+	return err ? -EINVAL : 0;
 }
 
 static void pci_frontend_disable_msix(struct pci_dev *dev)
diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
index fa2b222..4db6c19 100644
--- a/drivers/xen/xen-pciback/pciback_ops.c
+++ b/drivers/xen/xen-pciback/pciback_ops.c
@@ -266,7 +266,7 @@ error:
 		pr_warn_ratelimited("%s: error enabling MSI-X for guest %u: err %d!\n",
 				    pci_name(dev), pdev->xdev->otherend_id,
 				    result);
-	return result > 0 ? 0 : result;
+	return result >= 0 ? 0 : XEN_PCI_ERR_op_failed;
 }
 
 #endif
diff --git a/include/xen/interface/io/pciif.h b/include/xen/interface/io/pciif.h
index d9922ae..c8b674f 100644
--- a/include/xen/interface/io/pciif.h
+++ b/include/xen/interface/io/pciif.h
@@ -70,7 +70,7 @@ struct xen_pci_op {
 	/* IN: what action to perform: XEN_PCI_OP_* */
 	uint32_t cmd;
 
-	/* OUT: will contain an error number (if any) from errno.h */
+	/* OUT: will contain an XEN_PCI_ERR_* number. */
 	int32_t err;
 
 	/* IN: which device to touch */
@@ -82,7 +82,9 @@ struct xen_pci_op {
 	int32_t offset;
 	int32_t size;
 
-	/* IN/OUT: Contains the result after a READ or the value to WRITE */
+	/* IN/OUT: Contains the result after a READ or the value to WRITE.
+	 * If the err does not have XEN_PCI_ERR_success, depending on
+	 * XEN_PCI_OP_* might have the errno value. */
 	uint32_t value;
 	/* IN: Contains extra infor for this operation */
 	uint32_t info;


> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?