From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 3/3] passthrough: allow to suppress SERR and PERR signaling altogether Date: Mon, 7 Apr 2014 13:47:40 +0100 Message-ID: <53429E6C.4090105@citrix.com> References: <533D471D0200007800005104@nat28.tlf.novell.com> <533D48D50200007800005128@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4918872934956586593==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WX8yE-0007zx-5Y for xen-devel@lists.xenproject.org; Mon, 07 Apr 2014 12:48:14 +0000 In-Reply-To: <533D48D50200007800005128@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , Asit K Mallick , Donald D Dugger , Jun Nakajima , xen-devel , xiantao.zhang@intel.com List-Id: xen-devel@lists.xenproject.org --===============4918872934956586593== Content-Type: multipart/alternative; boundary="------------010307080000090700070902" --------------010307080000090700070902 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 03/04/14 10:41, Jan Beulich wrote: > This is just to have a workaround at hand in case other chipsets (not > covered by the previous two patches) also have similar issues. > > Signed-off-by: Jan Beulich > > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -154,6 +154,112 @@ static void __init parse_phantom_dev(cha > } > custom_param("pci-phantom", parse_phantom_dev); > > +static u16 __read_mostly command_mask; > +static u16 __read_mostly bridge_ctl_mask; > + > +/* > + * The 'pci' parameter controls certain PCI device aspects. > + * Optional comma separated value may contain: > + * > + * serr don't suppress system errors (default) > + * no-serr suppress system errors > + * perr don't suppress parity errors (default) > + * no-perr suppress parity errors > + */ > +static void __init parse_pci_param(char *s) > +{ > + char *ss; > + > + do { > + bool_t on = !!strncmp(s, "no-", 3); > + u16 cmd_mask = 0, brctl_mask = 0; > + > + if ( !on ) > + s += 3; > + > + ss = strchr(s, ','); > + if ( ss ) > + *ss = '\0'; > + > + if ( !strcmp(s, "serr") ) > + { > + cmd_mask = PCI_COMMAND_SERR; > + brctl_mask = PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_DTMR_SERR; > + } > + else if ( !strcmp(s, "perr") ) > + { > + cmd_mask = PCI_COMMAND_PARITY; > + brctl_mask = PCI_BRIDGE_CTL_PARITY; > + } > + > + if ( on ) > + { > + command_mask &= ~cmd_mask; > + bridge_ctl_mask &= ~brctl_mask; > + } > + else > + { > + command_mask |= cmd_mask; > + bridge_ctl_mask |= brctl_mask; > + } > + > + s = ss + 1; > + } while ( ss ); > +} > +custom_param("pci", parse_pci_param); > + > +static void check_pdev(const struct pci_dev *pdev) > +{ > +#define PCI_STATUS_CHECK \ > + (PCI_STATUS_PARITY | PCI_STATUS_SIG_TARGET_ABORT | \ > + PCI_STATUS_REC_TARGET_ABORT | PCI_STATUS_REC_MASTER_ABORT | \ > + PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY) > + u16 seg = pdev->seg; > + u8 bus = pdev->bus; > + u8 dev = PCI_SLOT(pdev->devfn); > + u8 func = PCI_FUNC(pdev->devfn); > + u16 val; > + > + if ( command_mask ) > + { > + val = pci_conf_read16(seg, bus, dev, func, PCI_COMMAND); > + if ( val & command_mask ) > + pci_conf_write16(seg, bus, dev, func, PCI_COMMAND, > + val & ~command_mask); > + val = pci_conf_read16(seg, bus, dev, func, PCI_STATUS); > + if ( val & PCI_STATUS_CHECK ) > + { > + printk(XENLOG_INFO "%04x:%02x:%02x.%u status %04x\n", > + seg, bus, dev, func, val); What is the purpose of this printk? From the text alone it is not obvious. > + pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val); I dont think this code has any right to clear status bits other than the ones it is checking for, so the write should be "val & PCI_STATUS_CHECK" ~Andrew > + } > + } > + > + switch ( pci_conf_read8(seg, bus, dev, func, PCI_HEADER_TYPE) & 0x7f ) > + { > + case PCI_HEADER_TYPE_BRIDGE: > + if ( !bridge_ctl_mask ) > + break; > + val = pci_conf_read16(seg, bus, dev, func, PCI_BRIDGE_CONTROL); > + if ( val & bridge_ctl_mask ) > + pci_conf_write16(seg, bus, dev, func, PCI_BRIDGE_CONTROL, > + val & ~bridge_ctl_mask); > + val = pci_conf_read16(seg, bus, dev, func, PCI_SEC_STATUS); > + if ( val & PCI_STATUS_CHECK ) > + { > + printk(XENLOG_INFO "%04x:%02x:%02x.%u secondary status %04x\n", > + seg, bus, dev, func, val); > + pci_conf_write16(seg, bus, dev, func, PCI_SEC_STATUS, val); > + } > + break; > + > + case PCI_HEADER_TYPE_CARDBUS: > + /* TODO */ > + break; > + } > +#undef PCI_STATUS_CHECK > +} > + > static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn) > { > struct pci_dev *pdev; > @@ -252,6 +358,8 @@ static struct pci_dev *alloc_pdev(struct > break; > } > > + check_pdev(pdev); > + > return pdev; > } > > @@ -566,6 +674,8 @@ int pci_add_device(u16 seg, u8 bus, u8 d > seg, bus, slot, func, ctrl); > } > > + check_pdev(pdev); > + > ret = 0; > if ( !pdev->domain ) > { > --- a/xen/include/xen/pci_regs.h > +++ b/xen/include/xen/pci_regs.h > @@ -125,7 +125,7 @@ > #define PCI_IO_RANGE_TYPE_16 0x00 > #define PCI_IO_RANGE_TYPE_32 0x01 > #define PCI_IO_RANGE_MASK (~0x0fUL) > -#define PCI_SEC_STATUS 0x1e /* Secondary status register, only bit 14 used */ > +#define PCI_SEC_STATUS 0x1e /* Secondary status register */ > #define PCI_MEMORY_BASE 0x20 /* Memory range behind */ > #define PCI_MEMORY_LIMIT 0x22 > #define PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL > @@ -152,6 +152,7 @@ > #define PCI_BRIDGE_CTL_MASTER_ABORT 0x20 /* Report master aborts */ > #define PCI_BRIDGE_CTL_BUS_RESET 0x40 /* Secondary bus reset */ > #define PCI_BRIDGE_CTL_FAST_BACK 0x80 /* Fast Back2Back enabled on secondary interface */ > +#define PCI_BRIDGE_CTL_DTMR_SERR 0x800 /* SERR upon discard timer expiry */ > > /* Header type 2 (CardBus bridges) */ > #define PCI_CB_CAPABILITY_LIST 0x14 > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------010307080000090700070902 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 03/04/14 10:41, Jan Beulich wrote:
This is just to have a workaround at hand in case other chipsets (not
covered by the previous two patches) also have similar issues.

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

--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -154,6 +154,112 @@ static void __init parse_phantom_dev(cha
 }
 custom_param("pci-phantom", parse_phantom_dev);
 
+static u16 __read_mostly command_mask;
+static u16 __read_mostly bridge_ctl_mask;
+
+/*
+ * The 'pci' parameter controls certain PCI device aspects.
+ * Optional comma separated value may contain:
+ *
+ *   serr                       don't suppress system errors (default)
+ *   no-serr                    suppress system errors
+ *   perr                       don't suppress parity errors (default)
+ *   no-perr                    suppress parity errors
+ */
+static void __init parse_pci_param(char *s)
+{
+    char *ss;
+
+    do {
+        bool_t on = !!strncmp(s, "no-", 3);
+        u16 cmd_mask = 0, brctl_mask = 0;
+
+        if ( !on )
+            s += 3;
+
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        if ( !strcmp(s, "serr") )
+        {
+            cmd_mask = PCI_COMMAND_SERR;
+            brctl_mask = PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_DTMR_SERR;
+        }
+        else if ( !strcmp(s, "perr") )
+        {
+            cmd_mask = PCI_COMMAND_PARITY;
+            brctl_mask = PCI_BRIDGE_CTL_PARITY;
+        }
+
+        if ( on )
+        {
+            command_mask &= ~cmd_mask;
+            bridge_ctl_mask &= ~brctl_mask;
+        }
+        else
+        {
+            command_mask |= cmd_mask;
+            bridge_ctl_mask |= brctl_mask;
+        }
+
+        s = ss + 1;
+    } while ( ss );
+}
+custom_param("pci", parse_pci_param);
+
+static void check_pdev(const struct pci_dev *pdev)
+{
+#define PCI_STATUS_CHECK \
+    (PCI_STATUS_PARITY | PCI_STATUS_SIG_TARGET_ABORT | \
+     PCI_STATUS_REC_TARGET_ABORT | PCI_STATUS_REC_MASTER_ABORT | \
+     PCI_STATUS_SIG_SYSTEM_ERROR | PCI_STATUS_DETECTED_PARITY)
+    u16 seg = pdev->seg;
+    u8 bus = pdev->bus;
+    u8 dev = PCI_SLOT(pdev->devfn);
+    u8 func = PCI_FUNC(pdev->devfn);
+    u16 val;
+
+    if ( command_mask )
+    {
+        val = pci_conf_read16(seg, bus, dev, func, PCI_COMMAND);
+        if ( val & command_mask )
+            pci_conf_write16(seg, bus, dev, func, PCI_COMMAND,
+                             val & ~command_mask);
+        val = pci_conf_read16(seg, bus, dev, func, PCI_STATUS);
+        if ( val & PCI_STATUS_CHECK )
+        {
+            printk(XENLOG_INFO "%04x:%02x:%02x.%u status %04x\n",
+                   seg, bus, dev, func, val);

What is the purpose of this printk?  From the text alone it is not obvious.

+            pci_conf_write16(seg, bus, dev, func, PCI_STATUS, val);

I dont think this code has any right to clear status bits other than the ones it is checking for, so the write should be "val & PCI_STATUS_CHECK"

~Andrew

+        }
+    }
+
+    switch ( pci_conf_read8(seg, bus, dev, func, PCI_HEADER_TYPE) & 0x7f )
+    {
+    case PCI_HEADER_TYPE_BRIDGE:
+        if ( !bridge_ctl_mask )
+            break;
+        val = pci_conf_read16(seg, bus, dev, func, PCI_BRIDGE_CONTROL);
+        if ( val & bridge_ctl_mask )
+            pci_conf_write16(seg, bus, dev, func, PCI_BRIDGE_CONTROL,
+                             val & ~bridge_ctl_mask);
+        val = pci_conf_read16(seg, bus, dev, func, PCI_SEC_STATUS);
+        if ( val & PCI_STATUS_CHECK )
+        {
+            printk(XENLOG_INFO "%04x:%02x:%02x.%u secondary status %04x\n",
+                   seg, bus, dev, func, val);
+            pci_conf_write16(seg, bus, dev, func, PCI_SEC_STATUS, val);
+        }
+        break;
+
+    case PCI_HEADER_TYPE_CARDBUS:
+        /* TODO */
+        break;
+    }
+#undef PCI_STATUS_CHECK
+}
+
 static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
 {
     struct pci_dev *pdev;
@@ -252,6 +358,8 @@ static struct pci_dev *alloc_pdev(struct
             break;
     }
 
+    check_pdev(pdev);
+
     return pdev;
 }
 
@@ -566,6 +674,8 @@ int pci_add_device(u16 seg, u8 bus, u8 d
                    seg, bus, slot, func, ctrl);
     }
 
