From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [BUG] pci-passthrough generates "xen:events: Failed to obtain physical IRQ" for some devices Date: Mon, 8 Feb 2016 23:59:27 -0500 Message-ID: <20160209045927.GC3853@localhost.localdomain> References: <20160127183005.GB3134@char.us.oracle.com> <1454323426.28781.73.camel@citrix.com> <20160201145053.GA21826@char.us.oracle.com> <20160203142230.GC24446@mail-itl> <20160203152657.GE20732@char.us.oracle.com> <20160208173917.GD24446@mail-itl> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20160208173917.GD24446@mail-itl> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Marek =?iso-8859-1?Q?Marczykowski-G=F3recki?= Cc: Tommi Airikka , Ian Campbell , 810379@bugs.debian.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Mon, Feb 08, 2016 at 06:39:17PM +0100, Marek Marczykowski-G=F3recki wrot= e: > 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=F3recki = 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=A0http://xenbits.xen.org/xsa/advisory-157.html=A0for 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 betw= een > > > 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_in= fo->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 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 = 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-pcibac= k/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 >=3D 0 ? 0 : XEN_PCI_ERR_op_failed; } = #endif diff --git a/include/xen/interface/io/pciif.h b/include/xen/interface/io/pc= iif.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=F3recki > 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?