| Message ID | 1399828415-13007-2-git-send-email-a.rigo@virtualopensystems.com |
|---|---|
| State | New |
| Headers | show |
On 05/11/2014 07:13 PM, Alvise Rigo wrote: > The flag is mandatory for the ARM SMMU so we always add it if the MMIO > handles it. Hi Alvise, Refering to the root problem explanation found in https://lkml.org/lkml/2014/2/8/176, I understand the problem is specific to devices that fetch instructions from executable memory region sections (XN =0). Typically this is not the case of Midway xgmac which does not need executable regions and hence does not need that change. in http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007095.html, Will says most IOMMU mappings should be XN. I am not knowledged enough on mem mapping settings to understand the consequences of always setting XN=0, even for devices that do not need request it. Does anyone have an opinion on this? Best Regards Eric > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > hw/vfio/common.c | 9 +++++++++ > hw/vfio/vfio-common.h | 1 + > linux-headers/linux/vfio.h | 2 ++ > 3 files changed, 12 insertions(+) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 9d1f723..a805c5d 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova, > map.flags |= VFIO_DMA_MAP_FLAG_WRITE; > } > > + /* add exec flag */ > + if (container->iommu_data.has_exec_cap) { > + map.flags |= VFIO_DMA_MAP_FLAG_EXEC; > + } > + > /* > * Try the mapping, if it fails with EBUSY, unmap the region and try > * again. This shouldn't be necessary, but we sometimes see it in > @@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group) > return -errno; > } > > + if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) { > + container->iommu_data.has_exec_cap = true; > + } > + > container->iommu_data.type1.listener = vfio_memory_listener; > container->iommu_data.release = vfio_listener_release; > > diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h > index 21148ef..1abbd1a 100644 > --- a/hw/vfio/vfio-common.h > +++ b/hw/vfio/vfio-common.h > @@ -35,6 +35,7 @@ typedef struct VFIOContainer { > union { > VFIOType1 type1; > }; > + bool has_exec_cap; /* support of exec capability by the IOMMU */ > void (*release)(struct VFIOContainer *); > } iommu_data; > QLIST_HEAD(, VFIOGroup) group_list; > diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h > index 17c58e0..95a02c5 100644 > --- a/linux-headers/linux/vfio.h > +++ b/linux-headers/linux/vfio.h > @@ -24,6 +24,7 @@ > #define VFIO_TYPE1_IOMMU 1 > #define VFIO_SPAPR_TCE_IOMMU 2 > > +#define VFIO_IOMMU_PROT_EXEC 5 > /* > * The IOCTL interface is designed for extensibility by embedding the > * structure length (argsz) and flags into structures passed between > @@ -392,6 +393,7 @@ struct vfio_iommu_type1_dma_map { > __u32 flags; > #define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */ > #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */ > +#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2) /* executable from device */ > __u64 vaddr; /* Process virtual address */ > __u64 iova; /* IO virtual address */ > __u64 size; /* Size of mapping (bytes) */ >
On 23/05/2014 10:40, Eric Auger wrote: > On 05/11/2014 07:13 PM, Alvise Rigo wrote: >> The flag is mandatory for the ARM SMMU so we always add it if the MMIO >> handles it. > > Hi Alvise, > > Refering to the root problem explanation found in > https://lkml.org/lkml/2014/2/8/176, I understand the problem is specific > to devices that fetch instructions from executable memory region > sections (XN =0). > > Typically this is not the case of Midway xgmac which does not need > executable regions and hence does not need that change. > > in > http://lists.linuxfoundation.org/pipermail/iommu/2013-November/007095.html, > Will says most IOMMU mappings should be XN. Hello Eric, As I see it, to be as more device agnostic as possible, when a mapping is flagged as READ, then it should also be flagged with the EXEC flag (when the IOMMU supports it). In this way we don't compromise those devices that rely on it (like the ARM DMA330). Making this optional would require us to know a lot more about the device(s), something that probably we don't want. Moreover, as discussed in the thread "[RFC PATCH v5 02/11] ARM SMMU: Add capability IOMMU_CAP_DMA_EXEC", in the future this flag might be something inherently used that the user can optionally disable. > > I am not knowledged enough on mem mapping settings to understand the > consequences of always setting XN=0, even for devices that do not need > request it. I don't foresee any problem here, but better to have other opinions about it. Best regards, alvise > > Does anyone have an opinion on this? > > Best Regards > > Eric > >> >> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> >> --- >> hw/vfio/common.c | 9 +++++++++ >> hw/vfio/vfio-common.h | 1 + >> linux-headers/linux/vfio.h | 2 ++ >> 3 files changed, 12 insertions(+) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 9d1f723..a805c5d 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova, >> map.flags |= VFIO_DMA_MAP_FLAG_WRITE; >> } >> >> + /* add exec flag */ >> + if (container->iommu_data.has_exec_cap) { >> + map.flags |= VFIO_DMA_MAP_FLAG_EXEC; >> + } >> + >> /* >> * Try the mapping, if it fails with EBUSY, unmap the region and try >> * again. This shouldn't be necessary, but we sometimes see it in >> @@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group) >> return -errno; >> } >> >> + if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) { >> + container->iommu_data.has_exec_cap = true; >> + } >> + >> container->iommu_data.type1.listener = vfio_memory_listener; >> container->iommu_data.release = vfio_listener_release; >> >> diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h >> index 21148ef..1abbd1a 100644 >> --- a/hw/vfio/vfio-common.h >> +++ b/hw/vfio/vfio-common.h >> @@ -35,6 +35,7 @@ typedef struct VFIOContainer { >> union { >> VFIOType1 type1; >> }; >> + bool has_exec_cap; /* support of exec capability by the IOMMU */ >> void (*release)(struct VFIOContainer *); >> } iommu_data; >> QLIST_HEAD(, VFIOGroup) group_list; >> diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h >> index 17c58e0..95a02c5 100644 >> --- a/linux-headers/linux/vfio.h >> +++ b/linux-headers/linux/vfio.h >> @@ -24,6 +24,7 @@ >> #define VFIO_TYPE1_IOMMU 1 >> #define VFIO_SPAPR_TCE_IOMMU 2 >> >> +#define VFIO_IOMMU_PROT_EXEC 5 >> /* >> * The IOCTL interface is designed for extensibility by embedding the >> * structure length (argsz) and flags into structures passed between >> @@ -392,6 +393,7 @@ struct vfio_iommu_type1_dma_map { >> __u32 flags; >> #define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */ >> #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */ >> +#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2) /* executable from device */ >> __u64 vaddr; /* Process virtual address */ >> __u64 iova; /* IO virtual address */ >> __u64 size; /* Size of mapping (bytes) */ >> >
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 9d1f723..a805c5d 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -107,6 +107,11 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova, map.flags |= VFIO_DMA_MAP_FLAG_WRITE; } + /* add exec flag */ + if (container->iommu_data.has_exec_cap) { + map.flags |= VFIO_DMA_MAP_FLAG_EXEC; + } + /* * Try the mapping, if it fails with EBUSY, unmap the region and try * again. This shouldn't be necessary, but we sometimes see it in @@ -352,6 +357,10 @@ static int vfio_connect_container(VFIOGroup *group) return -errno; } + if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_IOMMU_PROT_EXEC)) { + container->iommu_data.has_exec_cap = true; + } + container->iommu_data.type1.listener = vfio_memory_listener; container->iommu_data.release = vfio_listener_release; diff --git a/hw/vfio/vfio-common.h b/hw/vfio/vfio-common.h index 21148ef..1abbd1a 100644 --- a/hw/vfio/vfio-common.h +++ b/hw/vfio/vfio-common.h @@ -35,6 +35,7 @@ typedef struct VFIOContainer { union { VFIOType1 type1; }; + bool has_exec_cap; /* support of exec capability by the IOMMU */ void (*release)(struct VFIOContainer *); } iommu_data; QLIST_HEAD(, VFIOGroup) group_list; diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h index 17c58e0..95a02c5 100644 --- a/linux-headers/linux/vfio.h +++ b/linux-headers/linux/vfio.h @@ -24,6 +24,7 @@ #define VFIO_TYPE1_IOMMU 1 #define VFIO_SPAPR_TCE_IOMMU 2 +#define VFIO_IOMMU_PROT_EXEC 5 /* * The IOCTL interface is designed for extensibility by embedding the * structure length (argsz) and flags into structures passed between @@ -392,6 +393,7 @@ struct vfio_iommu_type1_dma_map { __u32 flags; #define VFIO_DMA_MAP_FLAG_READ (1 << 0) /* readable from device */ #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1) /* writable from device */ +#define VFIO_DMA_MAP_FLAG_EXEC (1 << 2) /* executable from device */ __u64 vaddr; /* Process virtual address */ __u64 iova; /* IO virtual address */ __u64 size; /* Size of mapping (bytes) */
The flag is mandatory for the ARM SMMU so we always add it if the MMIO handles it. Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> --- hw/vfio/common.c | 9 +++++++++ hw/vfio/vfio-common.h | 1 + linux-headers/linux/vfio.h | 2 ++ 3 files changed, 12 insertions(+)