[PATCH V3] Decouple SnadyBridge quirk form VTd timeout

[PATCH V3] Decouple SnadyBridge quirk form VTd timeout

[Donald D. Dugger]

Currently the quirk code for SandyBridge uses the VTd timeout value when
writing to an IGD register.  This is the wrong timeout to use and, at
1000 msec., is also much too large.  This patch changes the quirk code to
use a timeout that is specific to the IGD device and allows the user
control of the timeout.

Boolean settings for the boot parameter `snb_igd_quirk' keep their current
meaning, enabling or disabling the quirk code with a timeout of 1000 msec.

In addition specifying `snb_igd_quirk=default' will enable the code and
set the timeout to the theoretical maximum of 670 msec.  For finer control,
specifying `snb_igd_quirk=n', where `n' is a decimal number, will enable
the code and set the timeout to `n' msec.

Signed-off-by: Don Dugger <donald.d.dugger@intel.com>
-- 
diff -r 9d485e2c8339 xen/drivers/passthrough/vtd/quirks.c
--- a/xen/drivers/passthrough/vtd/quirks.c	Mon Nov 10 12:03:36 2014 +0000
+++ b/xen/drivers/passthrough/vtd/quirks.c	Wed Nov 19 09:49:31 2014 -0700
@@ -50,6 +50,10 @@
 #define IS_ILK(id)    (id == 0x00408086 || id == 0x00448086 || id== 0x00628086 || id == 0x006A8086)
 #define IS_CPT(id)    (id == 0x01008086 || id == 0x01048086)
 
+#define SNB_IGD_TIMEOUT_LEGACY	MILLISECS(1000)
+#define SNB_IGD_TIMEOUT		MILLISECS( 670)
+static u32 snb_igd_timeout = 0;
+
 static u32 __read_mostly ioh_id;
 static u32 __initdata igd_id;
 bool_t __read_mostly rwbf_quirk;
@@ -158,6 +162,16 @@
  * Workaround is to prevent graphics get into RC6
  * state when doing VT-d IOTLB operations, do the VT-d
  * IOTLB operation, and then re-enable RC6 state.
+ *
+ * This quirk is enabled with the snb_igd_quirk command
+ * line parameter.  Specifying snb_igd_quirk with no value
+ * (or any of the standard boolean values) enables this
+ * quirk and sets the timeout to the legacy timeout of
+ * 1000 msec.  Setting this parameter to the string
+ * "default" enables this quirk and sets the timeout to
+ * the theoretical maximum of 670 msec.  Setting this
+ * parameter to a numerical value enables the quirk and
+ * sets the timeout to that numerical number of msecs.
  */
 static void snb_vtd_ops_preamble(struct iommu* iommu)
 {
@@ -177,7 +191,7 @@
     start_time = NOW();
     while ( (*(volatile u32 *)(igd_reg_va + 0x22AC) & 0xF) != 0 )
     {
-        if ( NOW() > start_time + DMAR_OPERATION_TIMEOUT )
+        if ( NOW() > start_time + snb_igd_timeout )
         {
             dprintk(XENLOG_INFO VTDPREFIX,
                     "snb_vtd_ops_preamble: failed to disable idle handshake\n");
@@ -208,13 +222,10 @@
  * call before VT-d translation enable and IOTLB flush operations.
  */
 
-static int snb_igd_quirk;
-boolean_param("snb_igd_quirk", snb_igd_quirk);
-
 void vtd_ops_preamble_quirk(struct iommu* iommu)
 {
     cantiga_vtd_ops_preamble(iommu);
-    if ( snb_igd_quirk )
+    if ( snb_igd_timeout != 0 )
     {
         spin_lock(&igd_lock);
 
@@ -228,7 +239,7 @@
  */
 void vtd_ops_postamble_quirk(struct iommu* iommu)
 {
-    if ( snb_igd_quirk )
+    if ( snb_igd_timeout != 0 )
     {
         snb_vtd_ops_postamble(iommu);
 
@@ -237,6 +248,42 @@
     }
 }
 
+static void __init parse_snb_timeout(const char *s)
+{
+	int not;
+
+	switch (*s) {
+
+	case '\0':
+		snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
+		break;
+
+	case '0':	case '1':	case '2':
+	case '3':	case '4':	case '5':
+	case '6':	case '7':	case '8':
+	case '9':
+		snb_igd_timeout = MILLISECS(simple_strtoul(s, &s, 0));
+		if ( snb_igd_timeout == MILLISECS(1) )
+			snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
+		break;
+
+	default:
+		if ( strncmp("default", s, 7) == 0 ) {
+			snb_igd_timeout = SNB_IGD_TIMEOUT;
+			break;
+		}
+		not = !strncmp("no-", s, 3);
+		if ( not )
+			s += 3;
+		if ( not ^ parse_bool(s) )
+			snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
+		break;
+
+	}
+	return;
+}
+custom_param("snb_igd_quirk", parse_snb_timeout);
+
 /* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
  * Fixed in stepping C-2. */
 static void __init tylersburg_intremap_quirk(void)

Re: [PATCH V3] Decouple SnadyBridge quirk form VTd timeout

[Ian Campbell]

On Wed, 2014-11-19 at 12:46 -0700, Donald D. Dugger wrote:
> Currently the quirk code for SandyBridge uses the VTd timeout value when

You've got a typo in the subject ("SnadyBridge").

Re: [PATCH V3] Decouple SnadyBridge quirk form VTd timeout

[Jan Beulich]

>>> On 19.11.14 at 20:46, <donald.d.dugger@intel.com> wrote:
> @@ -237,6 +248,42 @@
>      }
>  }
>  
> +static void __init parse_snb_timeout(const char *s)
> +{
> +	int not;
> +
> +	switch (*s) {
> +
> +	case '\0':
> +		snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> +		break;
> +
> +	case '0':	case '1':	case '2':
> +	case '3':	case '4':	case '5':
> +	case '6':	case '7':	case '8':
> +	case '9':
> +		snb_igd_timeout = MILLISECS(simple_strtoul(s, &s, 0));
> +		if ( snb_igd_timeout == MILLISECS(1) )
> +			snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> +		break;

Overly complicated. Just parse_bool() first, if that returns negative
check for "default" or "", and (if not matched) invoke strtoul(). No
need for this switch statement.

> +
> +	default:
> +		if ( strncmp("default", s, 7) == 0 ) {
> +			snb_igd_timeout = SNB_IGD_TIMEOUT;
> +			break;
> +		}
> +		not = !strncmp("no-", s, 3);

This makes no sense - you're looking for e.g. "snb_igd_quirk=no-no"
here. If the use specified "no-snb_igd_quirk", you'll end up seeing
"=no" when this function gets entered.

Also the whole function is white space damaged (using hard tabs)
and has misplaced opening braces.

Jan

Re: [PATCH V3] Decouple SnadyBridge quirk form VTd timeout

[Donald D. Dugger]

On Thu, Nov 20, 2014 at 12:31:48PM +0100, Jan Beulich wrote:
> >>> On 19.11.14 at 20:46, <donald.d.dugger@intel.com> wrote:
> > @@ -237,6 +248,42 @@
> >      }
> >  }
> >  
> > +static void __init parse_snb_timeout(const char *s)
> > +{
> > +	int not;
> > +
> > +	switch (*s) {
> > +
> > +	case '\0':
> > +		snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> > +		break;
> > +
> > +	case '0':	case '1':	case '2':
> > +	case '3':	case '4':	case '5':
> > +	case '6':	case '7':	case '8':
> > +	case '9':
> > +		snb_igd_timeout = MILLISECS(simple_strtoul(s, &s, 0));
> > +		if ( snb_igd_timeout == MILLISECS(1) )
> > +			snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> > +		break;
> 
> Overly complicated. Just parse_bool() first, if that returns negative
> check for "default" or "", and (if not matched) invoke strtoul(). No
> need for this switch statement.

Personally, I prefer switch statements whenever possible, they're linear
(if statements are evil) and the compiler optimizes them well.  Anyway,
NP, I'll follow your suggestion.

> 
> > +
> > +	default:
> > +		if ( strncmp("default", s, 7) == 0 ) {
> > +			snb_igd_timeout = SNB_IGD_TIMEOUT;
> > +			break;
> > +		}
> > +		not = !strncmp("no-", s, 3);
> 
> This makes no sense - you're looking for e.g. "snb_igd_quirk=no-no"
> here. If the use specified "no-snb_igd_quirk", you'll end up seeing
> "=no" when this function gets entered.

I was confused about the `no-' prefix, I thought it applied to the
value when it is really applied to the key (which makes a lot more
sense).  Fixed in next version.

> 
> Also the whole function is white space damaged (using hard tabs)
> and has misplaced opening braces.

Forgot Xen doesn't like tabs, my bad.

> 
> Jan
> 
> 

And, as Ian pointed out, dyslexia in the subject line, also fixed in the
next version.

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
n0ano@n0ano.com
Ph: 303/443-3786