diff mbox series

[1/5] IOMMU: iommu_intremap is x86-only

Message ID 00a4c7ca-36a4-c108-719c-01a6e16df9b2@suse.com (mailing list archive)
State Superseded
Headers show
Series IOMMU: restrict visibility/scope if certain variables | expand

Commit Message

Jan Beulich Feb. 28, 2020, 12:26 p.m. UTC 
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>

Comments

Andrew Cooper Feb. 28, 2020, 8:16 p.m. UTC | #1 
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.

> +
>  *   The `intpost` boolean controls the Posted Interrupt sub-feature.  In
>      combination with APIC acceleration (VT-x APICV, SVM AVIC), the IOMMU can
>      be configured to deliver interrupts from assigned PCI devices directly
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -35,7 +35,6 @@ bool __read_mostly iommu_quarantine = tr
>  bool_t __read_mostly iommu_igfx = 1;
>  bool_t __read_mostly iommu_snoop = 1;
>  bool_t __read_mostly iommu_qinval = 1;
> -enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
>  bool_t __read_mostly iommu_crash_disable;
>  
>  static bool __hwdom_initdata iommu_hwdom_none;
> @@ -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.

The result would certainly be easier to follow

~Andrew
Jan Beulich March 2, 2020, 10:07 a.m. UTC | #2 
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
Julien Grall March 2, 2020, 10:44 a.m. UTC | #3 
On 02/03/2020 10:07, Jan Beulich wrote:
> 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?

We don't  know. But, for a reviewer, it is going to be much easier to 
notice a command line option is going to be used as the patch would 
modify a caller.

So I agree with Andrew that we want to say "only valid on x86".

Cheers,
Jan Beulich March 2, 2020, 10:57 a.m. UTC | #4 
On 02.03.2020 11:44, Julien Grall wrote:
> 
> 
> On 02/03/2020 10:07, Jan Beulich wrote:
>> 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?
> 
> We don't  know. But, for a reviewer, it is going to be much easier to 
> notice a command line option is going to be used as the patch would 
> modify a caller.

I'm struggling with understanding this (not seeing the connection
between "command line option" and "caller"), but ...

> So I agree with Andrew that we want to say "only valid on x86".

... well, okay then (and done also for the iommu_intpost counterpart).

Jan
Julien Grall March 2, 2020, 11:25 a.m. UTC | #5 
Hi Jan,

On 02/03/2020 10:57, Jan Beulich wrote:
> On 02.03.2020 11:44, Julien Grall wrote:
>>
>>
>> On 02/03/2020 10:07, Jan Beulich wrote:
>>> 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?
>>
>> We don't  know. But, for a reviewer, it is going to be much easier to
>> notice a command line option is going to be used as the patch would
>> modify a caller.
> 
> I'm struggling with understanding this (not seeing the connection
> between "command line option" and "caller"), but ...

"caller" might have been the wrong word here. Let me expand it. The 
patch you sent contains an #ifdef CONFIG_X86 protecting the declaration 
of iommu_intremap:

+#ifdef CONFIG_X86
  extern enum __packed iommu_intremap {
     /*
      * In order to allow traditional boolean uses of the iommu_intremap
      * variable, the "off" value has to come first (yielding a value of 
zero).
      */
     iommu_intremap_off,
-#ifdef CONFIG_X86
     iommu_intremap_restricted,
-#endif
     iommu_intremap_full,
  } iommu_intremap;
+#else
+# define iommu_intremap false
+#endif

Someone wanted to use iommu_intremap (and by extent the command line 
option) in a new arch would have to modify the declaration for it to 
work. A commit message would also likely to contain "Implement the 
command line option ...". So a reviewer can spot the change and ask to 
update the documentation if this wasn't yet done.

At the inverse, if the new arch is not using iommu_intremap then there 
will be no modification in the code. Therefore, it may be more difficult 
for a reviewer to notice that the documentation needs to be updated.

Cheers,
diff mbox series

Patch

--- 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 `intpost` boolean controls the Posted Interrupt sub-feature.  In
     combination with APIC acceleration (VT-x APICV, SVM AVIC), the IOMMU can
     be configured to deliver interrupts from assigned PCI devices directly
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -35,7 +35,6 @@  bool __read_mostly iommu_quarantine = tr
 bool_t __read_mostly iommu_igfx = 1;
 bool_t __read_mostly iommu_snoop = 1;
 bool_t __read_mostly iommu_qinval = 1;
-enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 bool_t __read_mostly iommu_crash_disable;
 
 static bool __hwdom_initdata iommu_hwdom_none;
@@ -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
         else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
             iommu_intpost = val;
 #ifdef CONFIG_KEXEC
@@ -474,8 +475,11 @@  int __init iommu_setup(void)
         rc = iommu_hardware_setup();
         iommu_enabled = (rc == 0);
     }
+
+#ifndef iommu_intremap
     if ( !iommu_enabled )
         iommu_intremap = iommu_intremap_off;
+#endif
 
     if ( (force_iommu && !iommu_enabled) ||
          (force_intremap && !iommu_intremap) )
@@ -500,7 +504,9 @@  int __init iommu_setup(void)
         printk(" - Dom0 mode: %s\n",
                iommu_hwdom_passthrough ? "Passthrough" :
                iommu_hwdom_strict ? "Strict" : "Relaxed");
+#ifndef iommu_intremap
         printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis");
+#endif
         tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, NULL);
     }
 
@@ -558,7 +564,9 @@  void iommu_crash_shutdown(void)
     if ( iommu_enabled )
         iommu_get_ops()->crash_shutdown();
     iommu_enabled = iommu_intpost = 0;
+#ifndef iommu_intremap
     iommu_intremap = iommu_intremap_off;
+#endif
 }
 
 int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -27,6 +27,8 @@ 
 const struct iommu_init_ops *__initdata iommu_init_ops;
 struct iommu_ops __read_mostly iommu_ops;
 
+enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
+
 int __init iommu_hardware_setup(void)
 {
     struct IO_APIC_route_entry **ioapic_entries = NULL;
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -55,17 +55,20 @@  static inline bool_t dfn_eq(dfn_t x, dfn
 extern bool_t iommu_enable, iommu_enabled;
 extern bool force_iommu, iommu_quarantine, iommu_verbose, iommu_igfx;
 extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
+
+#ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
    /*
     * In order to allow traditional boolean uses of the iommu_intremap
     * variable, the "off" value has to come first (yielding a value of zero).
     */
    iommu_intremap_off,
-#ifdef CONFIG_X86
    iommu_intremap_restricted,
-#endif
    iommu_intremap_full,
 } iommu_intremap;
+#else
+# define iommu_intremap false
+#endif
 
 #if defined(CONFIG_IOMMU_FORCE_PT_SHARE)
 #define iommu_hap_pt_share true