* Re: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-14 13:58 [PATCH v2] iommu: setup inclusive mappings before enabling iommu Roger Pau Monne
@ 2018-09-14 16:35 ` Dario Faggioli
2018-09-17 12:49 ` Jan Beulich
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Dario Faggioli @ 2018-09-14 16:35 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit,
Julien Grall, Jan Beulich, Brian Woods
[-- Attachment #1.1: Type: text/plain, Size: 1416 bytes --]
On Fri, 2018-09-14 at 15:58 +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
> <freeze>
>
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
>
> Note that on AMD hardware the order is also changed to add inclusive
> mappings before adding any devices.
>
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
I tested this too (sorry it took a bit more) and, like v1, it works.
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-14 13:58 [PATCH v2] iommu: setup inclusive mappings before enabling iommu Roger Pau Monne
2018-09-14 16:35 ` Dario Faggioli
@ 2018-09-17 12:49 ` Jan Beulich
2018-09-17 13:22 ` Roger Pau Monné
2018-09-18 13:00 ` Ping: " Jan Beulich
` (2 subsequent siblings)
4 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2018-09-17 12:49 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Kevin Tian, Stefano Stabellini, Dario Faggioli, Julien Grall,
Suravee Suthikulpanit, xen-devel, Brian Woods
>>> On 14.09.18 at 15:58, <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
> <freeze>
>
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
>
> Note that on AMD hardware the order is also changed to add inclusive
> mappings before adding any devices.
>
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
With the exception of the ARM aspect (because it's not
clear to me why the code removed from iommu_hwdom_init()
doesn't need to re-appear in ARM-specific code as well)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-17 12:49 ` Jan Beulich
@ 2018-09-17 13:22 ` Roger Pau Monné
0 siblings, 0 replies; 7+ messages in thread
From: Roger Pau Monné @ 2018-09-17 13:22 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Stefano Stabellini, Dario Faggioli, Julien Grall,
Suravee Suthikulpanit, xen-devel, Brian Woods
On Mon, Sep 17, 2018 at 06:49:24AM -0600, Jan Beulich wrote:
> >>> On 14.09.18 at 15:58, <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
> > <freeze>
> >
> > This restores the behavior before commit 66a9274cc3435 that changed
> > the order and enabled the iommu without having the inclusive mappings
> > setup.
> >
> > Note that on AMD hardware the order is also changed to add inclusive
> > mappings before adding any devices.
> >
> > Reported-by: Dario Faggioli <dfaggioli@suse.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> With the exception of the ARM aspect (because it's not
> clear to me why the code removed from iommu_hwdom_init()
> doesn't need to re-appear in ARM-specific code as well)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I guess you mean the following chunk:
- if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
- {
- printk(XENLOG_WARNING
- "IOMMU inclusive mappings are only supported on PV Dom0\n");
- iommu_hwdom_inclusive = 0;
- }
I haven't added it for ARM because iommu_hwdom_inclusive is always set
to 0 in the ARM case, so the condition would never match.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Ping: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-14 13:58 [PATCH v2] iommu: setup inclusive mappings before enabling iommu Roger Pau Monne
2018-09-14 16:35 ` Dario Faggioli
2018-09-17 12:49 ` Jan Beulich
@ 2018-09-18 13:00 ` Jan Beulich
2018-09-19 5:32 ` Tian, Kevin
2018-09-20 16:02 ` Julien Grall
4 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2018-09-18 13:00 UTC (permalink / raw)
To: Suravee Suthikulpanit, Julien Grall, Kevin Tian
Cc: xen-devel, Dario Faggioli, Stefano Stabellini, Brian Woods,
Roger Pau Monne
>>> On 14.09.18 at 15:58, <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
> <freeze>
>
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
>
> Note that on AMD hardware the order is also changed to add inclusive
> mappings before adding any devices.
>
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
All,
I realize it's a bit early for a ping, but the master branch is stuck at a
regression this patch is supposed to address. May I therefore please
ask for timely review of this patch?
Thanks, Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-14 13:58 [PATCH v2] iommu: setup inclusive mappings before enabling iommu Roger Pau Monne
` (2 preceding siblings ...)
2018-09-18 13:00 ` Ping: " Jan Beulich
@ 2018-09-19 5:32 ` Tian, Kevin
2018-09-20 16:02 ` Julien Grall
4 siblings, 0 replies; 7+ messages in thread
From: Tian, Kevin @ 2018-09-19 5:32 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini, Suravee Suthikulpanit, Dario Faggioli,
Julien Grall, Jan Beulich, Brian Woods
> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Friday, September 14, 2018 9:59 PM
>
> 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
> <freeze>
>
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
>
> Note that on AMD hardware the order is also changed to add inclusive
> mappings before adding any devices.
>
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] iommu: setup inclusive mappings before enabling iommu
2018-09-14 13:58 [PATCH v2] iommu: setup inclusive mappings before enabling iommu Roger Pau Monne
` (3 preceding siblings ...)
2018-09-19 5:32 ` Tian, Kevin
@ 2018-09-20 16:02 ` Julien Grall
4 siblings, 0 replies; 7+ messages in thread
From: Julien Grall @ 2018-09-20 16:02 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit,
Dario Faggioli, Jan Beulich, Brian Woods
Hi Roger,
On 09/14/2018 02:58 PM, 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
> <freeze>
>
> This restores the behavior before commit 66a9274cc3435 that changed
> the order and enabled the iommu without having the inclusive mappings
> setup.
>
> Note that on AMD hardware the order is also changed to add inclusive
> mappings before adding any devices.
>
> Reported-by: Dario Faggioli <dfaggioli@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Dario Faggioli <dfaggioli@suse.com>
> ---
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 ++
> xen/drivers/passthrough/arm/smmu.c | 2 ++
> xen/drivers/passthrough/iommu.c | 10 ----------
> xen/drivers/passthrough/vtd/iommu.c | 2 ++
> xen/drivers/passthrough/x86/iommu.c | 8 ++++++++
> 5 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 330f9ce386..4a633ca940 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -300,6 +300,8 @@ static void __hwdom_init amd_iommu_hwdom_init(struct domain *d)
> IOMMU_MMIO_REGION_LENGTH - 1)) )
> BUG();
>
> + /* Make sure workarounds are applied (if needed) before adding devices. */
> + arch_iommu_hwdom_init(d);
> setup_hwdom_pci_devices(d, amd_iommu_add_device);
> }
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 43ece42a50..8f91807b1b 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2736,6 +2736,8 @@ static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
> printk(XENLOG_WARNING
> "map-reserved dom0-iommu option is not supported on ARM\n");
> iommu_hwdom_reserved = 0;
> +
> + arch_iommu_hwdom_init(d);
> }
>
> static void arm_smmu_iommu_domain_teardown(struct domain *d)
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index ee3f523fdf..ae6cf2f0ff 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -238,16 +238,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> }
>
> hd->platform_ops->hwdom_init(d);
> -
> - ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> - if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
> - {
> - printk(XENLOG_WARNING
> - "IOMMU inclusive mappings are only supported on PV Dom0\n");
> - iommu_hwdom_inclusive = 0;
> - }
> -
> - arch_iommu_hwdom_init(d);
> }
>
> void iommu_teardown(struct domain *d)
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index adc70f205a..bb422ec58c 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1313,6 +1313,8 @@ static void __hwdom_init intel_iommu_hwdom_init(struct domain *d)
>
> setup_hwdom_pci_devices(d, setup_hwdom_device);
> setup_hwdom_rmrr(d);
> + /* Make sure workarounds are applied before enabling the IOMMU(s). */
> + arch_iommu_hwdom_init(d);
>
> if ( iommu_flush_all() )
> printk(XENLOG_WARNING VTDPREFIX
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index 47a078272a..b7c8b5be41 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -210,6 +210,14 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>
> BUG_ON(!is_hardware_domain(d));
>
> + ASSERT(iommu_hwdom_inclusive != -1 && iommu_hwdom_inclusive != -1);
> + if ( iommu_hwdom_inclusive && !is_pv_domain(d) )
> + {
> + printk(XENLOG_WARNING
> + "IOMMU inclusive mappings are only supported on PV Dom0\n");
> + iommu_hwdom_inclusive = 0;
> + }
> +
> if ( iommu_hwdom_passthrough )
> return;
>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread