[Xen-devel] [PATCH v2 0/2] AMD/IOMMU: improve x2APIC handling when XT is unavailable

[Xen-devel] [PATCH v2 0/2] AMD/IOMMU: improve x2APIC handling when XT is unavailable

[Jan Beulich]

For AMD the connection between IOMMU and x2APIC is less strong,
hence even if unlikely to occur we would better deal correctly
with XT being unavailable (for whatever reasons) in particular
when x2APIC is already enabled on a system.

1: correct handling when XT's prereq features are unavailable
2: 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 v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

We should neither cause IOMMU initialization as a whole to fail in this
case (we should still be able to bring up the system in non-x2APIC or
x2APIC physical mode), nor should the remainder of the function be
skipped (as the main part of it won't get entered a 2nd time) in such an
event. It is merely necessary for the function to indicate to the caller
(iov_supports_xt()) that setup failed as far as x2APIC is concerned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Also adjust the loop consuming "xt". no_xt -> has_xt.

--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1364,6 +1364,7 @@ static int __init amd_iommu_prepare_one(
 int __init amd_iommu_prepare(bool xt)
 {
     struct amd_iommu *iommu;
+    bool has_xt = true;
     int rc = -ENODEV;
 
     BUG_ON( !iommu_found() );
@@ -1400,17 +1401,16 @@ int __init amd_iommu_prepare(bool xt)
         if ( rc )
             goto error_out;
 
-        rc = -ENODEV;
-        if ( xt && (!iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup) )
-            goto error_out;
+        if ( !iommu->features.flds.ga_sup || !iommu->features.flds.xt_sup )
+            has_xt = false;
     }
 
     for_each_amd_iommu ( iommu )
     {
         /* NB: There's no need to actually write these out right here. */
-        iommu->ctrl.ga_en |= xt;
-        iommu->ctrl.xt_en = xt;
-        iommu->ctrl.int_cap_xt_en = xt;
+        iommu->ctrl.ga_en |= xt && has_xt;
+        iommu->ctrl.xt_en = xt && has_xt;
+        iommu->ctrl.int_cap_xt_en = xt && has_xt;
     }
 
     rc = amd_iommu_update_ivrs_mapping_acpi();
@@ -1422,7 +1422,7 @@ int __init amd_iommu_prepare(bool xt)
         ivhd_type = 0;
     }
 
-    return rc;
+    return rc ?: xt && !has_xt ? -ENODEV : 0;
 }
 
 int __init amd_iommu_init(bool xt)


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

Re: [Xen-devel] [PATCH v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Andrew Cooper]

