[PATCH V4] Decouple SandyBridge quirk from VTd timeout

[PATCH V4] Decouple SandyBridge quirk from 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	Thu Nov 20 22:00:51 2014 -0700
@@ -50,6 +50,11 @@
 #define IS_ILK(id)    (id == 0x00408086 || id == 0x00448086 || id== 0x00628086 || id == 0x006A8086)
 #define IS_CPT(id)    (id == 0x01008086 || id == 0x01048086)
 
+/* SandyBridge IGD timeouts in milliseconds */
+#define SNB_IGD_TIMEOUT_LEGACY    1000
+#define SNB_IGD_TIMEOUT            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 +163,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 +192,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 +223,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 +240,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 +249,36 @@
     }
 }
 
+static void __init parse_snb_timeout(const char *s)
+{
+    int t;
+
+    t = parse_bool(s);
+    if ( t < 0 )
+    {
+        if ( *s == '\0' )
+        {
+            t = SNB_IGD_TIMEOUT_LEGACY;
+        }
+        else if ( strcmp(s, "default") == 0 )
+        {
+            t = SNB_IGD_TIMEOUT;
+        }
+        else
+        {
+            t = strtoul(s, NULL, 0);
+        }
+    }
+    else
+    {
+        t = t ? SNB_IGD_TIMEOUT_LEGACY : 0;
+    }
+    snb_igd_timeout = MILLISECS(t);
+
+    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 V4] Decouple SandyBridge quirk from VTd timeout

[Tian, Kevin]

> From: Donald D. Dugger
> Sent: Friday, November 21, 2014 1:12 PM
> 
> 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.

I'm OK with the patch except one comment. User usually thinks bool option
means default, but here an explicit 'default' actually means a different value.
It's a bit confused to me. How about changing 'default' to another more
meaningful word, e.g. 'max'? and you may still keep a 'snb_igd_quirk=
legacy' option, so all the possibilities are included in the assigned case, with
bool option alias to 'legacy'.

Thanks
Kevin

> 
> 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	Thu Nov 20 22:00:51 2014
> -0700
> @@ -50,6 +50,11 @@
>  #define IS_ILK(id)    (id == 0x00408086 || id == 0x00448086 || id==
> 0x00628086 || id == 0x006A8086)
>  #define IS_CPT(id)    (id == 0x01008086 || id == 0x01048086)
> 
> +/* SandyBridge IGD timeouts in milliseconds */
> +#define SNB_IGD_TIMEOUT_LEGACY    1000
> +#define SNB_IGD_TIMEOUT            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 +163,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 +192,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 +223,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 +240,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 +249,36 @@
>      }
>  }
> 
> +static void __init parse_snb_timeout(const char *s)
> +{
> +    int t;
> +
> +    t = parse_bool(s);
> +    if ( t < 0 )
> +    {
> +        if ( *s == '\0' )
> +        {
> +            t = SNB_IGD_TIMEOUT_LEGACY;
> +        }
> +        else if ( strcmp(s, "default") == 0 )
> +        {
> +            t = SNB_IGD_TIMEOUT;
> +        }
> +        else
> +        {
> +            t = strtoul(s, NULL, 0);
> +        }
> +    }
> +    else
> +    {
> +        t = t ? SNB_IGD_TIMEOUT_LEGACY : 0;
> +    }
> +    snb_igd_timeout = MILLISECS(t);
> +
> +    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)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH V4] Decouple SandyBridge quirk from VTd timeout

[Don Dugger]

On Fri, Dec 05, 2014 at 05:30:52AM +0000, Tian, Kevin wrote:
> > From: Donald D. Dugger
> > Sent: Friday, November 21, 2014 1:12 PM
> > 
> > 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.
> 
> I'm OK with the patch except one comment. User usually thinks bool option
> means default, but here an explicit 'default' actually means a different value.
> It's a bit confused to me. How about changing 'default' to another more
> meaningful word, e.g. 'max'? and you may still keep a 'snb_igd_quirk=
> legacy' option, so all the possibilities are included in the assigned case, with
> bool option alias to 'legacy'.

I don't see a need for a `legacy' option, merely enabling the option (which is
what current users are doing) will select the current 1 sec. timeout.  If you
don't like `default' (and I don't like `max' since that would select a value
less that the current default) how about the string `cap' (the cap on the
theoretical timeout) to select the 670 msec value?

> 
> Thanks
> Kevin

...

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

Re: [PATCH V4] Decouple SandyBridge quirk from VTd timeout

[Tian, Kevin]

> From: Don Dugger [mailto:n0ano@n0ano.com]
> Sent: Friday, December 05, 2014 2:13 PM
> 
> On Fri, Dec 05, 2014 at 05:30:52AM +0000, Tian, Kevin wrote:
> > > From: Donald D. Dugger
> > > Sent: Friday, November 21, 2014 1:12 PM
> > >
> > > 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.
> >
> > I'm OK with the patch except one comment. User usually thinks bool option
> > means default, but here an explicit 'default' actually means a different
> value.
> > It's a bit confused to me. How about changing 'default' to another more
> > meaningful word, e.g. 'max'? and you may still keep a 'snb_igd_quirk=
> > legacy' option, so all the possibilities are included in the assigned case, with
> > bool option alias to 'legacy'.
> 
> I don't see a need for a `legacy' option, merely enabling the option (which is
> what current users are doing) will select the current 1 sec. timeout.  If you
> don't like `default' (and I don't like `max' since that would select a value
> less that the current default) how about the string `cap' (the cap on the
> theoretical timeout) to select the 670 msec value?
> 

that looks fine to me. You can have my ACK for your next version. :-)

Acked-by: Kevin Tian <kevin.tian@intel.com>