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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.18 at 10:02, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel