* [PATCH 0/2] IOMMU: use ioremap() @ 2013-07-10 10:36 Jan Beulich 2013-07-10 10:48 ` [PATCH 1/2] VT-d: " Jan Beulich 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Jan Beulich @ 2013-07-10 10:36 UTC (permalink / raw) To: xen-devel; +Cc: Jacob Shin, xiantao.zhang, suravee.suthikulpanit Both AMD IOMMU and VT-d code used iounmap() in their error cleanup path for something not established via ioremap(), leading to crashes if those error paths actually get used. 1: VT-d: use ioremap() 2: AMD IOMMU: use ioremap() Signed-off-by: Jan Beulich <jbeulich@suse.com> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] VT-d: use ioremap() 2013-07-10 10:36 [PATCH 0/2] IOMMU: use ioremap() Jan Beulich @ 2013-07-10 10:48 ` Jan Beulich 2013-07-10 11:37 ` Keir Fraser 2013-07-11 8:19 ` [PATCH v2 " Jan Beulich 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich 1 sibling, 2 replies; 10+ messages in thread From: Jan Beulich @ 2013-07-10 10:48 UTC (permalink / raw) To: xen-devel; +Cc: Jacob Shin, xiantao.zhang, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 1999 bytes --] There's no point in using the fixmap here, and it gets iommu_alloc() in line with iommu_free(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,8 +127,6 @@ do { } \ } while (0) -void *map_to_nocache_virt(int nr_iommus, u64 maddr); - int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_usb_device(u16 seg, u8 bus, u8 devfn); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1143,7 +1143,7 @@ int __init iommu_alloc(struct acpi_drhd_ } iommu->intel->drhd = drhd; - iommu->reg = map_to_nocache_virt(nr_iommus, drhd->address); + iommu->reg = ioremap(drhd->address, PAGE_SIZE); iommu->index = nr_iommus++; iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG); --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -64,12 +64,6 @@ void flush_all_cache() wbinvd(); } -void *__init map_to_nocache_virt(int nr_iommus, u64 maddr) -{ - set_fixmap_nocache(FIX_IOMMU_REGS_BASE_0 + nr_iommus, maddr); - return (void *)fix_to_virt(FIX_IOMMU_REGS_BASE_0 + nr_iommus); -} - static int _hvm_dpci_isairq_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_REGS_BASE_0, - FIX_IOMMU_REGS_END = FIX_IOMMU_REGS_BASE_0 + MAX_IOMMUS-1, FIX_IOMMU_MMIO_BASE_0, FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, [-- Attachment #2: VT-d-use-ioremap.patch --] [-- Type: text/plain, Size: 2016 bytes --] VT-d: use ioremap() There's no point in using the fixmap here, and it gets iommu_alloc() in line with iommu_free(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/vtd/dmar.h +++ b/xen/drivers/passthrough/vtd/dmar.h @@ -127,8 +127,6 @@ do { } \ } while (0) -void *map_to_nocache_virt(int nr_iommus, u64 maddr); - int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_usb_device(u16 seg, u8 bus, u8 devfn); --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1143,7 +1143,7 @@ int __init iommu_alloc(struct acpi_drhd_ } iommu->intel->drhd = drhd; - iommu->reg = map_to_nocache_virt(nr_iommus, drhd->address); + iommu->reg = ioremap(drhd->address, PAGE_SIZE); iommu->index = nr_iommus++; iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG); --- a/xen/drivers/passthrough/vtd/x86/vtd.c +++ b/xen/drivers/passthrough/vtd/x86/vtd.c @@ -64,12 +64,6 @@ void flush_all_cache() wbinvd(); } -void *__init map_to_nocache_virt(int nr_iommus, u64 maddr) -{ - set_fixmap_nocache(FIX_IOMMU_REGS_BASE_0 + nr_iommus, maddr); - return (void *)fix_to_virt(FIX_IOMMU_REGS_BASE_0 + nr_iommus); -} - static int _hvm_dpci_isairq_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_REGS_BASE_0, - FIX_IOMMU_REGS_END = FIX_IOMMU_REGS_BASE_0 + MAX_IOMMUS-1, FIX_IOMMU_MMIO_BASE_0, FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] VT-d: use ioremap() 2013-07-10 10:48 ` [PATCH 1/2] VT-d: " Jan Beulich @ 2013-07-10 11:37 ` Keir Fraser 2013-07-11 8:19 ` [PATCH v2 " Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Keir Fraser @ 2013-07-10 11:37 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Jacob Shin, suravee.suthikulpanit, xiantao.zhang On 10/07/2013 11:48, "Jan Beulich" <JBeulich@suse.com> wrote: > There's no point in using the fixmap here, and it gets iommu_alloc() > in line with iommu_free(), which was already using iounmap() (thus > crashing if actually used). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] VT-d: use ioremap() 2013-07-10 10:48 ` [PATCH 1/2] VT-d: " Jan Beulich 2013-07-10 11:37 ` Keir Fraser @ 2013-07-11 8:19 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2013-07-11 8:19 UTC (permalink / raw) To: xen-devel; +Cc: Jacob Shin, xiantao.zhang, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 2737 bytes --] There's no point in using the fixmap here, and it gets iommu_alloc() in line with iommu_free(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> --- v2: Add error handling to the ioremap() invocation. --- 2013-06-21.orig/xen/drivers/passthrough/vtd/dmar.h 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/dmar.h 2013-07-10 11:16:27.000000000 +0200 @@ -127,8 +127,6 @@ do { } \ } while (0) -void *map_to_nocache_virt(int nr_iommus, u64 maddr); - int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_usb_device(u16 seg, u8 bus, u8 devfn); --- 2013-06-21.orig/xen/drivers/passthrough/vtd/iommu.c 2013-07-10 11:29:34.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/iommu.c 2013-07-11 10:09:36.000000000 +0200 @@ -1142,15 +1142,16 @@ int __init iommu_alloc(struct acpi_drhd_ return -ENOMEM; } iommu->intel->drhd = drhd; + drhd->iommu = iommu; - iommu->reg = map_to_nocache_virt(nr_iommus, drhd->address); + iommu->reg = ioremap(drhd->address, PAGE_SIZE); + if ( !iommu->reg ) + return -ENOMEM; iommu->index = nr_iommus++; iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG); iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG); - drhd->iommu = iommu; - if ( iommu_verbose ) { dprintk(VTDPREFIX, --- 2013-06-21.orig/xen/drivers/passthrough/vtd/x86/vtd.c 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/x86/vtd.c 2013-07-10 11:16:14.000000000 +0200 @@ -64,12 +64,6 @@ void flush_all_cache() wbinvd(); } -void *__init map_to_nocache_virt(int nr_iommus, u64 maddr) -{ - set_fixmap_nocache(FIX_IOMMU_REGS_BASE_0 + nr_iommus, maddr); - return (void *)fix_to_virt(FIX_IOMMU_REGS_BASE_0 + nr_iommus); -} - static int _hvm_dpci_isairq_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { --- 2013-06-21.orig/xen/include/asm-x86/fixmap.h 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/include/asm-x86/fixmap.h 2013-07-10 11:16:00.000000000 +0200 @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_REGS_BASE_0, - FIX_IOMMU_REGS_END = FIX_IOMMU_REGS_BASE_0 + MAX_IOMMUS-1, FIX_IOMMU_MMIO_BASE_0, FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, [-- Attachment #2: VT-d-use-ioremap.patch --] [-- Type: text/plain, Size: 2754 bytes --] VT-d: use ioremap() There's no point in using the fixmap here, and it gets iommu_alloc() in line with iommu_free(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> --- v2: Add error handling to the ioremap() invocation. --- 2013-06-21.orig/xen/drivers/passthrough/vtd/dmar.h 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/dmar.h 2013-07-10 11:16:27.000000000 +0200 @@ -127,8 +127,6 @@ do { } \ } while (0) -void *map_to_nocache_virt(int nr_iommus, u64 maddr); - int vtd_hw_check(void); void disable_pmr(struct iommu *iommu); int is_usb_device(u16 seg, u8 bus, u8 devfn); --- 2013-06-21.orig/xen/drivers/passthrough/vtd/iommu.c 2013-07-10 11:29:34.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/iommu.c 2013-07-11 10:09:36.000000000 +0200 @@ -1142,15 +1142,16 @@ int __init iommu_alloc(struct acpi_drhd_ return -ENOMEM; } iommu->intel->drhd = drhd; + drhd->iommu = iommu; - iommu->reg = map_to_nocache_virt(nr_iommus, drhd->address); + iommu->reg = ioremap(drhd->address, PAGE_SIZE); + if ( !iommu->reg ) + return -ENOMEM; iommu->index = nr_iommus++; iommu->cap = dmar_readq(iommu->reg, DMAR_CAP_REG); iommu->ecap = dmar_readq(iommu->reg, DMAR_ECAP_REG); - drhd->iommu = iommu; - if ( iommu_verbose ) { dprintk(VTDPREFIX, --- 2013-06-21.orig/xen/drivers/passthrough/vtd/x86/vtd.c 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/drivers/passthrough/vtd/x86/vtd.c 2013-07-10 11:16:14.000000000 +0200 @@ -64,12 +64,6 @@ void flush_all_cache() wbinvd(); } -void *__init map_to_nocache_virt(int nr_iommus, u64 maddr) -{ - set_fixmap_nocache(FIX_IOMMU_REGS_BASE_0 + nr_iommus, maddr); - return (void *)fix_to_virt(FIX_IOMMU_REGS_BASE_0 + nr_iommus); -} - static int _hvm_dpci_isairq_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, void *arg) { --- 2013-06-21.orig/xen/include/asm-x86/fixmap.h 2013-07-11 10:05:51.000000000 +0200 +++ 2013-06-21/xen/include/asm-x86/fixmap.h 2013-07-10 11:16:00.000000000 +0200 @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_REGS_BASE_0, - FIX_IOMMU_REGS_END = FIX_IOMMU_REGS_BASE_0 + MAX_IOMMUS-1, FIX_IOMMU_MMIO_BASE_0, FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] AMD IOMMU: use ioremap() 2013-07-10 10:36 [PATCH 0/2] IOMMU: use ioremap() Jan Beulich 2013-07-10 10:48 ` [PATCH 1/2] VT-d: " Jan Beulich @ 2013-07-10 10:49 ` Jan Beulich 2013-07-10 11:37 ` Keir Fraser ` (2 more replies) 1 sibling, 3 replies; 10+ messages in thread From: Jan Beulich @ 2013-07-10 10:49 UTC (permalink / raw) To: xen-devel; +Cc: Jacob Shin, xiantao.zhang, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 2216 bytes --] There's no point in using the fixmap here, and it gets map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ static int __init map_iommu_mmio_region(struct amd_iommu *iommu) { - unsigned long mfn; - - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) - { - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); + iommu->mmio_base = ioremap(iommu->mmio_base_phys, + IOMMU_MMIO_REGION_LENGTH); + if ( iommu->mmio_base ) return -ENOMEM; - } - - iommu->mmio_base = (void *)fix_to_virt( - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_MMIO_BASE_0, - FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, FIX_MSIX_IO_RESERV_BASE, FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -464,10 +464,7 @@ #define IOMMU_CONTROL_DISABLED 0 #define IOMMU_CONTROL_ENABLED 1 -#define MMIO_PAGES_PER_IOMMU (IOMMU_MMIO_REGION_LENGTH / PAGE_SIZE_4K) -#define IOMMU_PAGES (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS) #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 -#define MAX_AMD_IOMMUS 32 /* interrupt remapping table */ #define INT_REMAP_INDEX_DM_MASK 0x1C00 [-- Attachment #2: AMD-IOMMU-use-ioremap.patch --] [-- Type: text/plain, Size: 2238 bytes --] AMD IOMMU: use ioremap() There's no point in using the fixmap here, and it gets map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ static int __init map_iommu_mmio_region(struct amd_iommu *iommu) { - unsigned long mfn; - - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) - { - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); + iommu->mmio_base = ioremap(iommu->mmio_base_phys, + IOMMU_MMIO_REGION_LENGTH); + if ( iommu->mmio_base ) return -ENOMEM; - } - - iommu->mmio_base = (void *)fix_to_virt( - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_MMIO_BASE_0, - FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, FIX_MSIX_IO_RESERV_BASE, FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -464,10 +464,7 @@ #define IOMMU_CONTROL_DISABLED 0 #define IOMMU_CONTROL_ENABLED 1 -#define MMIO_PAGES_PER_IOMMU (IOMMU_MMIO_REGION_LENGTH / PAGE_SIZE_4K) -#define IOMMU_PAGES (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS) #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 -#define MAX_AMD_IOMMUS 32 /* interrupt remapping table */ #define INT_REMAP_INDEX_DM_MASK 0x1C00 [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] AMD IOMMU: use ioremap() 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich @ 2013-07-10 11:37 ` Keir Fraser 2013-07-10 17:12 ` Suravee Suthikulanit 2013-07-11 8:22 ` [PATCH v2 " Jan Beulich 2 siblings, 0 replies; 10+ messages in thread From: Keir Fraser @ 2013-07-10 11:37 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Jacob Shin, suravee.suthikulpanit, xiantao.zhang On 10/07/2013 11:49, "Jan Beulich" <JBeulich@suse.com> wrote: > There's no point in using the fixmap here, and it gets > map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which > was already using iounmap() (thus crashing if actually used). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] AMD IOMMU: use ioremap() 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich 2013-07-10 11:37 ` Keir Fraser @ 2013-07-10 17:12 ` Suravee Suthikulanit 2013-07-11 8:14 ` Jan Beulich 2013-07-11 8:22 ` [PATCH v2 " Jan Beulich 2 siblings, 1 reply; 10+ messages in thread From: Suravee Suthikulanit @ 2013-07-10 17:12 UTC (permalink / raw) To: Jan Beulich; +Cc: Jacob Shin, xiantao.zhang, xen-devel On 7/10/2013 5:49 AM, Jan Beulich wrote: > There's no point in using the fixmap here, and it gets > map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which > was already using iounmap() (thus crashing if actually used). > > Signed-off-by: Jan Beulich<jbeulich@suse.com> > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ > > static int __init map_iommu_mmio_region(struct amd_iommu *iommu) > { > - unsigned long mfn; > - > - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) > - { > - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); > + iommu->mmio_base = ioremap(iommu->mmio_base_phys, > + IOMMU_MMIO_REGION_LENGTH); > + if ( iommu->mmio_base ) This should have been "if ( !iommu->mmio_base )". > return -ENOMEM; > - } > - > - iommu->mmio_base = (void *)fix_to_virt( > - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); > - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); > - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, > - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); > > memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); Once I changed the above logic, IOMMU is initialized correctly. I have tested pass-though a NIC and thing looks fine. Suravee ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] AMD IOMMU: use ioremap() 2013-07-10 17:12 ` Suravee Suthikulanit @ 2013-07-11 8:14 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2013-07-11 8:14 UTC (permalink / raw) To: Suravee Suthikulanit; +Cc: Jacob Shin, xiantao.zhang, xen-devel >>> On 10.07.13 at 19:12, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote: > On 7/10/2013 5:49 AM, Jan Beulich wrote: >> There's no point in using the fixmap here, and it gets >> map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which >> was already using iounmap() (thus crashing if actually used). >> >> Signed-off-by: Jan Beulich<jbeulich@suse.com> >> >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ >> >> static int __init map_iommu_mmio_region(struct amd_iommu *iommu) >> { >> - unsigned long mfn; >> - >> - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) >> - { >> - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); >> + iommu->mmio_base = ioremap(iommu->mmio_base_phys, >> + IOMMU_MMIO_REGION_LENGTH); >> + if ( iommu->mmio_base ) > This should have been "if ( !iommu->mmio_base )". Oops, of course. While changing this I also noticed that the corresponding VT-d code had no error checking at all, so I'll send a v2 for both. >> return -ENOMEM; >> - } >> - >> - iommu->mmio_base = (void *)fix_to_virt( >> - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); >> - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); >> - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, >> - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); >> >> memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); > Once I changed the above logic, IOMMU is initialized correctly. I have > tested pass-though a NIC and thing looks fine. Please explicitly send an ack (or which other tags to apply) for the v2 that I'll send out in a minute. Thanks, Jan ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] AMD IOMMU: use ioremap() 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich 2013-07-10 11:37 ` Keir Fraser 2013-07-10 17:12 ` Suravee Suthikulanit @ 2013-07-11 8:22 ` Jan Beulich 2013-07-11 14:05 ` Suravee Suthikulanit 2 siblings, 1 reply; 10+ messages in thread From: Jan Beulich @ 2013-07-11 8:22 UTC (permalink / raw) To: xen-devel; +Cc: Jacob Shin, xiantao.zhang, suravee.suthikulpanit [-- Attachment #1: Type: text/plain, Size: 2325 bytes --] There's no point in using the fixmap here, and it gets map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> --- v2: Fix polarity of error check (thanks for noticing, Suravee). --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ static int __init map_iommu_mmio_region(struct amd_iommu *iommu) { - unsigned long mfn; - - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) - { - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); + iommu->mmio_base = ioremap(iommu->mmio_base_phys, + IOMMU_MMIO_REGION_LENGTH); + if ( !iommu->mmio_base ) return -ENOMEM; - } - - iommu->mmio_base = (void *)fix_to_virt( - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_MMIO_BASE_0, - FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, FIX_MSIX_IO_RESERV_BASE, FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -464,10 +464,7 @@ #define IOMMU_CONTROL_DISABLED 0 #define IOMMU_CONTROL_ENABLED 1 -#define MMIO_PAGES_PER_IOMMU (IOMMU_MMIO_REGION_LENGTH / PAGE_SIZE_4K) -#define IOMMU_PAGES (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS) #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 -#define MAX_AMD_IOMMUS 32 /* interrupt remapping table */ #define INT_REMAP_INDEX_DM_MASK 0x1C00 [-- Attachment #2: AMD-IOMMU-use-ioremap.patch --] [-- Type: text/plain, Size: 2347 bytes --] AMD IOMMU: use ioremap() There's no point in using the fixmap here, and it gets map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which was already using iounmap() (thus crashing if actually used). Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Keir Fraser <keir@xen.org> --- v2: Fix polarity of error check (thanks for noticing, Suravee). --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ static int __init map_iommu_mmio_region(struct amd_iommu *iommu) { - unsigned long mfn; - - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) - { - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); + iommu->mmio_base = ioremap(iommu->mmio_base_phys, + IOMMU_MMIO_REGION_LENGTH); + if ( !iommu->mmio_base ) return -ENOMEM; - } - - iommu->mmio_base = (void *)fix_to_virt( - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); --- a/xen/include/asm-x86/fixmap.h +++ b/xen/include/asm-x86/fixmap.h @@ -60,8 +60,6 @@ enum fixed_addresses { FIX_KEXEC_BASE_0, FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, - FIX_IOMMU_MMIO_BASE_0, - FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, FIX_TBOOT_SHARED_BASE, FIX_MSIX_IO_RESERV_BASE, FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h @@ -464,10 +464,7 @@ #define IOMMU_CONTROL_DISABLED 0 #define IOMMU_CONTROL_ENABLED 1 -#define MMIO_PAGES_PER_IOMMU (IOMMU_MMIO_REGION_LENGTH / PAGE_SIZE_4K) -#define IOMMU_PAGES (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS) #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 -#define MAX_AMD_IOMMUS 32 /* interrupt remapping table */ #define INT_REMAP_INDEX_DM_MASK 0x1C00 [-- Attachment #3: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] AMD IOMMU: use ioremap() 2013-07-11 8:22 ` [PATCH v2 " Jan Beulich @ 2013-07-11 14:05 ` Suravee Suthikulanit 0 siblings, 0 replies; 10+ messages in thread From: Suravee Suthikulanit @ 2013-07-11 14:05 UTC (permalink / raw) To: Jan Beulich; +Cc: Jacob Shin, xiantao.zhang, xen-devel On 7/11/2013 3:22 AM, Jan Beulich wrote: > There's no point in using the fixmap here, and it gets > map_iommu_mmio_region() in line with unmap_iommu_mmio_region(), which > was already using iounmap() (thus crashing if actually used). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Keir Fraser <keir@xen.org> Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > v2: Fix polarity of error check (thanks for noticing, Suravee). > > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -48,19 +48,10 @@ static int iommu_has_ht_flag(struct amd_ > > static int __init map_iommu_mmio_region(struct amd_iommu *iommu) > { > - unsigned long mfn; > - > - if ( nr_amd_iommus > MAX_AMD_IOMMUS ) > - { > - AMD_IOMMU_DEBUG("nr_amd_iommus %d > MAX_IOMMUS\n", nr_amd_iommus); > + iommu->mmio_base = ioremap(iommu->mmio_base_phys, > + IOMMU_MMIO_REGION_LENGTH); > + if ( !iommu->mmio_base ) > return -ENOMEM; > - } > - > - iommu->mmio_base = (void *)fix_to_virt( > - FIX_IOMMU_MMIO_BASE_0 + nr_amd_iommus * MMIO_PAGES_PER_IOMMU); > - mfn = (unsigned long)(iommu->mmio_base_phys >> PAGE_SHIFT); > - map_pages_to_xen((unsigned long)iommu->mmio_base, mfn, > - MMIO_PAGES_PER_IOMMU, PAGE_HYPERVISOR_NOCACHE); > > memset(iommu->mmio_base, 0, IOMMU_MMIO_REGION_LENGTH); > > --- a/xen/include/asm-x86/fixmap.h > +++ b/xen/include/asm-x86/fixmap.h > @@ -60,8 +60,6 @@ enum fixed_addresses { > FIX_KEXEC_BASE_0, > FIX_KEXEC_BASE_END = FIX_KEXEC_BASE_0 \ > + ((KEXEC_XEN_NO_PAGES >> 1) * KEXEC_IMAGE_NR) - 1, > - FIX_IOMMU_MMIO_BASE_0, > - FIX_IOMMU_MMIO_END = FIX_IOMMU_MMIO_BASE_0 + IOMMU_PAGES -1, > FIX_TBOOT_SHARED_BASE, > FIX_MSIX_IO_RESERV_BASE, > FIX_MSIX_IO_RESERV_END = FIX_MSIX_IO_RESERV_BASE + FIX_MSIX_MAX_PAGES -1, > --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h > +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h > @@ -464,10 +464,7 @@ > #define IOMMU_CONTROL_DISABLED 0 > #define IOMMU_CONTROL_ENABLED 1 > > -#define MMIO_PAGES_PER_IOMMU (IOMMU_MMIO_REGION_LENGTH / PAGE_SIZE_4K) > -#define IOMMU_PAGES (MMIO_PAGES_PER_IOMMU * MAX_AMD_IOMMUS) > #define DEFAULT_DOMAIN_ADDRESS_WIDTH 48 > -#define MAX_AMD_IOMMUS 32 > > /* interrupt remapping table */ > #define INT_REMAP_INDEX_DM_MASK 0x1C00 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-07-11 14:05 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-10 10:36 [PATCH 0/2] IOMMU: use ioremap() Jan Beulich 2013-07-10 10:48 ` [PATCH 1/2] VT-d: " Jan Beulich 2013-07-10 11:37 ` Keir Fraser 2013-07-11 8:19 ` [PATCH v2 " Jan Beulich 2013-07-10 10:49 ` [PATCH 2/2] AMD IOMMU: " Jan Beulich 2013-07-10 11:37 ` Keir Fraser 2013-07-10 17:12 ` Suravee Suthikulanit 2013-07-11 8:14 ` Jan Beulich 2013-07-11 8:22 ` [PATCH v2 " Jan Beulich 2013-07-11 14:05 ` Suravee Suthikulanit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.