Re: [Xen-devel] [PATCH 1/5] IOMMU: iommu_intremap is x86-only

[Jan Beulich <jbeulich@suse.com>]

On 28.02.2020 21:16, Andrew Cooper wrote:
> On 28/02/2020 12:26, Jan Beulich wrote:
>> Provide a #define for other cases; it didn't seem worthwhile to me to
>> introduce an IOMMU_INTREMAP Kconfig option at this point.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1299,6 +1299,8 @@ boolean (e.g. `iommu=no`) can override t
>>      generation of IOMMUs only supported DMA remapping, and Interrupt Remapping
>>      appeared in the second generation.
>>  
>> +    This option is not valid on Arm.
> 
> The longevity of this comment would be greater if it were phrased as "is
> only valid on x86", especially given the RFC RISCV series on list.

How do we know how intremap is going to work on future ports?

>> @@ -90,8 +89,10 @@ static int __init parse_iommu_param(cons
>>              iommu_snoop = val;
>>          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
>>              iommu_qinval = val;
>> +#ifndef iommu_intremap
>>          else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
>>              iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
>> +#endif
> 
> The use of ifndef in particular makes the result very weird to read. 
> There appear to be no uses of iommu_intremap outside of x86 code, other
> than in this setup, so having it false in the !CONFIG_X86 case isn't
> helpful.
> 
> How about just guarding uses of the variable with IS_ENABLED(CONFIG_X86)
> and a common extern?  We use this DCE trick already to reduce the
> ifdefary in the code.

A common extern would mean to guard _all_ uses of the variable, also
reads. That's a lot of IS_ENABLED(CONFIG_X86) to add. Furthermore,
as said above, I'm unconvinced all future ports would be Arm-like in
this regard (historically at least ia64 wasn't).

The idea of using #ifdef like is done here is that a new port would
typically only need to adjust the conditional around the declaration/
#define to choose one of the two options. No other could would need
touching. IS_ENABLED(CONFIG_X86), otoh, would require all sites we'd
add now to be touched again when an x86-like port appears.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel