[PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Roger Pau Monne]

Or else it can lead to freezes when enabling the iommu on certain
Intel hardware:

[...]
(XEN) ELF: addresses:
(XEN)     virt_base        = 0xffffffff80000000
(XEN)     elf_paddr_offset = 0x0
(XEN)     virt_offset      = 0xffffffff80000000
(XEN)     virt_kstart      = 0xffffffff81000000
(XEN)     virt_kend        = 0xffffffff82953000
(XEN)     virt_entry       = 0xffffffff8274e180
(XEN)     p2m_base         = 0x8000000000
(XEN)  Xen  kernel: 64-bit, lsb, compat32
(XEN)  Dom0 kernel: 64-bit, PAE, lsb, paddr 0x1000000 -> 0x295300

This restores the behavior before commit 66a9274cc3435 that changed
the order and enabled the iommu without having the inclusive mappings
setup.

Note that in order to restore previous behavior a new enable hook is
added to the iommu_ops struct that's only used by VT-d.

Reported-by: Dario Faggioli <dfaggioli@suse.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Dario Faggioli <dfaggioli@suse.com>
---
 xen/drivers/passthrough/iommu.c     |  3 +++
 xen/drivers/passthrough/vtd/iommu.c | 10 ++++++++--
 xen/include/xen/iommu.h             |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ee3f523fdf..1e291aa673 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -248,6 +248,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
     }
 
     arch_iommu_hwdom_init(d);
+
+    if ( hd->platform_ops->enable )
+        hd->platform_ops->enable();
 }
 
 void iommu_teardown(struct domain *d)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index adc70f205a..ac30623697 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1319,12 +1319,17 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
                " IOMMU flush all failed for hardware domain\n");
 
     for_each_drhd_unit ( drhd )
-    {
         if ( iomem_deny_access(d, PFN_DOWN(drhd->address),
                                PFN_DOWN(drhd->address)) )
             BUG();
+}
+
+static void __hwdom_init intel_iommu_enable(void)
+{
+    struct acpi_drhd_unit *drhd;
+
+    for_each_drhd_unit ( drhd )
         iommu_enable_translation(drhd);
-    }
 }
 
 int domain_context_mapping_one(
@@ -2650,6 +2655,7 @@ static void vtd_dump_p2m_table(struct domain *d)
 
 const struct iommu_ops intel_iommu_ops = {
     .init = intel_iommu_domain_init,
+    .enable = intel_iommu_enable,
     .hwdom_init = intel_iommu_hwdom_init,
     .add_device = intel_iommu_add_device,
     .enable_device = intel_iommu_enable_device,
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 57c4e81ec6..96b4ff6e1e 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -140,6 +140,7 @@ typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt);
 
 struct iommu_ops {
     int (*init)(struct domain *d);
+    void (*enable)(void);
     void (*hwdom_init)(struct domain *d);
     int (*add_device)(u8 devfn, device_t *dev);
     int (*enable_device)(device_t *dev);
-- 
2.18.0


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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Jan Beulich]

>>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
> Or else it can lead to freezes when enabling the iommu on certain
> Intel hardware:
> 
> [...]
> (XEN) ELF: addresses:
> (XEN)     virt_base        = 0xffffffff80000000
> (XEN)     elf_paddr_offset = 0x0
> (XEN)     virt_offset      = 0xffffffff80000000
> (XEN)     virt_kstart      = 0xffffffff81000000
> (XEN)     virt_kend        = 0xffffffff82953000
> (XEN)     virt_entry       = 0xffffffff8274e180
> (XEN)     p2m_base         = 0x8000000000
> (XEN)  Xen  kernel: 64-bit, lsb, compat32
> (XEN)  Dom0 kernel: 64-bit, PAE, lsb, paddr 0x1000000 -> 0x295300

I think you mean to tell us that output stops after these lines, but
that's in no way made explicit.

> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
> 
> Note that in order to restore previous behavior a new enable hook is
> added to the iommu_ops struct that's only used by VT-d.

But your earlier series also extends inclusive mapping support to AMD -
why is there no similar change needed there in case someone overrides
the default of off in that case?

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -248,6 +248,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      }
>  
>      arch_iommu_hwdom_init(d);
> +
> +    if ( hd->platform_ops->enable )
> +        hd->platform_ops->enable();
>  }

I realize this is nothing you change, but this is a __hwdom_init
function, yet doing the enable only when constructing the "late hwdom"
is imo too late (the same imo applies to the key handler registration,
btw). I wonder what the original authors' thoughts here were...

While looking at this I also notice that dom0_construct_pvh()'s call to
iommu_hwdom_init() is unconditional, while dom0_construct_pv()'s is
conditional. Is this really intentional?

Furthermore, an option without new hook would look to be to call
arch_iommu_hwdom_init() out of intel_iommu_hwdom_init(). This
would probably require the function to bail when invoked a second
time; I'm sure there is a way to recognize this fact.

Jan



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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Roger Pau Monné]

On Fri, Sep 14, 2018 at 03:06:03AM -0600, Jan Beulich wrote:
> >>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
> > Or else it can lead to freezes when enabling the iommu on certain
> > Intel hardware:
> > 
> > [...]
> > (XEN) ELF: addresses:
> > (XEN)     virt_base        = 0xffffffff80000000
> > (XEN)     elf_paddr_offset = 0x0
> > (XEN)     virt_offset      = 0xffffffff80000000
> > (XEN)     virt_kstart      = 0xffffffff81000000
> > (XEN)     virt_kend        = 0xffffffff82953000
> > (XEN)     virt_entry       = 0xffffffff8274e180
> > (XEN)     p2m_base         = 0x8000000000
> > (XEN)  Xen  kernel: 64-bit, lsb, compat32
> > (XEN)  Dom0 kernel: 64-bit, PAE, lsb, paddr 0x1000000 -> 0x295300
> 
> I think you mean to tell us that output stops after these lines, but
> that's in no way made explicit.

Right, I meant to imply that from the '...lead to freezes...' in the
sentence above, but I will make it explicit by adding a <freezes> tag
at the end of the log.

> > This restores the behavior before commit 66a9274cc3435 that changed
> > the order and enabled the iommu without having the inclusive mappings
> > setup.
> > 
> > Note that in order to restore previous behavior a new enable hook is
> > added to the iommu_ops struct that's only used by VT-d.
> 
> But your earlier series also extends inclusive mapping support to AMD -
> why is there no similar change needed there in case someone overrides
> the default of off in that case?

I don't see any iommu enable related code in amd_iommu_hwdom_init, but
maybe I'm missing something (same applies to ARM SMMU). AFAICT for AMD
the iommu is initialized in iommu_setup which happens before Dom0
creation.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -248,6 +248,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >      }
> >  
> >      arch_iommu_hwdom_init(d);
> > +
> > +    if ( hd->platform_ops->enable )
> > +        hd->platform_ops->enable();
> >  }
> 
> I realize this is nothing you change, but this is a __hwdom_init
> function, yet doing the enable only when constructing the "late hwdom"
> is imo too late (the same imo applies to the key handler registration,
> btw). I wonder what the original authors' thoughts here were...

Yes, I agree this is not ideal, but I didn't want to change the
behavior here, since this is a bugfix to restore the previous
functionality.

> While looking at this I also notice that dom0_construct_pvh()'s call to
> iommu_hwdom_init() is unconditional, while dom0_construct_pv()'s is
> conditional. Is this really intentional?

No, I don't think so. AFAICT it should have the same check also
present on the PV Dom0 builder.

But then other logic in the PVH Dom0 builder should also be moved
under such check. For example a PVH Dom0 that's not the hardware
domain shouldn't get a vIOAPIC, access to the native ACPI tables or
the low 1MB and it could even have a flat physmap, as a PVH DomU would
get.

Note that such check also seems to be missing on the ARM Dom0 builder.

> Furthermore, an option without new hook would look to be to call
> arch_iommu_hwdom_init() out of intel_iommu_hwdom_init(). This
> would probably require the function to bail when invoked a second
> time; I'm sure there is a way to recognize this fact.

This was my first approach, but I didn't like it because then I would
either have to move the call to arch_iommu_hwdom_init into the
different hwdom_init hooks, or as you say allow it to be called
multiple times (on Intel hw it would be called twice, while on other
hw only once). I think adding this new hook is the cleaner option IMO.

Thanks, Roger.

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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Jan Beulich]

>>> On 14.09.18 at 11:54, <roger.pau@citrix.com> wrote:
> On Fri, Sep 14, 2018 at 03:06:03AM -0600, Jan Beulich wrote:
>> >>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
>> > This restores the behavior before commit 66a9274cc3435 that changed
>> > the order and enabled the iommu without having the inclusive mappings
>> > setup.
>> > 
>> > Note that in order to restore previous behavior a new enable hook is
>> > added to the iommu_ops struct that's only used by VT-d.
>> 
>> But your earlier series also extends inclusive mapping support to AMD -
>> why is there no similar change needed there in case someone overrides
>> the default of off in that case?
> 
> I don't see any iommu enable related code in amd_iommu_hwdom_init, but
> maybe I'm missing something (same applies to ARM SMMU). AFAICT for AMD
> the iommu is initialized in iommu_setup which happens before Dom0
> creation.

