ARM: shmobile: ipmmu: Add basic PMB support
PATCH 1/1 ARM: shmobile: ipmmu: Add basic PMB support
The PMB can be used to remap 16, 64, 128 or 512 MiB pages from the 0x80000000-0xffffffff address range to anywhere in the 0x00000000-0x7fffffff range.
It also has the ability to perform tiled-linear address translation, which can be used to access a memory buffer as a series of n x m tiles, useful for image encoding/decoding.
Currently only the userspace API via character device is supported. All register access and hardware dependent functionality is provided by the IPMMU driver, which is shared with the IOMMU/TLB module.
---
This patch must be applied on top of the IPMMU patch series by Hideki EIRAKU. The code has been placed in the drivers/iommu directory for two reasons:
1) The PMB also performs hardware address translation
2) Since the PMB shares the same register address range as the shmobile IOMMU, the two functions are accessed through the same platform device, the driver for which is in drivers/iommu
drivers/iommu/Kconfig | 14 ++ drivers/iommu/Makefile | 1 + drivers/iommu/shmobile-ipmmu.c | 117 ++++++++++++- drivers/iommu/shmobile-ipmmu.h | 28 +++- drivers/iommu/shmobile-pmb.c | 362 ++++++++++++++++++++++++++++++++++++++++ include/linux/ipmmu.h | 29 ++++ 6 files changed, 544 insertions(+), 7 deletions(-) create mode 100644 drivers/iommu/shmobile-pmb.c create mode 100644 include/linux/ipmmu.h
drivers/iommu/Kconfig +14 -0
@@ -261,4 +261,18 @@ config SHMOBILE_IOMMU_L1SIZE default 256 if SHMOBILE_IOMMU_ADDRSIZE_64MB default 128 if SHMOBILE_IOMMU_ADDRSIZE_32MB +config SHMOBILE_PMB + bool "IPMMU PMB driver" + default n + select SHMOBILE_IPMMU + help + This enables the PMB interface of the IPMMU hardware module. + The PMB can be used to remap 16, 64, 128 or 512 MiB pages from + the 0x80000000-0xffffffff address range to anywhere in the + 0x00000000-0x7fffffff range. + PMB support can be used either with or without SHMOBILE IOMMU + support. + + If unsure, say N. + endif # IOMMU_SUPPORT
drivers/iommu/Makefile +1 -0
@@ -15,3 +15,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o obj-$(CONFIG_SHMOBILE_IOMMU) += shmobile-iommu.o obj-$(CONFIG_SHMOBILE_IPMMU) += shmobile-ipmmu.o +obj-$(CONFIG_SHMOBILE_PMB) += shmobile-pmb.o
drivers/iommu/shmobile-ipmmu.c +111 -6
138 lines not on this page · raw message
drivers/iommu/shmobile-ipmmu.h +27 -1
@@ -11,6 +11,8 @@#define __SHMOBILE_IPMMU_H__struct dma_iommu_mapping ; +struct pmb_tile_info ; +struct ipmmu_pmb_info ;struct shmobile_ipmmu {struct device *dev;
@@ -20,10 +22,12 @@ struct shmobile_ipmmu {struct dma_iommu_mapping *iommu_mapping;const char *const *dev_names;unsigned int num_dev_names; +void *pmb_priv; }; -#ifdef CONFIG_SHMOBILE_IPMMU_TLB void ipmmu_tlb_flush (struct shmobile_ipmmu *ipmmu); + +#ifdef CONFIG_SHMOBILE_IPMMU_TLB void ipmmu_tlb_set (struct shmobile_ipmmu *ipmmu,unsigned long phys,int size,int asid);int ipmmu_iommu_init (struct shmobile_ipmmu *ipmmu);
@@ -34,4 +38,26 @@ static int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu) }#endif +#ifdef CONFIG_SHMOBILE_PMB +/* Access functions used by PMB device */ +/*void handle_free(struct device *dev, unsigned long handle, int size_mb); +unsigned long handle_alloc(struct device *dev, int size_mb);*/ +int ipmmu_pmb_set_addr (struct shmobile_ipmmu *ipmmu, +int index,unsigned long addr, +int enabled); +int ipmmu_pmb_set_data (struct shmobile_ipmmu *ipmmu,int index, +struct ipmmu_pmb_info *info, +struct pmb_tile_info *tile); +int ipmmu_pmb_enable (struct shmobile_ipmmu *ipmmu,int index); +/* PMB initialization */ +void *ipmmu_pmb_init (struct shmobile_ipmmu *ipmmu); +void ipmmu_pmb_deinit (void *pmb_priv); +#else +static inline void *ipmmu_pmb_init (struct device *dev) +{ +return NULL ; +} +static inline void ipmmu_pmb_deinit (void *pmb_priv) { } +#endif +#endif /* __SHMOBILE_IPMMU_H__ */
drivers/iommu/shmobile-pmb.c new file +362 -0
362 lines not on this page · raw message
include/linux/ipmmu.h new file +29 -0
@@ -0,0 +1,29 @@ +#ifndef __LINUX_IPMMU_PMB_H__ +#define __LINUX_IPMMU_PMB_H__ + +struct ipmmu_pmb_info { +int enabled; +unsigned long paddr; +int size_mb; +}; + +struct pmb_tile_info { +int tile_width; +int tile_height; +int buffer_pitch; +int enabled; +}; + +/* IOCTL commands. */ + +#define IPMMU_SET_PMB _IOW('S', 37, struct ipmmu_pmb_phys *) +#define IPMMU_GET_PMB _IOR('S', 38, struct ipmmu_pmb_phys *) +#define IPMMU_GET_PMB_HANDLE _IOR('S', 39, unsigned long *) +#define IPMMU_SET_PMB_TL _IOW('S', 41, struct ipmmu_pmb_tile_info *) +#define IPMMU_GET_PMB_TL _IOR('S', 42, struct ipmmu_pmb_tile_info *) + +#ifdef __kernel + +#endif /* __kernel */ + +#endif /* __LINUX_IPMMU_PMB_H__ */
Signature · 1 line
1.7.5.4
PATCH 1/1 ARM: shmobile: ipmmu: Add basic PMB support
Hi Damian,
Thank you for the patch.
Quoted 3 lines · On Friday 18 January 2013 15:35:10 Damian Hobson-Garcia wrote:
The PMB can be used to remap 16, 64, 128 or 512 MiB pages from the 0x80000000-0xffffffff address range to anywhere in the 0x00000000-0x7fffffff range.
Isn't it 0x80000000 - 0xbfffffff to 0x00000000 - 0xffffffff ?
Quoted 4 lines
It also has the ability to perform tiled-linear address translation, which can be used to access a memory buffer as a series of n x m tiles, useful for image encoding/decoding.
Currently only the userspace API via character device is supported.
If I understand this correctly, you're allowing userspace to remap a virtual address block to a physical address block without performing any sanity check. Isn't that a major security issue ?
Quoted 101 lines
All register access and hardware dependent functionality is provided by the IPMMU driver, which is shared with the IOMMU/TLB module.
Signed-off-by Damian Hobson-Garcia <[email protected]>---
This patch must be applied on top of the IPMMU patch series by Hideki EIRAKU. The code has been placed in thedrivers/iommudirectory for two reasons:
1) The PMB also performs hardware address translation
2) Since the PMB shares the same register address range as the shmobile IOMMU, the two functions are accessed through the same platform device, the driver for which is indrivers/iommudrivers/iommu/Kconfig | 14 ++ drivers/iommu/Makefile | 1 + drivers/iommu/shmobile-ipmmu.c | 117 ++++++++++++- drivers/iommu/shmobile-ipmmu.h | 28 +++- drivers/iommu/shmobile-pmb.c | 362 +++++++++++++++++++++++++++++++++++++ include/linux/ipmmu.h | 29 ++++ 6 files changed, 544 insertions(+), 7 deletions(-) create mode 100644 drivers/iommu/shmobile-pmb.c create mode 100644 include/linux/ipmmu.hdrivers/iommu/Kconfig +14
-0@@ -261,4 +261,18 @@ config SHMOBILE_IOMMU_L1SIZE default 256 if SHMOBILE_IOMMU_ADDRSIZE_64MB default 128 if SHMOBILE_IOMMU_ADDRSIZE_32MB +config SHMOBILE_PMB + bool "IPMMU PMB driver" + default n + select SHMOBILE_IPMMU + help + This enables the PMB interface of the IPMMU hardware module. + The PMB can be used to remap 16, 64, 128 or 512 MiB pages from + the 0x80000000-0xffffffff address range to anywhere in the + 0x00000000-0x7fffffff range. + PMB support can be used either with or without SHMOBILE IOMMU + support. + + If unsure, say N. + endif # IOMMU_SUPPORTdrivers/iommu/Makefile +1
-0@@ -15,3 +15,4 @@ obj-$(CONFIG_TEGRA_IOMMU_SMMU) += tegra-smmu.o obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o obj-$(CONFIG_SHMOBILE_IOMMU) += shmobile-iommu.o obj-$(CONFIG_SHMOBILE_IPMMU) += shmobile-ipmmu.o +obj-$(CONFIG_SHMOBILE_PMB) += shmobile-pmb.odrivers/iommu/shmobile-ipmmu.c +25
-6@@ -14,15 +14,34 @@#include <linux/slab.h> #include <linux/platform_data/sh_ipmmu.h> #include "shmobile-ipmmu.h" +#include <linux/ipmmu.h> -#define IMCTR1 0x000 -#define IMCTR2 0x004 -#define IMASID 0x010 -#define IMTTBR 0x014 -#define IMTTBCR 0x018 - +#define IMCTR1 0x00#define IMCTR1_TLBEN (1 << 0)#define IMCTR1_FLUSH (1 << 1) +#define IMCTR2 0x04 +#define IMCTR2_PMBEN 0x01 +#define IMASID 0x010 +#define IMTTBR 0x014 +#define IMTTBCR 0x018 +#define IMPMBA_BASE 0x80 +#define IMPBMA_V (1 << 8) +#define IMPMBD_BASE 0xC0 +#define IMPBMD_V (1 << 8) +#define IMPBMD_SZ_16M 0x00 +#define IMPBMD_SZ_64M 0x10 +#define IMPBMD_SZ_128M 0x80 +#define IMPBMD_SZ_512M 0x90 +#define IMPBMD_BV (1 << 9) +#define IMPBMD_TBM_MASK (7 << 12) +#define IMPBMD_TBM_POS 12 +#define IMPBMD_HBM_MASK (7 << 16) +#define IMPBMD_HBM_POS 16 +#define IMPBMD_VBM_MASK (7 << 20) +#define IMPBMD_VBM_POS 20 + +#define IMPBMA (x) (IMPMBA_BASE + 0x4*x) +#define IMPBMD (x) (IMPMBD_BASE + 0x4*x)static void ipmmu_reg_write (struct shmobile_ipmmu *ipmmu,unsigned long reg_off,unsigned long data)
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, · line 114
Quoted 8 lines
drivers/iommu/shmobile-ipmmu.c +5
-0@@ -88,6 +107,91 @@ void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, mutex_unlock(&ipmmu->flush_lock); } +int ipmmu_pmb_enable (struct shmobile_ipmmu *ipmmu, +int enable) +{ +ipmmu_reg_write (ipmmu,IMCTR2 , enable ?IMCTR2_PMBEN :0 ); +return 0 ;
This function could be void.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 118
Quoted 4 lines
drivers/iommu/shmobile-ipmmu.c +4
-0+ +} +int ipmmu_pmb_set_addr(struct shmobile_ipmmu *ipmmu, + int index,
The index could be an unsigned int.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 120
Quoted 2 lines
drivers/iommu/shmobile-ipmmu.c +2
-0+unsigned long addr, + int enabled)
enabled could be a bool.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 130
Quoted 10 lines
drivers/iommu/shmobile-ipmmu.c +10
-0+{ + +if (!enabled) { +ipmmu_reg_write (ipmmu,IMPBMA (index),0 ); +return 0 ; + } + +ipmmu_reg_write (ipmmu,IMPBMA (index), addr | +IMPBMD_V ); +return 0 ;
This function could be void.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, · line 131
Quoted 1 line
drivers/iommu/shmobile-ipmmu.c +1
-0+
Please avoid extra blank lines.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 135
Quoted 4 lines
drivers/iommu/shmobile-ipmmu.c +4
-0+} + +int ipmmu_pmb_set_data(struct shmobile_ipmmu *ipmmu, + int index,
index could be an unsigned int.
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 137
Quoted 2 lines
drivers/iommu/shmobile-ipmmu.c +2
-0+struct ipmmu_pmb_info *info, + struct pmb_tile_info *tile)
info and tile are not modified, you can use const struct ...
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 141
Quoted 4 lines
drivers/iommu/shmobile-ipmmu.c +4
-0+{ +int vbm, hbm, tbm; +int w, h; +unsigned long temp;
temp stores the value of a register, I think it would be better to use a type with an explicit length (u32 in this case). I would also rename temp to val, reg, regval or something similar.
Quoted 13 lines
drivers/iommu/shmobile-ipmmu.c +13
-0+ +if (!info || !info->enabled) { +ipmmu_reg_write (ipmmu,IMPBMD (index),0 ); +return 0 ; + } + + temp = info->paddr; + +switch (info->size_mb) { +case 16 : + temp |=IMPBMD_SZ_16M ; +break ; +case 64 :
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 164
Quoted 10 lines
drivers/iommu/shmobile-ipmmu.c +10
-0+ temp |=IMPBMD_SZ_64M ; +break ; +case 128 : + temp |=IMPBMD_SZ_128M ; +break ; +case 512 : + temp |=IMPBMD_SZ_512M ; +break ; +default : +break ;
Shouldn't you return an error here ?
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 177
Quoted 13 lines
drivers/iommu/shmobile-ipmmu.c +13
-0+ } + + temp |=IMPBMD_V ; + +if (!tile || !tile->enabled) { +ipmmu_reg_write (ipmmu,IMPBMD (index), temp); +return 0 ; + } + + w = tile->tile_width; + h = tile->tile_height; + +if (w & (w -1 ) || h & (h -1 ))
What about using the IS_ALIGNED macro here ?
if (!IS_ALIGNED(tile->tile_width) || !IS_ALIGNED(tile->tile_height))
You could then get rid of the w and h variables.
Shouldn't you also check that the width and height are inside the valid range ?
drivers/iommu/shmobile-ipmmu.c void ipmmu_tlb_set(struct shmobile_ipmmu *ipmmu, unsigned long phys, int size, · line 190
Quoted 13 lines
drivers/iommu/shmobile-ipmmu.c +13
-0+return -EINVAL ; + + tbm =ilog2 (tile->tile_width); + vbm =ilog2 (tile->tile_height) -1 ; + hbm =ilog2 (tile->buffer_pitch) - tbm -1 ; + tbm -=4 ; + + temp |= (tbm <<IMPBMD_TBM_POS ) &IMPBMD_TBM_MASK ; + temp |= (vbm <<IMPBMD_VBM_POS ) &IMPBMD_VBM_MASK ; + temp |= (hbm <<IMPBMD_HBM_POS ) &IMPBMD_HBM_MASK ; + temp |=IMPBMD_BV ; +ipmmu_reg_write (ipmmu,IMPBMD (index), + temp);
No need for a line split here.
Quoted 94 lines
drivers/iommu/shmobile-ipmmu.c +5
-0+ipmmu_tlb_flush (ipmmu); +return 0 ; +} +static int ipmmu_probe (struct platform_device *pdev) {struct shmobile_ipmmu *ipmmu;@@ -118,6 +222,7 @@ static int ipmmu_probe(struct platform_device *pdev)ipmmu_reg_write (ipmmu,IMCTR1 ,0x0 );/* disable TLB */ ipmmu_reg_write (ipmmu,IMCTR2 ,0x0 );/* disable PMB */ ipmmu_iommu_init (ipmmu); + ipmmu->pmb_priv =ipmmu_pmb_init (ipmmu);return 0 ; }drivers/iommu/shmobile-ipmmu.h +27
-1@@ -11,6 +11,8 @@#define __SHMOBILE_IPMMU_H__struct dma_iommu_mapping ; +struct pmb_tile_info ; +struct ipmmu_pmb_info ;struct shmobile_ipmmu {struct device *dev;@@ -20,10 +22,12 @@ struct shmobile_ipmmu {struct dma_iommu_mapping *iommu_mapping;const char *const *dev_names;unsigned int num_dev_names; +void *pmb_priv; }; -#ifdef CONFIG_SHMOBILE_IPMMU_TLB void ipmmu_tlb_flush (struct shmobile_ipmmu *ipmmu); + +#ifdef CONFIG_SHMOBILE_IPMMU_TLB void ipmmu_tlb_set (struct shmobile_ipmmu *ipmmu,unsigned long phys,int size,int asid);int ipmmu_iommu_init (struct shmobile_ipmmu *ipmmu);@@ -34,4 +38,26 @@ static int ipmmu_iommu_init(struct shmobile_ipmmu *ipmmu) }#endif +#ifdef CONFIG_SHMOBILE_PMB +/* Access functions used by PMB device */ +/*void handle_free(struct device *dev, unsigned long handle, int size_mb); +unsigned long handle_alloc(struct device *dev, int size_mb);*/ +int ipmmu_pmb_set_addr (struct shmobile_ipmmu *ipmmu, +int index,unsigned long addr, +int enabled); +int ipmmu_pmb_set_data (struct shmobile_ipmmu *ipmmu,int index, +struct ipmmu_pmb_info *info, +struct pmb_tile_info *tile); +int ipmmu_pmb_enable (struct shmobile_ipmmu *ipmmu,int index); +/* PMB initialization */ +void *ipmmu_pmb_init (struct shmobile_ipmmu *ipmmu); +void ipmmu_pmb_deinit (void *pmb_priv); +#else +static inline void *ipmmu_pmb_init (struct device *dev) +{ +return NULL ; +} +static inline void ipmmu_pmb_deinit (void *pmb_priv) { } +#endif +#endif /* __SHMOBILE_IPMMU_H__ */ drivers/iommu/shmobile-pmb.c new file +16
-0@@ -0,0 +1,362 @@ +/* +* IPMMU-PMB driver +* Copyright (C) 2012 Damian Hobson-Garcia +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; version 2 of the License. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +* You should have received a copy of the GNU General Public License +* along with this program; if not, write to the Free Software +* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + *
+ */drivers/iommu/shmobile-pmb.c new file +1
-0+
drivers/iommu/shmobile-pmb.c line 29
Quoted 10 lines
drivers/iommu/shmobile-pmb.c new file +10
-0+#include <linux/platform_device.h> +#include <linux/io.h> +#include <linux/err.h> +#include <linux/export.h> +#include <linux/cdev.h> +#include <linux/fs.h> +#include <linux/slab.h> +#include <linux/ipmmu.h> +#include "shmobile-ipmmu.h" +#include <linux/uaccess.h>
Please sort header files alphabetically, and move the #include "" at the end.
drivers/iommu/shmobile-pmb.c line 40
Quoted 11 lines
drivers/iommu/shmobile-pmb.c new file +11
-0+ +#define PMB_DEVICE_NAME "pmb" + +#define PMB_NR 16 +/* the smallest size that can be reserverd in the pmb */ +#define PMB_GRANULARITY (16 << 20) +#define PMB_START_ADDR 0x80000000 +#define PMB_SIZE 0x40000000 +#define NUM_BITS (x) ((x) / PMB_GRANULARITY) +#define NR_BITMAPS ((NUM_BITS(PMB_SIZE) + BITS_PER_LONG - 1) \ + >> ilog2(BITS_PER_LONG))
Does ilog2(BITS_PER_LONG) resolve to a compile-time constant ?
Quoted 11 lines
drivers/iommu/shmobile-pmb.c new file +11
-0+ +struct ipmmu_pmb_data { +struct ipmmu_pmb_priv *priv; +struct ipmmu_pmb_info info; +struct pmb_tile_info tile; +unsigned long handle; +int index; +}; + +struct ipmmu_pmb_priv { +struct shmobile_ipmmu *ipmmu;
drivers/iommu/shmobile-pmb.c line 61
Quoted 10 lines
drivers/iommu/shmobile-pmb.c new file +10
-0+struct device *ipmmu_dev; +struct cdev cdev; +struct class *pmb_class; +dev_t pmb_dev; +unsigned long busy_pmbs; +struct mutex pmb_lock; +struct ipmmu_pmb_data pmbs[PMB_NR ]; +struct mutex alloc_lock; +unsigned long alloc_bitmaps[NR_BITMAPS ]; +int pmb_enabled;
bool could do here.
Quoted 153 lines
drivers/iommu/shmobile-pmb.c new file +153
-0+}; + +static int valid_size (int size_mb) +{ +switch (size_mb) { +case 16 : +case 64 : +case 128 : +case 512 : +return 1 ; + } +return 0 ; + +} + +static unsigned long alloc_handle (struct ipmmu_pmb_priv *priv, +int size_mb) +{ +int i; +int idx; +unsigned long tmp_bitmap; +unsigned long alloc_mask; +unsigned long align_mask; +int alloc_bits; + +if (!valid_size (size_mb)) +return -1 ; + + alloc_bits =NUM_BITS (size_mb <<20 ); + alloc_mask = alloc_bits <BITS_PER_LONG ? + (1 << alloc_bits) -1 :-1 ; + + + align_mask = alloc_mask -1 ; +for (i =BITS_PER_LONG >>1 ; i >= alloc_bits; i = i >>1 ) + align_mask = align_mask | (align_mask << i); + +mutex_lock (&priv->alloc_lock); +for (i =0 ; i <NR_BITMAPS ; i++) { + tmp_bitmap = priv->alloc_bitmaps[i]; + tmp_bitmap |= align_mask; + idx =0 ; +while (idx <BITS_PER_LONG ) { + idx =find_next_zero_bit (&tmp_bitmap,BITS_PER_LONG , + idx); +if (!((alloc_mask << idx) & priv->alloc_bitmaps[i]) || + (idx ==BITS_PER_LONG )) +break ; + idx++; + } +if (idx <BITS_PER_LONG ) +break ; + } +if (i ==NR_BITMAPS ) { +mutex_unlock (&priv->alloc_lock); +return 0 ; + } + + priv->alloc_bitmaps[i] |= (alloc_mask << idx); +mutex_unlock (&priv->alloc_lock); + +return PMB_START_ADDR + (i *BITS_PER_LONG + idx) *PMB_GRANULARITY ; +} + +void free_handle (struct ipmmu_pmb_priv *priv, +unsigned long handle, +int size_mb) +{ +int idx; +unsigned long offset; +unsigned long alloc_bits; +unsigned long alloc_mask; + + alloc_bits =NUM_BITS (size_mb <<20 ); + alloc_mask = alloc_bits <BITS_PER_LONG ? + (1 << alloc_bits) -1 :-1 ; + offset = handle -PMB_START_ADDR ; + offset /=PMB_GRANULARITY ; + idx = offset & (BITS_PER_LONG -1 ); + offset = offset /BITS_PER_LONG ; +mutex_lock (&priv->alloc_lock); + priv->alloc_bitmaps[offset] &= ~(alloc_mask << idx); +mutex_unlock (&priv->alloc_lock); +} + +static int set_pmb (struct ipmmu_pmb_data *data, +struct ipmmu_pmb_info *info) +{ +struct ipmmu_pmb_priv *priv = data->priv; +unsigned long data_mask; +int err; + +if (!info->enabled) { +if (data->handle) { +free_handle (priv, data->handle, + data->info.size_mb); + data->handle =0 ; + } + data->info = *info; +ipmmu_pmb_set_data (priv->ipmmu, data->index,NULL ,NULL ); +ipmmu_pmb_set_addr (priv->ipmmu, data->index,0 ,0 ); +ipmmu_tlb_flush (priv->ipmmu); +return 0 ; + } + +if (data->info.enabled) { + err = -EBUSY ; + goto err_out; + } + + data_mask = ~((info->size_mb) -1 ); + +if (info->paddr & ~(data_mask)) { + err = -EINVAL ; + goto err_out; + } + + data->handle =alloc_handle (priv, info->size_mb); + +if (!data->handle) { + err = -ENOMEM ; + goto err_out; + } + + data->info = *info; + +ipmmu_pmb_set_addr (priv->ipmmu, data->index, data->handle,1 ); +ipmmu_pmb_set_data (priv->ipmmu, data->index, &data->info, + &data->tile); + +if (!data->priv->pmb_enabled) { +ipmmu_pmb_enable (priv->ipmmu,1 ); + data->priv->pmb_enabled =1 ; + } + +ipmmu_tlb_flush (priv->ipmmu); + +return 0 ; + +err_out: + info->enabled =0 ; +return err; +} + +static int set_tile (struct ipmmu_pmb_data *data, +struct pmb_tile_info *tile) +{ +struct ipmmu_pmb_priv *priv = data->priv; + data->tile = *tile; +return ipmmu_pmb_set_data (priv->ipmmu, data->index, &data->info, + &data->tile); +} +
drivers/iommu/shmobile-pmb.c line 224
Quoted 10 lines
drivers/iommu/shmobile-pmb.c new file +10
-0+static int ipmmu_pmb_open (struct inode *inode,struct file *filp) +{ +struct ipmmu_pmb_priv *priv; +int idx; + priv = container_of(inode->i_cdev, struct ipmmu_pmb_priv, + cdev); + +mutex_lock (&priv->pmb_lock); + idx =find_first_zero_bit (&priv->busy_pmbs,PMB_NR ); +if (idx ==PMB_NR )
You should unlock the mutex here.
drivers/iommu/shmobile-pmb.c line 230
Quoted 6 lines
drivers/iommu/shmobile-pmb.c new file +6
-0+return -EBUSY ; + +__set_bit (idx, &priv->busy_pmbs); +mutex_unlock (&priv->pmb_lock); + priv->pmbs[idx].index = idx; + priv->pmbs[idx].priv = priv;
You could set the index and priv fields for all PMBs at init time.
Quoted 76 lines
drivers/iommu/shmobile-pmb.c new file +76
-0+ filp->private_data = &priv->pmbs[idx]; +return 0 ; +} + +static int ipmmu_pmb_release (struct inode *inode,struct file *filp) +{ +struct ipmmu_pmb_data *pmb; + pmb = filp->private_data; +if (pmb->info.enabled) { + pmb->info.enabled =0 ; +set_pmb (pmb, &pmb->info); + } + +mutex_lock (&pmb->priv->pmb_lock); +__clear_bit (pmb->index, &pmb->priv->busy_pmbs); +mutex_unlock (&pmb->priv->pmb_lock); +return 0 ; +} + +static long ipmmu_pmb_ioctl (struct file *filp,unsigned int cmd_in, +unsigned long arg) +{ +struct ipmmu_pmb_data *pmb; +struct ipmmu_pmb_info user_set; +struct pmb_tile_info user_tile; +long ret = -EINVAL ; + pmb = filp->private_data; +switch (cmd_in) { +case IPMMU_GET_PMB : + ret =copy_to_user ((char *)arg, &pmb->info,sizeof (pmb->info)); +break ; +case IPMMU_SET_PMB : + ret =copy_from_user (&user_set, (char *)arg,sizeof (user_set)); +if (ret) +break ; + ret =set_pmb (pmb, &user_set); +if (!ret) + pmb->info = user_set; +break ; +case IPMMU_GET_PMB_HANDLE : + ret =copy_to_user ((char *)arg, &pmb->handle, +sizeof (pmb->handle)); +break ; +case IPMMU_GET_PMB_TL : + ret =copy_to_user ((char *)arg, &pmb->tile,sizeof (pmb->tile)); +break ; +case IPMMU_SET_PMB_TL : + ret =copy_from_user (&user_tile, (char *)arg, +sizeof (user_tile)); +if (ret) +break ; + ret =set_tile (pmb, &user_tile); +if (!ret) + pmb->tile = user_tile; +break ; + } +return ret; +} + +static const struct file_operations ipmmu_pmb_fops = { + .owner =THIS_MODULE , + .open = ipmmu_pmb_open, + .release = ipmmu_pmb_release, + .unlocked_ioctl = ipmmu_pmb_ioctl, +}; + +void ipmmu_pmb_deinit (void *arg) +{ +struct ipmmu_pmb_priv *priv = arg; + +if (!priv ||IS_ERR (priv)) +return ; + +cdev_del (&priv->cdev); +device_destroy (priv->pmb_class, priv->pmb_dev); +unregister_chrdev_region (priv->pmb_dev,1 );
drivers/iommu/shmobile-pmb.c line 316
Quoted 10 lines
drivers/iommu/shmobile-pmb.c new file +10
-0+class_destroy (priv->pmb_class); +kfree (priv); +} + +void *ipmmu_pmb_init (struct shmobile_ipmmu *ipmmu) +{ +int err = -ENOENT ; +struct ipmmu_pmb_priv *priv; + + priv =kzalloc (sizeof (struct ipmmu_pmb_priv ),GFP_KERNEL );
Please use the devm_* allocation functions.
Quoted 81 lines
drivers/iommu/shmobile-pmb.c new file +46
-0+if (!priv) { +dev_err (ipmmu->dev,"cannot allocate device data\n" ); +return ERR_PTR (-ENOMEM ); + } + + priv->ipmmu = ipmmu; + priv->ipmmu_dev = ipmmu->dev; + +mutex_init (&priv->pmb_lock); +mutex_init (&priv->alloc_lock); + + priv->pmb_class =class_create (THIS_MODULE ,"ipmmu-pmb" ); +if (!priv->pmb_class) + goto free_priv; + + err =alloc_chrdev_region (&priv->pmb_dev,0 ,1 ,PMB_DEVICE_NAME ); +if (err) { +dev_err (ipmmu->dev,"cannot allocate device num\n" ); + goto destroy_class; + } + +if (!device_create (priv->pmb_class, ipmmu->dev, priv->pmb_dev, priv, +"pmb" )) + goto unregister_region; + +cdev_init (&priv->cdev, &ipmmu_pmb_fops); + priv->cdev.owner =THIS_MODULE ; + priv->cdev.ops = &ipmmu_pmb_fops; + err =cdev_add (&priv->cdev, priv->pmb_dev,1 ); +if (err) { +dev_err (ipmmu->dev,"cannot add ipmmu_pmb device\n" ); + goto destroy_device; + } + +return priv; + +destroy_device: +device_destroy (priv->pmb_class, priv->pmb_dev); +unregister_region: +unregister_chrdev_region (priv->pmb_dev,1 ); +destroy_class: +class_destroy (priv->pmb_class); +free_priv: +kfree (priv); +return ERR_PTR (err); +}include/linux/ipmmu.h new file +29
-0@@ -0,0 +1,29 @@ +#ifndef __LINUX_IPMMU_PMB_H__ +#define __LINUX_IPMMU_PMB_H__ + +struct ipmmu_pmb_info { +int enabled; +unsigned long paddr; +int size_mb; +}; + +struct pmb_tile_info { +int tile_width; +int tile_height; +int buffer_pitch; +int enabled; +}; + +/* IOCTL commands. */ + +#define IPMMU_SET_PMB _IOW('S', 37, struct ipmmu_pmb_phys *) +#define IPMMU_GET_PMB _IOR('S', 38, struct ipmmu_pmb_phys *) +#define IPMMU_GET_PMB_HANDLE _IOR('S', 39, unsigned long *) +#define IPMMU_SET_PMB_TL _IOW('S', 41, struct ipmmu_pmb_tile_info *) +#define IPMMU_GET_PMB_TL _IOR('S', 42, struct ipmmu_pmb_tile_info *) + +#ifdef __kernel + +#endif /* __kernel */ + +#endif /* __LINUX_IPMMU_PMB_H__ */
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/1 ARM: shmobile: ipmmu: Add basic PMB support
Hi Laurent,
Thanks for your comments.
Quoted 10 lines · Laurent Pinchart
Hi Damian,
Thank you for the patch.
Quoted 3 lines · On Friday 18 January 2013 15:35:10 Damian Hobson-Garcia wrote:
The PMB can be used to remap 16, 64, 128 or 512 MiB pages from the 0x80000000-0xffffffff address range to anywhere in the 0x00000000-0x7fffffff range.
Isn't it
0x80000000-0xbfffffffto0x00000000-0xffffffff?
Yes, looking again at the spec, your values are the correct ones.
Quoted 9 lines
Quoted 5 lines
It also has the ability to perform tiled-linear address translation, which can be used to access a memory buffer as a series of n x m tiles, useful for image encoding/decoding.
Currently only the userspace API via character device is supported.If I understand this correctly, you're allowing userspace to remap a virtual address block to a physical address block without performing any sanity check. Isn't that a major security issue ?
No, not really. The PMB will only remap physical addresses, not virtual addresses. Moreover, the remapped address is only accessible from the IP blocks which are on the ICB bus, not the CPU. I will update the comment to mention this. These IP blocks already have access to the entire physical memory address space, so I don't think that adding the the PMB into mix introduces any new security issues.
...
Quoted 14 lines
drivers/iommu/shmobile-pmb.c line 40
Quoted 12 lines
drivers/iommu/shmobile-pmb.c new file +11
-0+ +#define PMB_DEVICE_NAME "pmb" + +#define PMB_NR 16 +/* the smallest size that can be reserverd in the pmb */ +#define PMB_GRANULARITY (16 << 20) +#define PMB_START_ADDR 0x80000000 +#define PMB_SIZE 0x40000000 +#define NUM_BITS (x) ((x) / PMB_GRANULARITY) +#define NR_BITMAPS ((NUM_BITS(PMB_SIZE) + BITS_PER_LONG - 1) \ + >> ilog2(BITS_PER_LONG))Does
ilog2(BITS_PER_LONG)resolve to a compile-time constant ?
Yes it does.
Thanks for your other comments too. I'll look into making those changes.
Damian
Signature · 3 lines
Damian Hobson-Garcia IGEL Co.,Ltd http://www.igel.co.jp
PATCH 1/1 ARM: shmobile: ipmmu: Add basic PMB support
Hi Damian,
Quoted 19 lines · On Tuesday 22 January 2013 11:31:51 Damian Hobson-Garcia wrote:
Quoted 6 lines · Laurent Pinchart
Quoted 3 lines · On Friday 18 January 2013 15:35:10 Damian Hobson-Garcia wrote:
The PMB can be used to remap 16, 64, 128 or 512 MiB pages from the 0x80000000-0xffffffff address range to anywhere in the 0x00000000-0x7fffffff range.
Isn't it
0x80000000-0xbfffffffto0x00000000-0xffffffff?Yes, looking again at the spec, your values are the correct ones.
Quoted 8 lines
Quoted 4 lines
It also has the ability to perform tiled-linear address translation, which can be used to access a memory buffer as a series of n x m tiles, useful for image encoding/decoding.
Currently only the userspace API via character device is supported.If I understand this correctly, you're allowing userspace to remap a virtual address block to a physical address block without performing any sanity check. Isn't that a major security issue ?
Quoted 6 lines
No, not really. The PMB will only remap physical addresses, not virtual addresses. Moreover, the remapped address is only accessible from the IP blocks which are on the ICB bus, not the CPU. I will update the comment to mention this. These IP blocks already have access to the entire physical memory address space, so I don't think that adding the the PMB into mix introduces any new security issues.
The IP block will be programmed through a driver that will control where it writes to/reads from. Adding the PMB will remap those read/write operations without notifying the driver, opening the door to potential issues.
What are the use cases for controlling the PMB from userspace ?
Quoted 19 lines
...
Quoted 5 lines
Quoted 5 lines
drivers/iommu/shmobile-pmb.c new file +5
-0+ +#define PMB_DEVICE_NAME "pmb" + +#define PMB_NR 16 +/* the smallest size that can be reserverd in the pmb */ Quoted 8 lines
drivers/iommu/shmobile-pmb.c line 40
Quoted 6 lines
drivers/iommu/shmobile-pmb.c new file +6
-0+#define PMB_GRANULARITY (16 << 20) +#define PMB_START_ADDR 0x80000000 +#define PMB_SIZE 0x40000000 +#define NUM_BITS (x) ((x) / PMB_GRANULARITY) +#define NR_BITMAPS ((NUM_BITS(PMB_SIZE) + BITS_PER_LONG - 1) \ + >> ilog2(BITS_PER_LONG))Does
ilog2(BITS_PER_LONG)resolve to a compile-time constant ?Yes it does.
Thanks for your other comments too. I'll look into making those changes.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 1/1 ARM: shmobile: ipmmu: Add basic PMB support
Hi Laurent,
Quoted 29 lines · Laurent Pinchart
Hi Damian,
Quoted 19 lines · On Tuesday 22 January 2013 11:31:51 Damian Hobson-Garcia wrote:
Quoted 6 lines · Laurent Pinchart
Quoted 3 lines · On Friday 18 January 2013 15:35:10 Damian Hobson-Garcia wrote:
The PMB can be used to remap 16, 64, 128 or 512 MiB pages from the 0x80000000-0xffffffff address range to anywhere in the 0x00000000-0x7fffffff range.
Isn't it
0x80000000-0xbfffffffto0x00000000-0xffffffff?Yes, looking again at the spec, your values are the correct ones.
Quoted 8 lines
Quoted 4 lines
It also has the ability to perform tiled-linear address translation, which can be used to access a memory buffer as a series of n x m tiles, useful for image encoding/decoding.
Currently only the userspace API via character device is supported.If I understand this correctly, you're allowing userspace to remap a virtual address block to a physical address block without performing any sanity check. Isn't that a major security issue ?
Quoted 6 lines
No, not really. The PMB will only remap physical addresses, not virtual addresses. Moreover, the remapped address is only accessible from the IP blocks which are on the ICB bus, not the CPU. I will update the comment to mention this. These IP blocks already have access to the entire physical memory address space, so I don't think that adding the the PMB into mix introduces any new security issues.
Quoted 3 lines
The IP block will be programmed through a driver that will control where it writes to/reads from. Adding the PMB will remap those read/write operations without notifying the driver, opening the door to potential issues.
Actually this cannot happen. The driver must use the PMB driver to request a mapping, and that mapping cannot be modified by user-space or any other drivers.
Since the PMB only remaps address starting from 0x80000000 (which is outside of the DRAM address space supported by the PMB enabled shmobile devices), only drivers that intentionally use addresses within the PMB range will have their data remapped.
By requesting a mapping by specifying the size and mapping destination to the PMB driver, a driver will effectively lock a region of the PMB address space for its own use. Since the specific region that is locked is decided by the PMB driver, no other device can make changes to the mappings in that region.
For example:
1. Driver A requests a PMB remap to address addr1 with a size of 16M
2. The PMB driver will allocate a region in the PMB address area to hold the mapping (eg. 0x80000000 - 0x80000000 + 16M - 1)
3. Driver A accesses addr1 by specifying a DMA address of 0x80000000 to the hardware
If another client (driver instance, user space, etc) tries to create another mapping, the PMB driver allocates that request to an unused memory region (perhaps 0x80000000 + 64M). If there are no available regions left, the request will fail.
Quoted 2 lines
What are the use cases for controlling the PMB from userspace ?
user-space drivers.
Thanks,
Damian
Quoted 21 lines
Quoted 20 lines
...
Quoted 5 lines
Quoted 5 lines
drivers/iommu/shmobile-pmb.c new file +5
-0+ +#define PMB_DEVICE_NAME "pmb" + +#define PMB_NR 16 +/* the smallest size that can be reserverd in the pmb */ Quoted 8 lines
drivers/iommu/shmobile-pmb.c line 40
Quoted 6 lines
drivers/iommu/shmobile-pmb.c new file +6
-0+#define PMB_GRANULARITY (16 << 20) +#define PMB_START_ADDR 0x80000000 +#define PMB_SIZE 0x40000000 +#define NUM_BITS (x) ((x) / PMB_GRANULARITY) +#define NR_BITMAPS ((NUM_BITS(PMB_SIZE) + BITS_PER_LONG - 1) \ + >> ilog2(BITS_PER_LONG))Does
ilog2(BITS_PER_LONG)resolve to a compile-time constant ?Yes it does.
Thanks for your other comments too. I'll look into making those changes.
Signature · 3 lines
Damian Hobson-Garcia IGEL Co.,Ltd http://www.igel.co.jp