On 28/02/2020 12:10, Jan Beulich wrote:
> We should neither cause IOMMU initialization as a whole to fail in this
> case (we should still be able to bring up the system in non-x2APIC or
> x2APIC physical mode), nor should the remainder of the function be
> skipped (as the main part of it won't get entered a 2nd time) in such an
> event. It is merely necessary for the function to indicate to the caller
> (iov_supports_xt()) that setup failed as far as x2APIC is concerned.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Looks much better.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [Xen-devel] [PATCH v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Roger Pau Monné]

On Fri, Feb 28, 2020 at 01:10:59PM +0100, Jan Beulich wrote:
> We should neither cause IOMMU initialization as a whole to fail in this
> case (we should still be able to bring up the system in non-x2APIC or
> x2APIC physical mode), nor should the remainder of the function be
> skipped (as the main part of it won't get entered a 2nd time)

I'm not sure I see why it won't get entered a second time. AFAICT
init_done won't be set if amd_iommu_init fails, and hence will be
called again with xt == false in iov_detect?

> in such an
> event. It is merely necessary for the function to indicate to the caller
> (iov_supports_xt()) that setup failed as far as x2APIC is concerned.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM, but it needs to go in with 2/2 AFAICT, or else you would report
interrupt remapping enabled and x2APIC also enabled but won't handle
correctly a 32 bit destination field.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

On 28.02.2020 13:41, Roger Pau Monné wrote:
> On Fri, Feb 28, 2020 at 01:10:59PM +0100, Jan Beulich wrote:
>> We should neither cause IOMMU initialization as a whole to fail in this
>> case (we should still be able to bring up the system in non-x2APIC or
>> x2APIC physical mode), nor should the remainder of the function be
>> skipped (as the main part of it won't get entered a 2nd time)
> 
> I'm not sure I see why it won't get entered a second time. AFAICT
> init_done won't be set if amd_iommu_init fails, and hence will be
> called again with xt == false in iov_detect?
> 
>> in such an
>> event. It is merely necessary for the function to indicate to the caller
>> (iov_supports_xt()) that setup failed as far as x2APIC is concerned.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> LGTM, but it needs to go in with 2/2 AFAICT, or else you would report
> interrupt remapping enabled and x2APIC also enabled but won't handle
> correctly a 32 bit destination field.

Well, this is an improvement on its own imo, even if it doesn't
fully fix everything. The primary point here is to make IOMMU
initialization not fail altogether, when the system could be run
in xAPIC mode.

Jan

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

Re: [Xen-devel] [PATCH v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Jan Beulich]

On 28.02.2020 13:41, Roger Pau Monné wrote:
> On Fri, Feb 28, 2020 at 01:10:59PM +0100, Jan Beulich wrote:
>> We should neither cause IOMMU initialization as a whole to fail in this
>> case (we should still be able to bring up the system in non-x2APIC or
>> x2APIC physical mode), nor should the remainder of the function be
>> skipped (as the main part of it won't get entered a 2nd time)
> 
> I'm not sure I see why it won't get entered a second time. AFAICT
> init_done won't be set if amd_iommu_init fails, and hence will be
> called again with xt == false in iov_detect?

Not very far from the top of the function there is

    /* Have we been here before? */
    if ( ivrs_bdf_entries )
        return 0;

Hence me saying "main part" rather than "the function as a whole".

Jan

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

Re: [Xen-devel] [PATCH v2 1/2] AMD/IOMMU: correct handling when XT's prereq features are unavailable

[Roger Pau Monné]

On Fri, Feb 28, 2020 at 02:13:45PM +0100, Jan Beulich wrote:
> On 28.02.2020 13:41, Roger Pau Monné wrote:
> > On Fri, Feb 28, 2020 at 01:10:59PM +0100, Jan Beulich wrote:
> >> We should neither cause IOMMU initialization as a whole to fail in this
> >> case (we should still be able to bring up the system in non-x2APIC or
> >> x2APIC physical mode), nor should the remainder of the function be
> >> skipped (as the main part of it won't get entered a 2nd time)
> > 
> > I'm not sure I see why it won't get entered a second time. AFAICT
> > init_done won't be set if amd_iommu_init fails, and hence will be
> > called again with xt == false in iov_detect?
> 
> Not very far from the top of the function there is
> 
>     /* Have we been here before? */
>     if ( ivrs_bdf_entries )
>         return 0;
> 
> Hence me saying "main part" rather than "the function as a whole".

Oh, yes, sorry.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

[Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

The wider cluster mode APIC IDs aren't generally representable. Convert
the iommu_intremap variable into a tristate, allowing the AMD IOMMU
driver to signal this special restriction to the apic_x2apic_probe().
(Note: assignments to the variable get adjusted, while existing
consumers - all assuming a boolean property - are left alone.)

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

--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
         x2apic_phys = !iommu_intremap ||
                       (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
     }
-    else if ( !x2apic_phys && !iommu_intremap )
-    {
-        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
-               "x2APIC: forcing phys mode\n");
-        x2apic_phys = true;
-    }
+    else if ( !x2apic_phys )
+        switch ( iommu_intremap )
+        {
+        case iommu_intremap_off:
+        case iommu_intremap_restricted:
+            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
+                   "x2APIC: forcing phys mode\n",
+                   iommu_intremap == iommu_intremap_off ? "without"
+                                                        : "with restricted");
+            x2apic_phys = true;
+            break;
+
+        case iommu_intremap_full:
+            break;
+        }
 
     if ( x2apic_phys )
         return &apic_x2apic_phys;
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1139,7 +1139,7 @@ static void __init amd_iommu_init_cleanu
 
     iommu_enabled = 0;
     iommu_hwdom_passthrough = false;
-    iommu_intremap = 0;
+    iommu_intremap = iommu_intremap_off;
     iommuv2_enabled = 0;
 }
 
@@ -1413,6 +1413,9 @@ int __init amd_iommu_prepare(bool xt)
         iommu->ctrl.int_cap_xt_en = xt && has_xt;
     }
 
+    if ( iommu_intremap && !has_xt )
+        iommu_intremap = iommu_intremap_restricted;
+
     rc = amd_iommu_update_ivrs_mapping_acpi();
 
  error_out:
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -157,7 +157,7 @@ int __init acpi_ivrs_init(void)
 
     if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
     {
-        iommu_intremap = 0;
+        iommu_intremap = iommu_intremap_off;
         return -ENODEV;
     }
 
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -35,7 +35,7 @@ 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;
-bool_t __read_mostly iommu_intremap = 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;
@@ -91,7 +91,7 @@ static int __init parse_iommu_param(cons
         else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
             iommu_qinval = val;
         else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
-            iommu_intremap = val;
+            iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
         else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
             iommu_intpost = val;
 #ifdef CONFIG_KEXEC
@@ -475,7 +475,7 @@ int __init iommu_setup(void)
         iommu_enabled = (rc == 0);
     }
     if ( !iommu_enabled )
-        iommu_intremap = 0;
+        iommu_intremap = iommu_intremap_off;
 
     if ( (force_iommu && !iommu_enabled) ||
          (force_intremap && !iommu_intremap) )
@@ -557,7 +557,8 @@ void iommu_crash_shutdown(void)
 
     if ( iommu_enabled )
         iommu_get_ops()->crash_shutdown();
-    iommu_enabled = iommu_intremap = iommu_intpost = 0;
+    iommu_enabled = iommu_intpost = 0;
+    iommu_intremap = iommu_intremap_off;
 }
 
 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
@@ -2177,7 +2177,7 @@ static int __must_check init_vtd_hw(void
         {
             if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
             {
-                iommu_intremap = 0;
+                iommu_intremap = iommu_intremap_off;
                 dprintk(XENLOG_ERR VTDPREFIX,
                     "ioapic_to_iommu: ioapic %#x (id: %#x) is NULL! "
                     "Will not try to enable Interrupt Remapping.\n",
@@ -2193,7 +2193,7 @@ static int __must_check init_vtd_hw(void
             iommu = drhd->iommu;
             if ( enable_intremap(iommu, 0) != 0 )
             {
-                iommu_intremap = 0;
+                iommu_intremap = iommu_intremap_off;
                 dprintk(XENLOG_WARNING VTDPREFIX,
                         "Interrupt Remapping not enabled\n");
 
@@ -2295,7 +2295,7 @@ static int __init vtd_setup(void)
             iommu_qinval = 0;
 
         if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
-            iommu_intremap = 0;
+            iommu_intremap = iommu_intremap_off;
 
         /*
          * We cannot use posted interrupt if X86_FEATURE_CX16 is
@@ -2320,7 +2320,7 @@ static int __init vtd_setup(void)
 
     if ( !iommu_qinval && iommu_intremap )
     {
-        iommu_intremap = 0;
+        iommu_intremap = iommu_intremap_off;
         dprintk(XENLOG_WARNING VTDPREFIX, "Interrupt Remapping disabled "
             "since Queued Invalidation isn't supported or enabled.\n");
     }
@@ -2347,7 +2347,7 @@ static int __init vtd_setup(void)
     iommu_snoop = 0;
     iommu_hwdom_passthrough = false;
     iommu_qinval = 0;
-    iommu_intremap = 0;
+    iommu_intremap = iommu_intremap_off;
     iommu_intpost = 0;
     return ret;
 }
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -54,7 +54,18 @@ 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_intremap, iommu_intpost;
+extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
+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;
 
 #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 v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Roger Pau Monné]

On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
> The wider cluster mode APIC IDs aren't generally representable. Convert
> the iommu_intremap variable into a tristate, allowing the AMD IOMMU
> driver to signal this special restriction to the apic_x2apic_probe().
> (Note: assignments to the variable get adjusted, while existing
> consumers - all assuming a boolean property - are left alone.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
> 
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>          x2apic_phys = !iommu_intremap ||
>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>      }
> -    else if ( !x2apic_phys && !iommu_intremap )
> -    {
> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
> -               "x2APIC: forcing phys mode\n");
> -        x2apic_phys = true;
> -    }
> +    else if ( !x2apic_phys )
> +        switch ( iommu_intremap )
> +        {
> +        case iommu_intremap_off:
> +        case iommu_intremap_restricted:
> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
> +                   "x2APIC: forcing phys mode\n",
> +                   iommu_intremap == iommu_intremap_off ? "without"
> +                                                        : "with restricted");
> +            x2apic_phys = true;

I think you also need to fixup the usage of iommu_intremap in __cpu_up
so that CPUs with APIC IDs > 255 are not brought up when in
iommu_intremap_restricted mode.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

On 28.02.2020 13:31, Roger Pau Monné wrote:
> On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>>          x2apic_phys = !iommu_intremap ||
>>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>      }
>> -    else if ( !x2apic_phys && !iommu_intremap )
>> -    {
>> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
>> -               "x2APIC: forcing phys mode\n");
>> -        x2apic_phys = true;
>> -    }
>> +    else if ( !x2apic_phys )
>> +        switch ( iommu_intremap )
>> +        {
>> +        case iommu_intremap_off:
>> +        case iommu_intremap_restricted:
>> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
>> +                   "x2APIC: forcing phys mode\n",
>> +                   iommu_intremap == iommu_intremap_off ? "without"
>> +                                                        : "with restricted");
>> +            x2apic_phys = true;
> 
> I think you also need to fixup the usage of iommu_intremap in __cpu_up
> so that CPUs with APIC IDs > 255 are not brought up when in
> iommu_intremap_restricted mode.

That certainly wants changing, yes, but I view this as an orthogonal
adjustment, which I'd like to make only once I understand what the
behavior for APIC ID 0xff should be in this setup.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Roger Pau Monné]

On Fri, Feb 28, 2020 at 01:39:59PM +0100, Jan Beulich wrote:
> On 28.02.2020 13:31, Roger Pau Monné wrote:
> > On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/genapic/x2apic.c
> >> +++ b/xen/arch/x86/genapic/x2apic.c
> >> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
> >>          x2apic_phys = !iommu_intremap ||
> >>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >>      }
> >> -    else if ( !x2apic_phys && !iommu_intremap )
> >> -    {
> >> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
> >> -               "x2APIC: forcing phys mode\n");
> >> -        x2apic_phys = true;
> >> -    }
> >> +    else if ( !x2apic_phys )
> >> +        switch ( iommu_intremap )
> >> +        {
> >> +        case iommu_intremap_off:
> >> +        case iommu_intremap_restricted:
> >> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
> >> +                   "x2APIC: forcing phys mode\n",
> >> +                   iommu_intremap == iommu_intremap_off ? "without"
> >> +                                                        : "with restricted");
> >> +            x2apic_phys = true;
> > 
> > I think you also need to fixup the usage of iommu_intremap in __cpu_up
> > so that CPUs with APIC IDs > 255 are not brought up when in
> > iommu_intremap_restricted mode.
> 
> That certainly wants changing, yes, but I view this as an orthogonal
> adjustment, which I'd like to make only once I understand what the
> behavior for APIC ID 0xff should be in this setup.

I would say APIC ID 0xff should be the broadcast ID, or else remapped
interrupts won't be able to use a broadcast destination? I'm however
not able to find any mention to this in the AMD-Vi spec.

So the check in __cpu_up should be adjusted to iommu_intremap !=
iommu_intremap_full I think, or else you won't be able to address the
CPUs brought up from the interrupt remapping tables.

Anyway, the change looks fine, so:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

On 28.02.2020 13:31, Roger Pau Monné wrote:
> On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>>          x2apic_phys = !iommu_intremap ||
>>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>      }
>> -    else if ( !x2apic_phys && !iommu_intremap )
>> -    {
>> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
>> -               "x2APIC: forcing phys mode\n");
>> -        x2apic_phys = true;
>> -    }
>> +    else if ( !x2apic_phys )
>> +        switch ( iommu_intremap )
>> +        {
>> +        case iommu_intremap_off:
>> +        case iommu_intremap_restricted:
>> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
>> +                   "x2APIC: forcing phys mode\n",
>> +                   iommu_intremap == iommu_intremap_off ? "without"
>> +                                                        : "with restricted");
>> +            x2apic_phys = true;
> 
> I think you also need to fixup the usage of iommu_intremap in __cpu_up
> so that CPUs with APIC IDs > 255 are not brought up when in
> iommu_intremap_restricted mode.

I've looked around some (finding indications that, as one would
suspect, broadcasting is also supported for IO-APIC based
interrupts, and then by implication also for MSI) and then also
thought about this some more. I think the corner case here
resolves like this: Whether 0xff means "broadcast" exclusively
depends on the local APIC. Hence in x2APIC mode, even without
XT, 0xff does not mean "broadcast", and hence the code in
__cpu_up() is fine as is. In this setup there simply is no way
to encode broadcast at the IO-APIC or MSI level; luckily we
also don't use this mode.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

On 03.03.2020 17:05, Jan Beulich wrote:
> On 28.02.2020 13:31, Roger Pau Monné wrote:
>> On Fri, Feb 28, 2020 at 01:12:03PM +0100, Jan Beulich wrote:
>>> --- a/xen/arch/x86/genapic/x2apic.c
>>> +++ b/xen/arch/x86/genapic/x2apic.c
>>> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>>>          x2apic_phys = !iommu_intremap ||
>>>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>>      }
>>> -    else if ( !x2apic_phys && !iommu_intremap )
>>> -    {
>>> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
>>> -               "x2APIC: forcing phys mode\n");
>>> -        x2apic_phys = true;
>>> -    }
>>> +    else if ( !x2apic_phys )
>>> +        switch ( iommu_intremap )
>>> +        {
>>> +        case iommu_intremap_off:
>>> +        case iommu_intremap_restricted:
>>> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
>>> +                   "x2APIC: forcing phys mode\n",
>>> +                   iommu_intremap == iommu_intremap_off ? "without"
>>> +                                                        : "with restricted");
>>> +            x2apic_phys = true;
>>
>> I think you also need to fixup the usage of iommu_intremap in __cpu_up
>> so that CPUs with APIC IDs > 255 are not brought up when in
>> iommu_intremap_restricted mode.
> 
> I've looked around some (finding indications that, as one would
> suspect, broadcasting is also supported for IO-APIC based
> interrupts, and then by implication also for MSI) and then also
> thought about this some more. I think the corner case here
> resolves like this: Whether 0xff means "broadcast" exclusively
> depends on the local APIC. Hence in x2APIC mode, even without
> XT, 0xff does not mean "broadcast", and hence the code in
> __cpu_up() is fine as is. In this setup there simply is no way
> to encode broadcast at the IO-APIC or MSI level; luckily we
> also don't use this mode.

I'm talking rubbish - yes, the code needs adjusting as you and I
first thought. All that I'm now sure about is that the adjustment
doesn't need to be more complicated. I'll make a patch.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Andrew Cooper]

On 28/02/2020 12:12, Jan Beulich wrote:
> The wider cluster mode APIC IDs aren't generally representable. Convert
> the iommu_intremap variable into a tristate, allowing the AMD IOMMU
> driver to signal this special restriction to the apic_x2apic_probe().
> (Note: assignments to the variable get adjusted, while existing
> consumers - all assuming a boolean property - are left alone.)

I think it would be helpful to state that while we are not aware of any
hardware with this as a restriction, it is a situation which can be
created on fully x2apic-capable systems via firmware settings.

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
>
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>          x2apic_phys = !iommu_intremap ||
>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>      }
> -    else if ( !x2apic_phys && !iommu_intremap )
> -    {
> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
> -               "x2APIC: forcing phys mode\n");
> -        x2apic_phys = true;
> -    }
> +    else if ( !x2apic_phys )
> +        switch ( iommu_intremap )
> +        {
> +        case iommu_intremap_off:
> +        case iommu_intremap_restricted:
> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
> +                   "x2APIC: forcing phys mode\n",

Any chance to fold this into a single line with "- forcing phys mode\n"
as a suffix?

> +                   iommu_intremap == iommu_intremap_off ? "without"
> +                                                        : "with restricted");
> +            x2apic_phys = true;
> +            break;
> +
> +        case iommu_intremap_full:
> +            break;
> +        }
>  
>      if ( x2apic_phys )
>          return &apic_x2apic_phys;
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -54,7 +54,18 @@ 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_intremap, iommu_intpost;
> +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
> +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,

This needs a note about its meaning.  How about this?

/* Interrupt remapping enabled, but only able to generate interrupts
with an 8-bit APIC ID. */

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Not an issue for now, but "restricted" might become ambiguous with
future extensions.

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

On 28.02.2020 15:48, Andrew Cooper wrote:
> On 28/02/2020 12:12, Jan Beulich wrote:
>> The wider cluster mode APIC IDs aren't generally representable. Convert
>> the iommu_intremap variable into a tristate, allowing the AMD IOMMU
>> driver to signal this special restriction to the apic_x2apic_probe().
>> (Note: assignments to the variable get adjusted, while existing
>> consumers - all assuming a boolean property - are left alone.)
> 
> I think it would be helpful to state that while we are not aware of any
> hardware with this as a restriction, it is a situation which can be
> created on fully x2apic-capable systems via firmware settings.

Added.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: New.
>>
>> --- a/xen/arch/x86/genapic/x2apic.c
>> +++ b/xen/arch/x86/genapic/x2apic.c
>> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>>          x2apic_phys = !iommu_intremap ||
>>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>>      }
>> -    else if ( !x2apic_phys && !iommu_intremap )
>> -    {
>> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
>> -               "x2APIC: forcing phys mode\n");
>> -        x2apic_phys = true;
>> -    }
>> +    else if ( !x2apic_phys )
>> +        switch ( iommu_intremap )
>> +        {
>> +        case iommu_intremap_off:
>> +        case iommu_intremap_restricted:
>> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
>> +                   "x2APIC: forcing phys mode\n",
> 
> Any chance to fold this into a single line with "- forcing phys mode\n"
> as a suffix?

I did consider doing so myself, but didn't do it then for being
an unrelated change. Now that you ask for it - done.

>> +                   iommu_intremap == iommu_intremap_off ? "without"
>> +                                                        : "with restricted");
>> +            x2apic_phys = true;
>> +            break;
>> +
>> +        case iommu_intremap_full:
>> +            break;
>> +        }
>>  
>>      if ( x2apic_phys )
>>          return &apic_x2apic_phys;
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -54,7 +54,18 @@ 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_intremap, iommu_intpost;
>> +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
>> +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,
> 
> This needs a note about its meaning.  How about this?
> 
> /* Interrupt remapping enabled, but only able to generate interrupts
> with an 8-bit APIC ID. */

Added.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> Not an issue for now, but "restricted" might become ambiguous with
> future extensions.

Yes, fair point.

Jan

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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Jan Beulich]

On 28.02.2020 13:12, Jan Beulich wrote:
> The wider cluster mode APIC IDs aren't generally representable. Convert
> the iommu_intremap variable into a tristate, allowing the AMD IOMMU
> driver to signal this special restriction to the apic_x2apic_probe().
> (Note: assignments to the variable get adjusted, while existing
> consumers - all assuming a boolean property - are left alone.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Kevin,

I see you've looked over and acked Xen patches over the weekend. Did
you miss this one? It doesn't look controversial, so it'd be nice to
have your ack, for it to go in.

Jan

> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
>          x2apic_phys = !iommu_intremap ||
>                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
>      }
> -    else if ( !x2apic_phys && !iommu_intremap )
> -    {
> -        printk("WARNING: x2APIC cluster mode is not supported without interrupt remapping\n"
> -               "x2APIC: forcing phys mode\n");
> -        x2apic_phys = true;
> -    }
> +    else if ( !x2apic_phys )
> +        switch ( iommu_intremap )
> +        {
> +        case iommu_intremap_off:
> +        case iommu_intremap_restricted:
> +            printk("WARNING: x2APIC cluster mode is not supported %s interrupt remapping\n"
> +                   "x2APIC: forcing phys mode\n",
> +                   iommu_intremap == iommu_intremap_off ? "without"
> +                                                        : "with restricted");
> +            x2apic_phys = true;
> +            break;
> +
> +        case iommu_intremap_full:
> +            break;
> +        }
>  
>      if ( x2apic_phys )
>          return &apic_x2apic_phys;
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1139,7 +1139,7 @@ static void __init amd_iommu_init_cleanu
>  
>      iommu_enabled = 0;
>      iommu_hwdom_passthrough = false;
> -    iommu_intremap = 0;
> +    iommu_intremap = iommu_intremap_off;
>      iommuv2_enabled = 0;
>  }
>  
> @@ -1413,6 +1413,9 @@ int __init amd_iommu_prepare(bool xt)
>          iommu->ctrl.int_cap_xt_en = xt && has_xt;
>      }
>  
> +    if ( iommu_intremap && !has_xt )
> +        iommu_intremap = iommu_intremap_restricted;
> +
>      rc = amd_iommu_update_ivrs_mapping_acpi();
>  
>   error_out:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -157,7 +157,7 @@ int __init acpi_ivrs_init(void)
>  
>      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
>      {
> -        iommu_intremap = 0;
> +        iommu_intremap = iommu_intremap_off;
>          return -ENODEV;
>      }
>  
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -35,7 +35,7 @@ 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;
> -bool_t __read_mostly iommu_intremap = 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;
> @@ -91,7 +91,7 @@ static int __init parse_iommu_param(cons
>          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
>              iommu_qinval = val;
>          else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
> -            iommu_intremap = val;
> +            iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
>          else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
>              iommu_intpost = val;
>  #ifdef CONFIG_KEXEC
> @@ -475,7 +475,7 @@ int __init iommu_setup(void)
>          iommu_enabled = (rc == 0);
>      }
>      if ( !iommu_enabled )
> -        iommu_intremap = 0;
> +        iommu_intremap = iommu_intremap_off;
>  
>      if ( (force_iommu && !iommu_enabled) ||
>           (force_intremap && !iommu_intremap) )
> @@ -557,7 +557,8 @@ void iommu_crash_shutdown(void)
>  
>      if ( iommu_enabled )
>          iommu_get_ops()->crash_shutdown();
> -    iommu_enabled = iommu_intremap = iommu_intpost = 0;
> +    iommu_enabled = iommu_intpost = 0;
> +    iommu_intremap = iommu_intremap_off;
>  }
>  
>  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
> @@ -2177,7 +2177,7 @@ static int __must_check init_vtd_hw(void
>          {
>              if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
>              {
> -                iommu_intremap = 0;
> +                iommu_intremap = iommu_intremap_off;
>                  dprintk(XENLOG_ERR VTDPREFIX,
>                      "ioapic_to_iommu: ioapic %#x (id: %#x) is NULL! "
>                      "Will not try to enable Interrupt Remapping.\n",
> @@ -2193,7 +2193,7 @@ static int __must_check init_vtd_hw(void
>              iommu = drhd->iommu;
>              if ( enable_intremap(iommu, 0) != 0 )
>              {
> -                iommu_intremap = 0;
> +                iommu_intremap = iommu_intremap_off;
>                  dprintk(XENLOG_WARNING VTDPREFIX,
>                          "Interrupt Remapping not enabled\n");
>  
> @@ -2295,7 +2295,7 @@ static int __init vtd_setup(void)
>              iommu_qinval = 0;
>  
>          if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
> -            iommu_intremap = 0;
> +            iommu_intremap = iommu_intremap_off;
>  
>          /*
>           * We cannot use posted interrupt if X86_FEATURE_CX16 is
> @@ -2320,7 +2320,7 @@ static int __init vtd_setup(void)
>  
>      if ( !iommu_qinval && iommu_intremap )
>      {
> -        iommu_intremap = 0;
> +        iommu_intremap = iommu_intremap_off;
>          dprintk(XENLOG_WARNING VTDPREFIX, "Interrupt Remapping disabled "
>              "since Queued Invalidation isn't supported or enabled.\n");
>      }
> @@ -2347,7 +2347,7 @@ static int __init vtd_setup(void)
>      iommu_snoop = 0;
>      iommu_hwdom_passthrough = false;
>      iommu_qinval = 0;
> -    iommu_intremap = 0;
> +    iommu_intremap = iommu_intremap_off;
>      iommu_intpost = 0;
>      return ret;
>  }
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -54,7 +54,18 @@ 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_intremap, iommu_intpost;
> +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
> +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;
>  
>  #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
> 


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

Re: [Xen-devel] [PATCH v2 2/2] AMD/IOMMU: without XT, x2APIC needs to be forced into physical mode

[Tian, Kevin]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, March 9, 2020 4:59 PM
> 
> On 28.02.2020 13:12, Jan Beulich wrote:
> > The wider cluster mode APIC IDs aren't generally representable. Convert
> > the iommu_intremap variable into a tristate, allowing the AMD IOMMU
> > driver to signal this special restriction to the apic_x2apic_probe().
> > (Note: assignments to the variable get adjusted, while existing
> > consumers - all assuming a boolean property - are left alone.)
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Kevin,
> 
> I see you've looked over and acked Xen patches over the weekend. Did
> you miss this one? It doesn't look controversial, so it'd be nice to
> have your ack, for it to go in.
> 
> Jan

It is in my list. My review process was interrupted by other tasks
yesterday, and now is resumed. 😊

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> > --- a/xen/arch/x86/genapic/x2apic.c
> > +++ b/xen/arch/x86/genapic/x2apic.c
> > @@ -236,12 +236,21 @@ const struct genapic *__init apic_x2apic
> >          x2apic_phys = !iommu_intremap ||
> >                        (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL);
> >      }
> > -    else if ( !x2apic_phys && !iommu_intremap )
> > -    {
> > -        printk("WARNING: x2APIC cluster mode is not supported without
> interrupt remapping\n"
> > -               "x2APIC: forcing phys mode\n");
> > -        x2apic_phys = true;
> > -    }
> > +    else if ( !x2apic_phys )
> > +        switch ( iommu_intremap )
> > +        {
> > +        case iommu_intremap_off:
> > +        case iommu_intremap_restricted:
> > +            printk("WARNING: x2APIC cluster mode is not supported %s
> interrupt remapping\n"
> > +                   "x2APIC: forcing phys mode\n",
> > +                   iommu_intremap == iommu_intremap_off ? "without"
> > +                                                        : "with restricted");
> > +            x2apic_phys = true;
> > +            break;
> > +
> > +        case iommu_intremap_full:
> > +            break;
> > +        }
> >
> >      if ( x2apic_phys )
> >          return &apic_x2apic_phys;
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -1139,7 +1139,7 @@ static void __init amd_iommu_init_cleanu
> >
> >      iommu_enabled = 0;
> >      iommu_hwdom_passthrough = false;
> > -    iommu_intremap = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >      iommuv2_enabled = 0;
> >  }
> >
> > @@ -1413,6 +1413,9 @@ int __init amd_iommu_prepare(bool xt)
> >          iommu->ctrl.int_cap_xt_en = xt && has_xt;
> >      }
> >
> > +    if ( iommu_intremap && !has_xt )
> > +        iommu_intremap = iommu_intremap_restricted;
> > +
> >      rc = amd_iommu_update_ivrs_mapping_acpi();
> >
> >   error_out:
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -157,7 +157,7 @@ int __init acpi_ivrs_init(void)
> >
> >      if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) )
> >      {
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >          return -ENODEV;
> >      }
> >
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -35,7 +35,7 @@ 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;
> > -bool_t __read_mostly iommu_intremap = 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;
> > @@ -91,7 +91,7 @@ static int __init parse_iommu_param(cons
> >          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
> >              iommu_qinval = val;
> >          else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
> > -            iommu_intremap = val;
> > +            iommu_intremap = val ? iommu_intremap_full :
> iommu_intremap_off;
> >          else if ( (val = parse_boolean("intpost", s, ss)) >= 0 )
> >              iommu_intpost = val;
> >  #ifdef CONFIG_KEXEC
> > @@ -475,7 +475,7 @@ int __init iommu_setup(void)
> >          iommu_enabled = (rc == 0);
> >      }
> >      if ( !iommu_enabled )
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >
> >      if ( (force_iommu && !iommu_enabled) ||
> >           (force_intremap && !iommu_intremap) )
> > @@ -557,7 +557,8 @@ void iommu_crash_shutdown(void)
> >
> >      if ( iommu_enabled )
> >          iommu_get_ops()->crash_shutdown();
> > -    iommu_enabled = iommu_intremap = iommu_intpost = 0;
> > +    iommu_enabled = iommu_intpost = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >  }
> >
> >  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
> > @@ -2177,7 +2177,7 @@ static int __must_check init_vtd_hw(void
> >          {
> >              if ( ioapic_to_iommu(IO_APIC_ID(apic)) == NULL )
> >              {
> > -                iommu_intremap = 0;
> > +                iommu_intremap = iommu_intremap_off;
> >                  dprintk(XENLOG_ERR VTDPREFIX,
> >                      "ioapic_to_iommu: ioapic %#x (id: %#x) is NULL! "
> >                      "Will not try to enable Interrupt Remapping.\n",
> > @@ -2193,7 +2193,7 @@ static int __must_check init_vtd_hw(void
> >              iommu = drhd->iommu;
> >              if ( enable_intremap(iommu, 0) != 0 )
> >              {
> > -                iommu_intremap = 0;
> > +                iommu_intremap = iommu_intremap_off;
> >                  dprintk(XENLOG_WARNING VTDPREFIX,
> >                          "Interrupt Remapping not enabled\n");
> >
> > @@ -2295,7 +2295,7 @@ static int __init vtd_setup(void)
> >              iommu_qinval = 0;
> >
> >          if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
> > -            iommu_intremap = 0;
> > +            iommu_intremap = iommu_intremap_off;
> >
> >          /*
> >           * We cannot use posted interrupt if X86_FEATURE_CX16 is
> > @@ -2320,7 +2320,7 @@ static int __init vtd_setup(void)
> >
> >      if ( !iommu_qinval && iommu_intremap )
> >      {
> > -        iommu_intremap = 0;
> > +        iommu_intremap = iommu_intremap_off;
> >          dprintk(XENLOG_WARNING VTDPREFIX, "Interrupt Remapping
> disabled "
> >              "since Queued Invalidation isn't supported or enabled.\n");
> >      }
> > @@ -2347,7 +2347,7 @@ static int __init vtd_setup(void)
> >      iommu_snoop = 0;
> >      iommu_hwdom_passthrough = false;
> >      iommu_qinval = 0;
> > -    iommu_intremap = 0;
> > +    iommu_intremap = iommu_intremap_off;
> >      iommu_intpost = 0;
> >      return ret;
> >  }
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -54,7 +54,18 @@ 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_intremap,
> iommu_intpost;
> > +extern bool_t iommu_snoop, iommu_qinval, iommu_intpost;
> > +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;
> >
> >  #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
> >

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