But isn't the problem here that the IOMMU gets enabled too early?
Who or what tells us this isn't a problem on AMD as well (which then
would be another regression of your earlier series, not the patch
here, but would presumably also want fixing here)?

>> While looking at this I also notice that dom0_construct_pvh()'s call to
>> iommu_hwdom_init() is unconditional, while dom0_construct_pv()'s is
>> conditional. Is this really intentional?
> 
> No, I don't think so. AFAICT it should have the same check also
> present on the PV Dom0 builder.
> 
> But then other logic in the PVH Dom0 builder should also be moved
> under such check. For example a PVH Dom0 that's not the hardware
> domain shouldn't get a vIOAPIC, access to the native ACPI tables or
> the low 1MB and it could even have a flat physmap, as a PVH DomU would
> get.

Can I take it that you'll be looking into this, a part of your PVH Dom0
work?

Jan



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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Roger Pau Monné]

On Fri, Sep 14, 2018 at 04:49:49AM -0600, Jan Beulich wrote:
> >>> On 14.09.18 at 11:54, <roger.pau@citrix.com> wrote:
> > On Fri, Sep 14, 2018 at 03:06:03AM -0600, Jan Beulich wrote:
> >> >>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
> >> > This restores the behavior before commit 66a9274cc3435 that changed
> >> > the order and enabled the iommu without having the inclusive mappings
> >> > setup.
> >> > 
> >> > Note that in order to restore previous behavior a new enable hook is
> >> > added to the iommu_ops struct that's only used by VT-d.
> >> 
> >> But your earlier series also extends inclusive mapping support to AMD -
> >> why is there no similar change needed there in case someone overrides
> >> the default of off in that case?
> > 
> > I don't see any iommu enable related code in amd_iommu_hwdom_init, but
> > maybe I'm missing something (same applies to ARM SMMU). AFAICT for AMD
> > the iommu is initialized in iommu_setup which happens before Dom0
> > creation.
> 
> But isn't the problem here that the IOMMU gets enabled too early?

For Intel yes, my earlier series changed the order for Intel and the
IOMMU got initialized before setting the inclusive mappings.

> Who or what tells us this isn't a problem on AMD as well (which then
> would be another regression of your earlier series, not the patch
> here, but would presumably also want fixing here)?

My earlier series only changed the initialization order of the Intel
IOMMU, AMD IOMMU has always been initialized earlier in iommu_setup,
and that's not changed.

> >> While looking at this I also notice that dom0_construct_pvh()'s call to
> >> iommu_hwdom_init() is unconditional, while dom0_construct_pv()'s is
> >> conditional. Is this really intentional?
> > 
> > No, I don't think so. AFAICT it should have the same check also
> > present on the PV Dom0 builder.
> > 
> > But then other logic in the PVH Dom0 builder should also be moved
> > under such check. For example a PVH Dom0 that's not the hardware
> > domain shouldn't get a vIOAPIC, access to the native ACPI tables or
> > the low 1MB and it could even have a flat physmap, as a PVH DomU would
> > get.
> 
> Can I take it that you'll be looking into this, a part of your PVH Dom0
> work?

Sure, there's some work to be done there.

Thanks, Roger.

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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Jan Beulich]

>>> On 14.09.18 at 13:06, <roger.pau@citrix.com> wrote:
> On Fri, Sep 14, 2018 at 04:49:49AM -0600, Jan Beulich wrote:
>> >>> On 14.09.18 at 11:54, <roger.pau@citrix.com> wrote:
>> > On Fri, Sep 14, 2018 at 03:06:03AM -0600, Jan Beulich wrote:
>> >> >>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
>> >> > This restores the behavior before commit 66a9274cc3435 that changed
>> >> > the order and enabled the iommu without having the inclusive mappings
>> >> > setup.
>> >> > 
>> >> > Note that in order to restore previous behavior a new enable hook is
>> >> > added to the iommu_ops struct that's only used by VT-d.
>> >> 
>> >> But your earlier series also extends inclusive mapping support to AMD -
>> >> why is there no similar change needed there in case someone overrides
>> >> the default of off in that case?
>> > 
>> > I don't see any iommu enable related code in amd_iommu_hwdom_init, but
>> > maybe I'm missing something (same applies to ARM SMMU). AFAICT for AMD
>> > the iommu is initialized in iommu_setup which happens before Dom0
>> > creation.
>> 
>> But isn't the problem here that the IOMMU gets enabled too early?
> 
> For Intel yes, my earlier series changed the order for Intel and the
> IOMMU got initialized before setting the inclusive mappings.
> 
>> Who or what tells us this isn't a problem on AMD as well (which then
>> would be another regression of your earlier series, not the patch
>> here, but would presumably also want fixing here)?
> 
> My earlier series only changed the initialization order of the Intel
> IOMMU, AMD IOMMU has always been initialized earlier in iommu_setup,
> and that's not changed.

But prior to that series, the command line parameter was private to
xen/drivers/passthrough/vtd/x86/vtd.c. You've insisted on making
the option available on AMD, but if it is to be of any use, ordering
there likely needs to match that for VT-d.

Jan



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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Roger Pau Monné]

On Fri, Sep 14, 2018 at 05:13:48AM -0600, Jan Beulich wrote:
> >>> On 14.09.18 at 13:06, <roger.pau@citrix.com> wrote:
> > On Fri, Sep 14, 2018 at 04:49:49AM -0600, Jan Beulich wrote:
> >> >>> On 14.09.18 at 11:54, <roger.pau@citrix.com> wrote:
> >> > On Fri, Sep 14, 2018 at 03:06:03AM -0600, Jan Beulich wrote:
> >> >> >>> On 14.09.18 at 10:02, <roger.pau@citrix.com> wrote:
> >> >> > This restores the behavior before commit 66a9274cc3435 that changed
> >> >> > the order and enabled the iommu without having the inclusive mappings
> >> >> > setup.
> >> >> > 
> >> >> > Note that in order to restore previous behavior a new enable hook is
> >> >> > added to the iommu_ops struct that's only used by VT-d.
> >> >> 
> >> >> But your earlier series also extends inclusive mapping support to AMD -
> >> >> why is there no similar change needed there in case someone overrides
> >> >> the default of off in that case?
> >> > 
> >> > I don't see any iommu enable related code in amd_iommu_hwdom_init, but
> >> > maybe I'm missing something (same applies to ARM SMMU). AFAICT for AMD
> >> > the iommu is initialized in iommu_setup which happens before Dom0
> >> > creation.
> >> 
> >> But isn't the problem here that the IOMMU gets enabled too early?
> > 
> > For Intel yes, my earlier series changed the order for Intel and the
> > IOMMU got initialized before setting the inclusive mappings.
> > 
> >> Who or what tells us this isn't a problem on AMD as well (which then
> >> would be another regression of your earlier series, not the patch
> >> here, but would presumably also want fixing here)?
> > 
> > My earlier series only changed the initialization order of the Intel
> > IOMMU, AMD IOMMU has always been initialized earlier in iommu_setup,
> > and that's not changed.
> 
> But prior to that series, the command line parameter was private to
> xen/drivers/passthrough/vtd/x86/vtd.c. You've insisted on making
> the option available on AMD, but if it is to be of any use, ordering
> there likely needs to match that for VT-d.

OK, I think the requirement here would be that inclusive mappings are
always setup before either the IOMMU is enabled (on the Intel case) or
devices are added (on the AMD case, since the IOMMU is enabled very
early).

So I think the best way to solve this is to move the call to
arch_iommu_hwdom_init inside of the hwdom_init handlers for each
implementation.

Roger.

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

Re: [PATCH] intel/iommu: setup inclusive mappings before enabling iommu

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1446 bytes --]

On Fri, 2018-09-14 at 10:02 +0200, Roger Pau Monne wrote:
> Or else it can lead to freezes when enabling the iommu on certain
> Intel hardware:
> 
> [...]
> (XEN) ELF: addresses:
> (XEN)     virt_base        = 0xffffffff80000000
> (XEN)     elf_paddr_offset = 0x0
> (XEN)     virt_offset      = 0xffffffff80000000
> (XEN)     virt_kstart      = 0xffffffff81000000
> (XEN)     virt_kend        = 0xffffffff82953000
> (XEN)     virt_entry       = 0xffffffff8274e180
> (XEN)     p2m_base         = 0x8000000000
> (XEN)  Xen  kernel: 64-bit, lsb, compat32
> (XEN)  Dom0 kernel: 64-bit, PAE, lsb, paddr 0x1000000 -> 0x295300
> 
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
> 
> Note that in order to restore previous behavior a new enable hook is
> added to the iommu_ops struct that's only used by VT-d.
> 
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
My box (the one that was freezing with current staging) boots fine if I
apply this patch.

Tested-by: Dario Faggioli <dfaggioli@suse.com>

Thanks,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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