[PATCH] x86/ats: Disable Address Translation Services by default

[PATCH] x86/ats: Disable Address Translation Services by default

[Andrew Cooper]

Xen cannot safely use any ATS functionality until it gains asynchronous queued
invalidation support, because of the current synchronous wait for completion.

Do not turn ATS on by default.

While editing the default in the command line documentation, correct the
statement regarding PCI Passthrough.  ATS is purely a performance
optimisation, and is certainly not required for PCI Passthrough to function.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
---
 docs/misc/xen-command-line.markdown |    9 ++++++---
 xen/drivers/passthrough/x86/ats.c   |    2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a8cab59..5f4680f 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -176,10 +176,13 @@ developers wishing Xen to fall back to older timing methods on newer hardware.
 ### ats
 > `= <boolean>`
 
-> Default: `true`
+> Default: `false`
+
+Permits Xen to set up and use PCI Address Translation Services.  This is a
+performance optimisation for PCI Passthrough.
 
-Permits Xen to set up and use PCI Address Translation Services, which
-is required for PCI Passthrough.
+**WARNING: Xen cannot currently safely use ATS because of its synchronous wait
+loops for Queued Invalidation completions.**
 
 ### availmem
 > `= <size>`
diff --git a/xen/drivers/passthrough/x86/ats.c b/xen/drivers/passthrough/x86/ats.c
index 1e3e03a..436eada 100644
--- a/xen/drivers/passthrough/x86/ats.c
+++ b/xen/drivers/passthrough/x86/ats.c
@@ -20,7 +20,7 @@
 
 LIST_HEAD(ats_devices);
 
-bool_t __read_mostly ats_enabled = 1;
+bool_t __read_mostly ats_enabled = 0;
 boolean_param("ats", ats_enabled);
 
 int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
-- 
1.7.10.4

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Jan Beulich]

>>> On 20.08.14 at 18:01, <andrew.cooper3@citrix.com> wrote:
> Xen cannot safely use any ATS functionality until it gains asynchronous 
> queued
> invalidation support, because of the current synchronous wait for 
> completion.
> 
> Do not turn ATS on by default.
> 
> While editing the default in the command line documentation, correct the
> statement regarding PCI Passthrough.  ATS is purely a performance
> optimisation, and is certainly not required for PCI Passthrough to function.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>

Even if not mandated by ./MAINTAINERS I think this definitely
should have been Cc-ed to the VT-d and AMD IOMMU maintainers
(now done).

Jan

> ---
>  docs/misc/xen-command-line.markdown |    9 ++++++---
>  xen/drivers/passthrough/x86/ats.c   |    2 +-
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index a8cab59..5f4680f 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -176,10 +176,13 @@ developers wishing Xen to fall back to older timing 
> methods on newer hardware.
>  ### ats
>  > `= <boolean>`
>  
> -> Default: `true`
> +> Default: `false`
> +
> +Permits Xen to set up and use PCI Address Translation Services.  This is a
> +performance optimisation for PCI Passthrough.
>  
> -Permits Xen to set up and use PCI Address Translation Services, which
> -is required for PCI Passthrough.
> +**WARNING: Xen cannot currently safely use ATS because of its synchronous 
> wait
> +loops for Queued Invalidation completions.**
>  
>  ### availmem
>  > `= <size>`
> diff --git a/xen/drivers/passthrough/x86/ats.c 
> b/xen/drivers/passthrough/x86/ats.c
> index 1e3e03a..436eada 100644
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -20,7 +20,7 @@
>  
>  LIST_HEAD(ats_devices);
>  
> -bool_t __read_mostly ats_enabled = 1;
> +bool_t __read_mostly ats_enabled = 0;
>  boolean_param("ats", ats_enabled);
>  
>  int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
> -- 
> 1.7.10.4

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Zhang, Yang Z]

Jan Beulich wrote on 2014-08-22:
>>>> On 20.08.14 at 18:01, <andrew.cooper3@citrix.com> wrote:
>> Xen cannot safely use any ATS functionality until it gains
>> asynchronous queued invalidation support, because of the current
>> synchronous wait for completion.
>> 
>> Do not turn ATS on by default.
>> 
>> While editing the default in the command line documentation, correct
>> the statement regarding PCI Passthrough.  ATS is purely a
>> performance optimisation, and is certainly not required for PCI Passthrough to function.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
> 
> Even if not mandated by ./MAINTAINERS I think this definitely should
> have been Cc-ed to the VT-d and AMD IOMMU maintainers (now done).

Thanks for forwarding to us.

BTW, do you want us to give a ack or just let us know?

