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>
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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>
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>
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
>>> 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
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/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 > > >