[Xen-devel] [PATCH 0/5] IOMMU: restrict visibility/scope if certain variables

[Xen-devel] [PATCH 0/5] IOMMU: restrict visibility/scope if certain variables

[Jan Beulich]

A number of the command line controlled variables are x86-
or even x86-HVM-specific. Don't have those variables elsewhere
in the first place (in some cases replace them by a #define),
and as a result also don't silently accept such "iommu="
sub-options which in fact have no effect.

1: iommu_intremap is x86-only
2: iommu_intpost is x86/HVM-only
3: iommu_igfx is x86-only
4: iommu_qinval is x86-only
5: iommu_snoop is x86/HVM-only

The series contextually depends on "AMD/IOMMU: without XT,
x2APIC needs to be forced into physical mode"

Jan

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

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

[Jan Beulich]

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 `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


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

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

[Andrew Cooper]

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

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

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Jan Beulich]

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

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Xen-devel] [PATCH 2/5] IOMMU: iommu_intpost is x86/HVM-only

[Jan Beulich]

Provide a #define for all other cases.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1309,6 +1309,9 @@ boolean (e.g. `iommu=no`) can override t
     This option depends on `intremap`, and is disabled by default due to some
     corner cases in the implementation which have yet to be resolved.
 
+    This option is not valid on Arm, or on x86 builds of Xen without HVM
+    support.
+
 *   The `crash-disable` boolean controls disabling IOMMU functionality (DMAR/IR/QI)
     before switching to a crash kernel. This option is inactive by default and
     is for compatibility with older kdump kernels only. Modern kernels copy
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -43,14 +43,6 @@ bool __read_mostly iommu_hwdom_passthrou
 bool __hwdom_initdata iommu_hwdom_inclusive;
 int8_t __hwdom_initdata iommu_hwdom_reserved = -1;
 
-/*
- * In the current implementation of VT-d posted interrupts, in some extreme
- * cases, the per cpu list which saves the blocked vCPU will be very long,
- * and this will affect the interrupt latency, so let this feature off by
- * default until we find a good solution to resolve it.
- */
-bool_t __read_mostly iommu_intpost;
-
 #ifndef iommu_hap_pt_share
 bool __read_mostly iommu_hap_pt_share = true;
 #endif
@@ -93,8 +85,10 @@ static int __init parse_iommu_param(cons
         else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
             iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
 #endif
+#ifndef iommu_intpost
         else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
             iommu_intpost = val;
+#endif
 #ifdef CONFIG_KEXEC
         else if ( (val = parse_boolean("crash-disable", s, ss)) >= 0 )
             iommu_crash_disable = val;
@@ -486,8 +480,10 @@ int __init iommu_setup(void)
         panic("Couldn't enable %s and iommu=required/force\n",
               !iommu_enabled ? "IOMMU" : "Interrupt Remapping");
 
+#ifndef iommu_intpost
     if ( !iommu_intremap )
         iommu_intpost = 0;
+#endif
 
     printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
     if ( !iommu_enabled )
@@ -563,10 +559,13 @@ void iommu_crash_shutdown(void)
 
     if ( iommu_enabled )
         iommu_get_ops()->crash_shutdown();
-    iommu_enabled = iommu_intpost = 0;
+    iommu_enabled = false;
 #ifndef iommu_intremap
     iommu_intremap = iommu_intremap_off;
 #endif
+#ifndef iommu_intpost
+    iommu_intpost = false;
+#endif
 }
 
 int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2297,13 +2297,15 @@ static int __init vtd_setup(void)
         if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
             iommu_intremap = iommu_intremap_off;
 
+#ifndef iommu_intpost
         /*
          * We cannot use posted interrupt if X86_FEATURE_CX16 is
          * not supported, since we count on this feature to
          * atomically update 16-byte IRTE in posted format.
          */
         if ( !cap_intr_post(iommu->cap) || !iommu_intremap || !cpu_has_cx16 )
-            iommu_intpost = 0;
+            iommu_intpost = false;
+#endif
 
         if ( !vtd_ept_page_compatible(iommu) )
             clear_iommu_hap_pt_share();
@@ -2330,7 +2332,9 @@ static int __init vtd_setup(void)
     P(iommu_hwdom_passthrough, "Dom0 DMA Passthrough");
     P(iommu_qinval, "Queued Invalidation");
     P(iommu_intremap, "Interrupt Remapping");
+#ifndef iommu_intpost
     P(iommu_intpost, "Posted Interrupt");
+#endif
     P(iommu_hap_pt_share, "Shared EPT tables");
 #undef P
 
@@ -2348,7 +2352,9 @@ static int __init vtd_setup(void)
     iommu_hwdom_passthrough = false;
     iommu_qinval = 0;
     iommu_intremap = iommu_intremap_off;
-    iommu_intpost = 0;
+#ifndef iommu_intpost
+    iommu_intpost = false;
+#endif
     return ret;
 }
 
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -29,6 +29,16 @@ struct iommu_ops __read_mostly iommu_ops
 
 enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
 
+#ifndef iommu_intpost
+/*
+ * In the current implementation of VT-d posted interrupts, in some extreme
+ * cases, the per cpu list which saves the blocked vCPU will be very long,
+ * and this will affect the interrupt latency, so let this feature off by
+ * default until we find a good solution to resolve it.
+ */
+bool __read_mostly iommu_intpost;
+#endif
+
 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
@@ -54,7 +54,7 @@ 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;
+extern bool_t iommu_snoop, iommu_qinval;
 
 #ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
@@ -70,6 +70,12 @@ extern enum __packed iommu_intremap {
 # define iommu_intremap false
 #endif
 
+#if defined(CONFIG_X86) && defined(CONFIG_HVM)
+extern bool iommu_intpost;
+#else
+# define iommu_intpost false
+#endif
+
 #if defined(CONFIG_IOMMU_FORCE_PT_SHARE)
 #define iommu_hap_pt_share true
 #elif defined(CONFIG_HVM)


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

[Xen-devel] [PATCH 3/5] IOMMU: iommu_igfx is x86-only

[Jan Beulich]

In fact it's VT-d specific, but we don't have a way yet to build code
for just one vendor.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -32,7 +32,6 @@ bool_t __read_mostly iommu_enabled;
 bool_t __read_mostly force_iommu;
 bool_t __read_mostly iommu_verbose;
 bool __read_mostly iommu_quarantine = true;
-bool_t __read_mostly iommu_igfx = 1;
 bool_t __read_mostly iommu_snoop = 1;
 bool_t __read_mostly iommu_qinval = 1;
 bool_t __read_mostly iommu_crash_disable;
@@ -73,8 +72,10 @@ static int __init parse_iommu_param(cons
             force_iommu = val;
         else if ( (val = parse_boolean("quarantine", s, ss)) >= 0 )
             iommu_quarantine = val;
+#ifdef CONFIG_X86
         else if ( (val = parse_boolean("igfx", s, ss)) >= 0 )
             iommu_igfx = val;
+#endif
         else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
             iommu_verbose = val;
         else if ( (val = parse_boolean("snoop", s, ss)) >= 0 )
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -50,6 +50,8 @@ struct mapped_rmrr {
 /* Possible unfiltered LAPIC/MSI messages from untrusted sources? */
 bool __read_mostly untrusted_msi;
 
+bool __read_mostly iommu_igfx = true;
+
 int nr_iommus;
 
 static struct tasklet vtd_fault_tasklet;
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -53,7 +53,7 @@ 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 force_iommu, iommu_quarantine, iommu_verbose;
 extern bool_t iommu_snoop, iommu_qinval;
 
 #ifdef CONFIG_X86
@@ -66,6 +66,7 @@ extern enum __packed iommu_intremap {
    iommu_intremap_restricted,
    iommu_intremap_full,
 } iommu_intremap;
+extern bool iommu_igfx;
 #else
 # define iommu_intremap false
 #endif


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

[Xen-devel] [PATCH 4/5] IOMMU: iommu_qinval is x86-only

[Jan Beulich]

In fact it's VT-d specific, but we don't have a way yet to build code
for just one vendor.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -33,7 +33,6 @@ bool_t __read_mostly force_iommu;
 bool_t __read_mostly iommu_verbose;
 bool __read_mostly iommu_quarantine = true;
 bool_t __read_mostly iommu_snoop = 1;
-bool_t __read_mostly iommu_qinval = 1;
 bool_t __read_mostly iommu_crash_disable;
 
 static bool __hwdom_initdata iommu_hwdom_none;
@@ -75,13 +74,13 @@ static int __init parse_iommu_param(cons
 #ifdef CONFIG_X86
         else if ( (val = parse_boolean("igfx", s, ss)) >= 0 )
             iommu_igfx = val;
+        else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
+            iommu_qinval = val;
 #endif
         else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
             iommu_verbose = val;
         else if ( (val = parse_boolean("snoop", s, ss)) >= 0 )
             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;
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -51,6 +51,7 @@ struct mapped_rmrr {
 bool __read_mostly untrusted_msi;
 
 bool __read_mostly iommu_igfx = true;
+bool __read_mostly iommu_qinval = true;
 
 int nr_iommus;
 
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -54,7 +54,7 @@ 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;
-extern bool_t iommu_snoop, iommu_qinval;
+extern bool_t iommu_snoop;
 
 #ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
@@ -66,7 +66,7 @@ extern enum __packed iommu_intremap {
    iommu_intremap_restricted,
    iommu_intremap_full,
 } iommu_intremap;
-extern bool iommu_igfx;
+extern bool iommu_igfx, iommu_qinval;
 #else
 # define iommu_intremap false
 #endif


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

[Xen-devel] [PATCH 5/5] IOMMU: iommu_snoop is x86/HVM-only

[Jan Beulich]

In fact it's VT-d specific, but we don't have a way yet to build code
for just one vendor. Provide a #define for all other cases.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -32,7 +32,6 @@ bool_t __read_mostly iommu_enabled;
 bool_t __read_mostly force_iommu;
 bool_t __read_mostly iommu_verbose;
 bool __read_mostly iommu_quarantine = true;
-bool_t __read_mostly iommu_snoop = 1;
 bool_t __read_mostly iommu_crash_disable;
 
 static bool __hwdom_initdata iommu_hwdom_none;
@@ -79,8 +78,10 @@ static int __init parse_iommu_param(cons
 #endif
         else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
             iommu_verbose = val;
+#ifndef iommu_snoop
         else if ( (val = parse_boolean("snoop", s, ss)) >= 0 )
             iommu_snoop = val;
+#endif
 #ifndef iommu_intremap
         else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
             iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
@@ -488,7 +489,9 @@ int __init iommu_setup(void)
     printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
     if ( !iommu_enabled )
     {
-        iommu_snoop = 0;
+#ifndef iommu_snoop
+        iommu_snoop = false;
+#endif
         iommu_hwdom_passthrough = false;
         iommu_hwdom_strict = false;
     }
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -52,6 +52,9 @@ bool __read_mostly untrusted_msi;
 
 bool __read_mostly iommu_igfx = true;
 bool __read_mostly iommu_qinval = true;
+#ifndef iommu_snoop
+bool __read_mostly iommu_snoop = true;
+#endif
 
 int nr_iommus;
 
@@ -2288,8 +2291,10 @@ static int __init vtd_setup(void)
                cap_sps_2mb(iommu->cap) ? ", 2MB" : "",
                cap_sps_1gb(iommu->cap) ? ", 1GB" : "");
 
+#ifndef iommu_snoop
         if ( iommu_snoop && !ecap_snp_ctl(iommu->ecap) )
-            iommu_snoop = 0;
+            iommu_snoop = false;
+#endif
 
         if ( iommu_hwdom_passthrough && !ecap_pass_thru(iommu->ecap) )
             iommu_hwdom_passthrough = false;
@@ -2331,7 +2336,9 @@ static int __init vtd_setup(void)
     }
 
 #define P(p,s) printk("Intel VT-d %s %senabled.\n", s, (p)? "" : "not ")
+#ifndef iommu_snoop
     P(iommu_snoop, "Snoop Control");
+#endif
     P(iommu_hwdom_passthrough, "Dom0 DMA Passthrough");
     P(iommu_qinval, "Queued Invalidation");
     P(iommu_intremap, "Interrupt Remapping");
@@ -2351,7 +2358,9 @@ static int __init vtd_setup(void)
 
  error:
     iommu_enabled = 0;
-    iommu_snoop = 0;
+#ifndef iommu_snoop
+    iommu_snoop = false;
+#endif
     iommu_hwdom_passthrough = false;
     iommu_qinval = 0;
     iommu_intremap = iommu_intremap_off;
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -54,7 +54,6 @@ 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;
-extern bool_t iommu_snoop;
 
 #ifdef CONFIG_X86
 extern enum __packed iommu_intremap {
@@ -72,9 +71,10 @@ extern bool iommu_igfx, iommu_qinval;
 #endif
 
 #if defined(CONFIG_X86) && defined(CONFIG_HVM)
-extern bool iommu_intpost;
+extern bool iommu_intpost, iommu_snoop;
 #else
 # define iommu_intpost false
+# define iommu_snoop false
 #endif
 
 #if defined(CONFIG_IOMMU_FORCE_PT_SHARE)


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

Re: [Xen-devel] [PATCH 5/5] IOMMU: iommu_snoop is x86/HVM-only

[Andrew Cooper]

On 28/02/2020 12:27, Jan Beulich wrote:
> In fact it's VT-d specific, but we don't have a way yet to build code
> for just one vendor. Provide a #define for all other cases.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

iommu_snoop has no specific interaction with HVM.

It is for any cacheability games the hypervisor may play on a VM, and
that in principle includes PV guests as well.

~Andrew

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

Re: [Xen-devel] [PATCH 5/5] IOMMU: iommu_snoop is x86/HVM-only

[Jan Beulich]

On 28.02.2020 21:20, Andrew Cooper wrote:
> On 28/02/2020 12:27, Jan Beulich wrote:
>> In fact it's VT-d specific, but we don't have a way yet to build code
>> for just one vendor. Provide a #define for all other cases.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> iommu_snoop has no specific interaction with HVM.
> 
> It is for any cacheability games the hypervisor may play on a VM, and
> that in principle includes PV guests as well.

Oh, in the grep results I overlooked

    /* Set the SNP on leaf page table if Snoop Control available */
    if ( iommu_snoop )
        dma_set_pte_snp(new);

Thanks for pointing out, will correct.

Jan

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

Re: [Xen-devel] [PATCH 0/5] IOMMU: restrict visibility/scope if certain variables

[Julien Grall]

Hi Jan,

Adding Paul in CC as he now co-maintain the IOMMU code.


On 28/02/2020 12:24, Jan Beulich wrote:
> A number of the command line controlled variables are x86-
> or even x86-HVM-specific. Don't have those variables elsewhere
> in the first place (in some cases replace them by a #define),
> and as a result also don't silently accept such "iommu="
> sub-options which in fact have no effect.

I can confirm that all the parameters listed below are not used on Arm.

> 
> 1: iommu_intremap is x86-only
> 2: iommu_intpost is x86/HVM-only
> 3: iommu_igfx is x86-only
> 4: iommu_qinval is x86-only
> 5: iommu_snoop is x86/HVM-only
> 
> The series contextually depends on "AMD/IOMMU: without XT,
> x2APIC needs to be forced into physical mode"
> 
> Jan
> 

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH 0/5] IOMMU: restrict visibility/scope if certain variables

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, February 28, 2020 8:25 PM
> 
> A number of the command line controlled variables are x86-
> or even x86-HVM-specific. Don't have those variables elsewhere
> in the first place (in some cases replace them by a #define),
> and as a result also don't silently accept such "iommu="
> sub-options which in fact have no effect.
> 
> 1: iommu_intremap is x86-only
> 2: iommu_intpost is x86/HVM-only
> 3: iommu_igfx is x86-only
> 4: iommu_qinval is x86-only
> 5: iommu_snoop is x86/HVM-only
> 
> The series contextually depends on "AMD/IOMMU: without XT,
> x2APIC needs to be forced into physical mode"
> 

I'm generally OK with this series, but will give my r-b after
all other vendors have confirmed the scope here.

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