> 
> Jan
> 
>> ---
>>  docs/misc/xen-command-line.markdown |    9 ++++++---
>>  xen/drivers/passthrough/x86/ats.c   |    2 +-
>>  2 files changed, 7 insertions(+), 4 deletions(-)
>> diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-command-line.markdown
>> index a8cab59..5f4680f 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -176,10 +176,13 @@ developers wishing Xen to fall back to older
>> timing methods on newer hardware.
>>  ### ats
>>  `= <boolean>`
>> 
>> -> Default: `true`
>> +> Default: `false`
>> +
>> +Permits Xen to set up and use PCI Address Translation Services.
>> +This is a performance optimisation for PCI Passthrough.
>> 
>> -Permits Xen to set up and use PCI Address Translation Services,
>> which -is required for PCI Passthrough.
>> +**WARNING: Xen cannot currently safely use ATS because of its
>> +synchronous
>> wait
>> +loops for Queued Invalidation completions.**
>> 
>>  ### availmem
>>  `= <size>`
>> diff --git a/xen/drivers/passthrough/x86/ats.c
>> b/xen/drivers/passthrough/x86/ats.c
>> index 1e3e03a..436eada 100644
>> --- a/xen/drivers/passthrough/x86/ats.c
>> +++ b/xen/drivers/passthrough/x86/ats.c
>> @@ -20,7 +20,7 @@
>> 
>>  LIST_HEAD(ats_devices);
>> -bool_t __read_mostly ats_enabled = 1;
>> +bool_t __read_mostly ats_enabled = 0;
>>  boolean_param("ats", ats_enabled);
>>  
>>  int enable_ats_device(int seg, int bus, int devfn, const void
>> *iommu)
>> --
>> 1.7.10.4
> 
>


Best regards,
Yang

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Jan Beulich]

>>> On 27.08.14 at 02:37, <yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2014-08-22:
>>>>> On 20.08.14 at 18:01, <andrew.cooper3@citrix.com> wrote:
>>> Xen cannot safely use any ATS functionality until it gains
>>> asynchronous queued invalidation support, because of the current
>>> synchronous wait for completion.
>>> 
>>> Do not turn ATS on by default.
>>> 
>>> While editing the default in the command line documentation, correct
>>> the statement regarding PCI Passthrough.  ATS is purely a
>>> performance optimisation, and is certainly not required for PCI Passthrough 
> to function.
>>> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Jan Beulich <JBeulich@suse.com>
>> 
>> Even if not mandated by ./MAINTAINERS I think this definitely should
>> have been Cc-ed to the VT-d and AMD IOMMU maintainers (now done).
> 
> Thanks for forwarding to us.
> 
> BTW, do you want us to give a ack or just let us know?

If you don't object to the change, an ack would certainly be
appreciated. If you object, comments as to alternatives would
be kind of expected.

Jan

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Suravee Suthikulpanit]

Andrew/Jan,

I have verified that PCI device pass-through works with ATS disabled. 
Although, could you please help described how "asynchronous queued 
invalidation support" supposed to work?

Thanks,

Suravee

On 08/22/2014 07:39 AM, Jan Beulich wrote:
>>>> On 20.08.14 at 18:01, <andrew.cooper3@citrix.com> wrote:
>> Xen cannot safely use any ATS functionality until it gains asynchronous
>> queued
>> invalidation support, because of the current synchronous wait for
>> completion.
>>
>> Do not turn ATS on by default.
>>
>> While editing the default in the command line documentation, correct the
>> statement regarding PCI Passthrough.  ATS is purely a performance
>> optimisation, and is certainly not required for PCI Passthrough to function.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>
> Even if not mandated by ./MAINTAINERS I think this definitely
> should have been Cc-ed to the VT-d and AMD IOMMU maintainers
> (now done).
>
> Jan
>
>> ---
>>   docs/misc/xen-command-line.markdown |    9 ++++++---
>>   xen/drivers/passthrough/x86/ats.c   |    2 +-
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/misc/xen-command-line.markdown
>> b/docs/misc/xen-command-line.markdown
>> index a8cab59..5f4680f 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -176,10 +176,13 @@ developers wishing Xen to fall back to older timing
>> methods on newer hardware.
>>   ### ats
>>   > `= <boolean>`
>>
>> -> Default: `true`
>> +> Default: `false`
>> +
>> +Permits Xen to set up and use PCI Address Translation Services.  This is a
>> +performance optimisation for PCI Passthrough.
>>
>> -Permits Xen to set up and use PCI Address Translation Services, which
>> -is required for PCI Passthrough.
>> +**WARNING: Xen cannot currently safely use ATS because of its synchronous
>> wait
>> +loops for Queued Invalidation completions.**
>>
>>   ### availmem
>>   > `= <size>`
>> diff --git a/xen/drivers/passthrough/x86/ats.c
>> b/xen/drivers/passthrough/x86/ats.c
>> index 1e3e03a..436eada 100644
>> --- a/xen/drivers/passthrough/x86/ats.c
>> +++ b/xen/drivers/passthrough/x86/ats.c
>> @@ -20,7 +20,7 @@
>>
>>   LIST_HEAD(ats_devices);
>>
>> -bool_t __read_mostly ats_enabled = 1;
>> +bool_t __read_mostly ats_enabled = 0;
>>   boolean_param("ats", ats_enabled);
>>
>>   int enable_ats_device(int seg, int bus, int devfn, const void *iommu)
>> --
>> 1.7.10.4
>
>
>

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Jan Beulich]

>>> On 28.08.14 at 09:08, <suravee.suthikulpanit@amd.com> wrote:
> I have verified that PCI device pass-through works with ATS disabled. 
> Although, could you please help described how "asynchronous queued 
> invalidation support" supposed to work?

Are asking about the abstract model, or the specific implementation?
The former is quite obvious is think (utilize the respective interrupt to
get notified of completions and suspend the execution in the current
context until then), while the latter isn't clear at all at this point, due
to it being necessary to inspect all involved code paths to see how
exactly suspension can be carried out.

Also, QI is - afaik - a VT-d specific feature anyway, i.e. doesn't have
a direct equivalent on the AMD side...

Jan

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Suravee Suthikulpanit]

On 08/28/2014 02:32 AM, Jan Beulich wrote:
>>>> On 28.08.14 at 09:08, <suravee.suthikulpanit@amd.com> wrote:
>> I have verified that PCI device pass-through works with ATS disabled.
>> Although, could you please help described how "asynchronous queued
>> invalidation support" supposed to work?
>
> Are asking about the abstract model, or the specific implementation?
> The former is quite obvious is think (utilize the respective interrupt to
> get notified of completions and suspend the execution in the current
> context until then),

So, what Andrew is saying is that the current implementation which uses 
the "synchronous wait for completion" is not safe? May be I'm not 
getting the whole picture here of how it is unsafe.

Thanks,

Suravee

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Jan Beulich]

>>> On 28.08.14 at 10:18, <suravee.suthikulpanit@amd.com> wrote:
> On 08/28/2014 02:32 AM, Jan Beulich wrote:
>>>>> On 28.08.14 at 09:08, <suravee.suthikulpanit@amd.com> wrote:
>>> I have verified that PCI device pass-through works with ATS disabled.
>>> Although, could you please help described how "asynchronous queued
>>> invalidation support" supposed to work?
>>
>> Are asking about the abstract model, or the specific implementation?
>> The former is quite obvious is think (utilize the respective interrupt to
>> get notified of completions and suspend the execution in the current
>> context until then),
> 
> So, what Andrew is saying is that the current implementation which uses 
> the "synchronous wait for completion" is not safe? May be I'm not 
> getting the whole picture here of how it is unsafe.

It may be spinning for up to a second, and for ATS it would really
need to be spinning for up to 60 seconds (perhaps even 90,
depending on how to interpret the spec).

Jan

Re: [PATCH] x86/ats: Disable Address Translation Services by default

[Suravee Suthikulpanit]

On 08/28/2014 03:30 AM, Jan Beulich wrote:
>>>> On 28.08.14 at 10:18, <suravee.suthikulpanit@amd.com> wrote:
>> On 08/28/2014 02:32 AM, Jan Beulich wrote:
>>>>>> On 28.08.14 at 09:08, <suravee.suthikulpanit@amd.com> wrote:
>>>> I have verified that PCI device pass-through works with ATS disabled.
>>>> Although, could you please help described how "asynchronous queued
>>>> invalidation support" supposed to work?
>>>
>>> Are asking about the abstract model, or the specific implementation?
>>> The former is quite obvious is think (utilize the respective interrupt to
>>> get notified of completions and suspend the execution in the current
>>> context until then),
>>
>> So, what Andrew is saying is that the current implementation which uses
>> the "synchronous wait for completion" is not safe? May be I'm not
>> getting the whole picture here of how it is unsafe.
>
> It may be spinning for up to a second, and for ATS it would really
> need to be spinning for up to 60 seconds (perhaps even 90,
> depending on how to interpret the spec).
>
> Jan
>
Thanks for clarification.

Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>