+    check_pdev(pdev);
+
     ret = 0;
     if ( !pdev->domain )
     {
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -125,7 +125,7 @@
 #define  PCI_IO_RANGE_TYPE_16	0x00
 #define  PCI_IO_RANGE_TYPE_32	0x01
 #define  PCI_IO_RANGE_MASK	(~0x0fUL)
-#define PCI_SEC_STATUS		0x1e	/* Secondary status register, only bit 14 used */
+#define PCI_SEC_STATUS		0x1e	/* Secondary status register */
 #define PCI_MEMORY_BASE		0x20	/* Memory range behind */
 #define PCI_MEMORY_LIMIT	0x22
 #define  PCI_MEMORY_RANGE_TYPE_MASK 0x0fUL
@@ -152,6 +152,7 @@
 #define  PCI_BRIDGE_CTL_MASTER_ABORT	0x20  /* Report master aborts */
 #define  PCI_BRIDGE_CTL_BUS_RESET	0x40	/* Secondary bus reset */
 #define  PCI_BRIDGE_CTL_FAST_BACK	0x80	/* Fast Back2Back enabled on secondary interface */
+#define  PCI_BRIDGE_CTL_DTMR_SERR	0x800	/* SERR upon discard timer expiry */
 
 /* Header type 2 (CardBus bridges) */
 #define PCI_CB_CAPABILITY_LIST	0x14





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

--------------010307080000090700070902-- --===============4918872934956586593== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4918872934956586593==--