Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
Hi Jan, On 06/11/2018 15:48, Jan Beulich wrote: On 02.10.18 at 12:38, wrote: On 02/10/2018 11:18, Jan Beulich wrote: ARM is intended to gain support for heterogeneous IOMMUs on a single system. This not only disallows boot time replacement of respective indirect calls (handling of which is the main goal of the introduction here), but more generally disallows calls using the iommu_ops() return value directly - all such calls need to have means (commonly a domain pointer) to know the targeted IOMMU. Disallow all hooks lacking such context for the time being, which in effect is some dead code elimination for ARM. Once extended suitably, individual of these hooks can be moved out of their guards again in the future. While in theory it is possible to have platform with hetereneous IOMMUs. I don't see such such support coming in Xen for the foreseeable future. Note that even Linux does not support such case. This patch is going to make more complicate to unshare page-tables as now we would need to care of mixed case. So I would rather not set IOMMU_MIXED on Arm until we have a use case for it. So if I drop this here, how would you want iommu_get_ops() get handled on Arm (patch 11)? Right now I'd mean to leave it alone, but it could also be switched to the (new) x86 way (but that would then perhaps make mixed mode introduction more difficult down the road), allowing to get away with fewer #ifdef-s. I would introduce the iommu_get_ops for x86 in asm-x86/iommu.h. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
>>> On 02.10.18 at 12:38, wrote: > On 02/10/2018 11:18, Jan Beulich wrote: >> ARM is intended to gain support for heterogeneous IOMMUs on a single >> system. This not only disallows boot time replacement of respective >> indirect calls (handling of which is the main goal of the introduction >> here), but more generally disallows calls using the iommu_ops() return >> value directly - all such calls need to have means (commonly a domain >> pointer) to know the targeted IOMMU. >> >> Disallow all hooks lacking such context for the time being, which in >> effect is some dead code elimination for ARM. Once extended suitably, >> individual of these hooks can be moved out of their guards again in the >> future. > > While in theory it is possible to have platform with hetereneous IOMMUs. > I don't see such such support coming in Xen for the foreseeable > future. Note that even Linux does not support such case. > > This patch is going to make more complicate to unshare page-tables as > now we would need to care of mixed case. So I would rather not set > IOMMU_MIXED on Arm until we have a use case for it. So if I drop this here, how would you want iommu_get_ops() get handled on Arm (patch 11)? Right now I'd mean to leave it alone, but it could also be switched to the (new) x86 way (but that would then perhaps make mixed mode introduction more difficult down the road), allowing to get away with fewer #ifdef-s. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
Hi Jan, On 02/10/2018 12:58, Jan Beulich wrote: Well, that's what I would have done if you hadn't brought up the mixed-IOMMU case - clearly, without being able to test, I wouldn't have dared to try and implement patching of indirect calls for ARM. I can certainly drop that patch, but in the end it means you've made me do more work than would have been needed to reach the immediate goal I have. I have never asked you to re-arrange the code for Arm. I only asked to avoid patching indirect calls for Arm and explained why I wanted to avoid it. Sorry for the misunderstanding. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
>>> On 02.10.18 at 13:00, wrote: > On 02/10/2018 11:42, Jan Beulich wrote: > On 02.10.18 at 12:38, wrote: >>> On 02/10/2018 11:18, Jan Beulich wrote: ARM is intended to gain support for heterogeneous IOMMUs on a single system. This not only disallows boot time replacement of respective indirect calls (handling of which is the main goal of the introduction here), but more generally disallows calls using the iommu_ops() return value directly - all such calls need to have means (commonly a domain pointer) to know the targeted IOMMU. Disallow all hooks lacking such context for the time being, which in effect is some dead code elimination for ARM. Once extended suitably, individual of these hooks can be moved out of their guards again in the future. >>> >>> While in theory it is possible to have platform with hetereneous IOMMUs. >>>I don't see such such support coming in Xen for the foreseeable >>> future. Note that even Linux does not support such case. >>> >>> This patch is going to make more complicate to unshare page-tables as >>> now we would need to care of mixed case. So I would rather not set >>> IOMMU_MIXED on Arm until we have a use case for it. >> >> Interesting. I essence this is the exact opposite of what you've >> told me when I inquired about indirect call patching of the IOMMU >> code. The sole purpose of this new option is to have a key off of >> which I can tell whether to use patchable indirect calls or plain >> ones. > > I don't think I am saying the opposite. I opened the door for mixed use > case. It does not mean, I want to see half of the helpers dropped on Arm > because you want them to be patchable. There are other way to do it. I drop no helpers (or really hooks), I merely hide ones which can't (currently) be used in mixed-IOMMU environments. And the mixed-IOMMU case was what you've used to request that indirect calls here not be patched for ARM. >> I'm also not following how this change would complicate anything >> for you. There's effectively no change for ARM, except for some >> dead code not getting built anymore. > > Well, with your patch free_page_table, resume & co are under > !IOMMU_MIXED. So they are unusable on Arm until we effectively rework > the code to handle mixed case. More likely those callback will be > necessary on Arm before mixed case. So I don't think this patch as such > is what we want for Arm at the moment. > > I would much prefer if you drop IOMMU_MIXED and implement iommu_{v,}call > on Arm as a iommu_ops->fun(...) (i.e no alternative for now). So call > are still not patchable for Arm, yet we still have access to all IOMMU > functions. Well, that's what I would have done if you hadn't brought up the mixed-IOMMU case - clearly, without being able to test, I wouldn't have dared to try and implement patching of indirect calls for ARM. I can certainly drop that patch, but in the end it means you've made me do more work than would have been needed to reach the immediate goal I have. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
Hi, On 02/10/2018 11:42, Jan Beulich wrote: On 02.10.18 at 12:38, wrote: On 02/10/2018 11:18, Jan Beulich wrote: ARM is intended to gain support for heterogeneous IOMMUs on a single system. This not only disallows boot time replacement of respective indirect calls (handling of which is the main goal of the introduction here), but more generally disallows calls using the iommu_ops() return value directly - all such calls need to have means (commonly a domain pointer) to know the targeted IOMMU. Disallow all hooks lacking such context for the time being, which in effect is some dead code elimination for ARM. Once extended suitably, individual of these hooks can be moved out of their guards again in the future. While in theory it is possible to have platform with hetereneous IOMMUs. I don't see such such support coming in Xen for the foreseeable future. Note that even Linux does not support such case. This patch is going to make more complicate to unshare page-tables as now we would need to care of mixed case. So I would rather not set IOMMU_MIXED on Arm until we have a use case for it. Interesting. I essence this is the exact opposite of what you've told me when I inquired about indirect call patching of the IOMMU code. The sole purpose of this new option is to have a key off of which I can tell whether to use patchable indirect calls or plain ones. I don't think I am saying the opposite. I opened the door for mixed use case. It does not mean, I want to see half of the helpers dropped on Arm because you want them to be patchable. There are other way to do it. I'm also not following how this change would complicate anything for you. There's effectively no change for ARM, except for some dead code not getting built anymore. Well, with your patch free_page_table, resume & co are under !IOMMU_MIXED. So they are unusable on Arm until we effectively rework the code to handle mixed case. More likely those callback will be necessary on Arm before mixed case. So I don't think this patch as such is what we want for Arm at the moment. I would much prefer if you drop IOMMU_MIXED and implement iommu_{v,}call on Arm as a iommu_ops->fun(...) (i.e no alternative for now). So call are still not patchable for Arm, yet we still have access to all IOMMU functions. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
>>> On 02.10.18 at 12:38, wrote: > On 02/10/2018 11:18, Jan Beulich wrote: >> ARM is intended to gain support for heterogeneous IOMMUs on a single >> system. This not only disallows boot time replacement of respective >> indirect calls (handling of which is the main goal of the introduction >> here), but more generally disallows calls using the iommu_ops() return >> value directly - all such calls need to have means (commonly a domain >> pointer) to know the targeted IOMMU. >> >> Disallow all hooks lacking such context for the time being, which in >> effect is some dead code elimination for ARM. Once extended suitably, >> individual of these hooks can be moved out of their guards again in the >> future. > > While in theory it is possible to have platform with hetereneous IOMMUs. > I don't see such such support coming in Xen for the foreseeable > future. Note that even Linux does not support such case. > > This patch is going to make more complicate to unshare page-tables as > now we would need to care of mixed case. So I would rather not set > IOMMU_MIXED on Arm until we have a use case for it. Interesting. I essence this is the exact opposite of what you've told me when I inquired about indirect call patching of the IOMMU code. The sole purpose of this new option is to have a key off of which I can tell whether to use patchable indirect calls or plain ones. I'm also not following how this change would complicate anything for you. There's effectively no change for ARM, except for some dead code not getting built anymore. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
Hi, On 02/10/2018 11:18, Jan Beulich wrote: ARM is intended to gain support for heterogeneous IOMMUs on a single system. This not only disallows boot time replacement of respective indirect calls (handling of which is the main goal of the introduction here), but more generally disallows calls using the iommu_ops() return value directly - all such calls need to have means (commonly a domain pointer) to know the targeted IOMMU. Disallow all hooks lacking such context for the time being, which in effect is some dead code elimination for ARM. Once extended suitably, individual of these hooks can be moved out of their guards again in the future. While in theory it is possible to have platform with hetereneous IOMMUs. I don't see such such support coming in Xen for the foreseeable future. Note that even Linux does not support such case. This patch is going to make more complicate to unshare page-tables as now we would need to care of mixed case. So I would rather not set IOMMU_MIXED on Arm until we have a use case for it. Cheers, Signed-off-by: Jan Beulich --- v4: New. --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -19,6 +19,7 @@ config ARM select HAS_DEVICE_TREE select HAS_PASSTHROUGH select HAS_PDX + select IOMMU_MIXED config ARCH_DEFCONFIG string --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -938,7 +938,7 @@ static int construct_memop_from_reservat return 0; } -#ifdef CONFIG_HAS_PASSTHROUGH +#if defined(CONFIG_HAS_PASSTHROUGH) && !defined(CONFIG_IOMMU_MIXED) struct get_reserved_device_memory { struct xen_reserved_device_memory_map map; unsigned int used_entries; @@ -1550,7 +1550,7 @@ long do_memory_op(unsigned long cmd, XEN break; } -#ifdef CONFIG_HAS_PASSTHROUGH +#if defined(CONFIG_HAS_PASSTHROUGH) && !defined(CONFIG_IOMMU_MIXED) case XENMEM_reserved_device_memory_map: { struct get_reserved_device_memory grdm; --- a/xen/drivers/passthrough/Kconfig +++ b/xen/drivers/passthrough/Kconfig @@ -2,6 +2,9 @@ config HAS_PASSTHROUGH bool +config IOMMU_MIXED + bool + if ARM config ARM_SMMU bool "ARM SMMUv1 and v2 driver" --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -77,9 +77,11 @@ bool_t __read_mostly amd_iommu_perdev_in DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb); +#ifndef CONFIG_IOMMU_MIXED DEFINE_SPINLOCK(iommu_pt_cleanup_lock); PAGE_LIST_HEAD(iommu_pt_cleanup_list); static struct tasklet iommu_pt_cleanup_tasklet; +#endif static int __init parse_iommu_param(const char *s) { @@ -246,7 +248,9 @@ void iommu_teardown(struct domain *d) d->need_iommu = 0; hd->platform_ops->teardown(d); +#ifndef CONFIG_IOMMU_MIXED tasklet_schedule(&iommu_pt_cleanup_tasklet); +#endif } int iommu_construct(struct domain *d) @@ -332,6 +336,7 @@ int iommu_unmap_page(struct domain *d, u return rc; } +#ifndef CONFIG_IOMMU_MIXED static void iommu_free_pagetables(unsigned long unused) { do { @@ -348,6 +353,7 @@ static void iommu_free_pagetables(unsign tasklet_schedule_on_cpu(&iommu_pt_cleanup_tasklet, cpumask_cycle(smp_processor_id(), &cpu_online_map)); } +#endif int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) @@ -433,12 +439,15 @@ int __init iommu_setup(void) iommu_hwdom_passthrough ? "Passthrough" : iommu_hwdom_strict ? "Strict" : "Relaxed"); printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis"); +#ifndef CONFIG_IOMMU_MIXED tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, 0); +#endif } return rc; } +#ifndef CONFIG_IOMMU_MIXED int iommu_suspend() { if ( iommu_enabled ) @@ -453,27 +462,6 @@ void iommu_resume() iommu_get_ops()->resume(); } -int iommu_do_domctl( -struct xen_domctl *domctl, struct domain *d, -XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) -{ -int ret = -ENODEV; - -if ( !iommu_enabled ) -return -ENOSYS; - -#ifdef CONFIG_HAS_PCI -ret = iommu_do_pci_domctl(domctl, d, u_domctl); -#endif - -#ifdef CONFIG_HAS_DEVICE_TREE -if ( ret == -ENODEV ) -ret = iommu_do_dt_domctl(domctl, d, u_domctl); -#endif - -return ret; -} - void iommu_share_p2m_table(struct domain* d) { if ( iommu_enabled && iommu_use_hap_pt(d) ) @@ -500,6 +488,28 @@ int iommu_get_reserved_device_memory(iom return ops->get_reserved_device_memory(func, ctxt); } +#endif + +int iommu_do_domctl( +struct xen_domctl *domctl, struct domain *d, +XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) +{ +int ret = -ENODEV; + +if ( !iommu_enabled ) +return -ENOSYS; + +#ifdef CONFIG_HAS_PCI +ret = iommu_do_pci_domctl(domctl, d, u_domctl); +#endif + +#ifdef CONFIG_HAS_DEVICE_TREE +
[Xen-devel] [PATCH v4 10/12] IOMMU: introduce IOMMU_MIXED config option
ARM is intended to gain support for heterogeneous IOMMUs on a single system. This not only disallows boot time replacement of respective indirect calls (handling of which is the main goal of the introduction here), but more generally disallows calls using the iommu_ops() return value directly - all such calls need to have means (commonly a domain pointer) to know the targeted IOMMU. Disallow all hooks lacking such context for the time being, which in effect is some dead code elimination for ARM. Once extended suitably, individual of these hooks can be moved out of their guards again in the future. Signed-off-by: Jan Beulich --- v4: New. --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -19,6 +19,7 @@ config ARM select HAS_DEVICE_TREE select HAS_PASSTHROUGH select HAS_PDX + select IOMMU_MIXED config ARCH_DEFCONFIG string --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -938,7 +938,7 @@ static int construct_memop_from_reservat return 0; } -#ifdef CONFIG_HAS_PASSTHROUGH +#if defined(CONFIG_HAS_PASSTHROUGH) && !defined(CONFIG_IOMMU_MIXED) struct get_reserved_device_memory { struct xen_reserved_device_memory_map map; unsigned int used_entries; @@ -1550,7 +1550,7 @@ long do_memory_op(unsigned long cmd, XEN break; } -#ifdef CONFIG_HAS_PASSTHROUGH +#if defined(CONFIG_HAS_PASSTHROUGH) && !defined(CONFIG_IOMMU_MIXED) case XENMEM_reserved_device_memory_map: { struct get_reserved_device_memory grdm; --- a/xen/drivers/passthrough/Kconfig +++ b/xen/drivers/passthrough/Kconfig @@ -2,6 +2,9 @@ config HAS_PASSTHROUGH bool +config IOMMU_MIXED + bool + if ARM config ARM_SMMU bool "ARM SMMUv1 and v2 driver" --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -77,9 +77,11 @@ bool_t __read_mostly amd_iommu_perdev_in DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb); +#ifndef CONFIG_IOMMU_MIXED DEFINE_SPINLOCK(iommu_pt_cleanup_lock); PAGE_LIST_HEAD(iommu_pt_cleanup_list); static struct tasklet iommu_pt_cleanup_tasklet; +#endif static int __init parse_iommu_param(const char *s) { @@ -246,7 +248,9 @@ void iommu_teardown(struct domain *d) d->need_iommu = 0; hd->platform_ops->teardown(d); +#ifndef CONFIG_IOMMU_MIXED tasklet_schedule(&iommu_pt_cleanup_tasklet); +#endif } int iommu_construct(struct domain *d) @@ -332,6 +336,7 @@ int iommu_unmap_page(struct domain *d, u return rc; } +#ifndef CONFIG_IOMMU_MIXED static void iommu_free_pagetables(unsigned long unused) { do { @@ -348,6 +353,7 @@ static void iommu_free_pagetables(unsign tasklet_schedule_on_cpu(&iommu_pt_cleanup_tasklet, cpumask_cycle(smp_processor_id(), &cpu_online_map)); } +#endif int iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int page_count) @@ -433,12 +439,15 @@ int __init iommu_setup(void) iommu_hwdom_passthrough ? "Passthrough" : iommu_hwdom_strict ? "Strict" : "Relaxed"); printk("Interrupt remapping %sabled\n", iommu_intremap ? "en" : "dis"); +#ifndef CONFIG_IOMMU_MIXED tasklet_init(&iommu_pt_cleanup_tasklet, iommu_free_pagetables, 0); +#endif } return rc; } +#ifndef CONFIG_IOMMU_MIXED int iommu_suspend() { if ( iommu_enabled ) @@ -453,27 +462,6 @@ void iommu_resume() iommu_get_ops()->resume(); } -int iommu_do_domctl( -struct xen_domctl *domctl, struct domain *d, -XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) -{ -int ret = -ENODEV; - -if ( !iommu_enabled ) -return -ENOSYS; - -#ifdef CONFIG_HAS_PCI -ret = iommu_do_pci_domctl(domctl, d, u_domctl); -#endif - -#ifdef CONFIG_HAS_DEVICE_TREE -if ( ret == -ENODEV ) -ret = iommu_do_dt_domctl(domctl, d, u_domctl); -#endif - -return ret; -} - void iommu_share_p2m_table(struct domain* d) { if ( iommu_enabled && iommu_use_hap_pt(d) ) @@ -500,6 +488,28 @@ int iommu_get_reserved_device_memory(iom return ops->get_reserved_device_memory(func, ctxt); } +#endif + +int iommu_do_domctl( +struct xen_domctl *domctl, struct domain *d, +XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) +{ +int ret = -ENODEV; + +if ( !iommu_enabled ) +return -ENOSYS; + +#ifdef CONFIG_HAS_PCI +ret = iommu_do_pci_domctl(domctl, d, u_domctl); +#endif + +#ifdef CONFIG_HAS_DEVICE_TREE +if ( ret == -ENODEV ) +ret = iommu_do_dt_domctl(domctl, d, u_domctl); +#endif + +return ret; +} bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature) { --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -147,7 +147,7 @@ struct iommu_ops { int (*assign_device)(struct domain *, u8 devfn, device_t *dev, u32 flag); int (*reassign_device)(struct domain *s, struct domain *t, u8 devfn, device_t *dev); -#ifdef CONFIG_HAS_PCI +#if defined(CONFIG_HAS_PC