Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.11.2020 22:04, Stefano Stabellini wrote:
> On Wed, 4 Nov 2020, Jan Beulich wrote:
>> On 04.11.2020 16:43, Jan Beulich wrote:
>>> On 03.11.2020 16:59, Rahul Singh wrote:
>>>> --- a/xen/drivers/pci/Kconfig
>>>> +++ b/xen/drivers/pci/Kconfig
>>>> @@ -1,3 +1,12 @@
>>>>  
>>>>  config HAS_PCI
>>>>    bool
>>>> +
>>>> +config PCI_ATS
>>>> +  bool "PCI ATS support"
>>>> +  default y
>>>> +  depends on X86 && HAS_PCI
>>>> +  ---help---
>>>> +   Enable PCI Address Translation Services.
>>>> +
>>>> +   If unsure, say Y.
>>>
>>> Support for "---help---" having gone away in Linux, I think we'd
>>> better not add new instances. Also indentation of help content
>>> typically is by a tab and two spaces. With these two adjusted
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Initially I wanted to merely reply indicating I'd be fine making
>> these changes while committing, but there are two more things
>> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
>> field now unused when !PCI_ATS? If so, if should get an #ifdef
>> added. And then, what exactly is it in ats.c that's x86-specific?
>> Shouldn't the whole file instead be moved one level up, and be
>> usable by Arm right away?
> 
> If the issue is that ATS wouldn't work on ARM straight away, then I
> think it would be best to make this a silent option like we did in patch
> #1: if x86 && HAS_PCI -> automatically enable, otherwise disable.

Taking the opportunity to make this a non-silent option was actually
a request of mine. As long as the code builds and isn't obviously
broken for Arm, I think it shouldn't have an X86 dependency (and it
then should be moved up in the tree). Arguably it could then
default to off for Arm, but when asking for this option to gain a
prompt I also indicated that I wonder whether the default shouldn't
be off on x86 as well, seeing that the controlling command line
option also defaults to off.

Jan