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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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@xxxxxxxxx>
> --
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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