From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36924) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wnl1p-0003EA-TU for qemu-devel@nongnu.org; Fri, 23 May 2014 04:40:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wnl1k-0006eN-Ls for qemu-devel@nongnu.org; Fri, 23 May 2014 04:40:37 -0400 Received: from mail-we0-f175.google.com ([74.125.82.175]:64967) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wnl1k-0006eB-Cl for qemu-devel@nongnu.org; Fri, 23 May 2014 04:40:32 -0400 Received: by mail-we0-f175.google.com with SMTP id t61so4572999wes.34 for ; Fri, 23 May 2014 01:40:31 -0700 (PDT) Message-ID: <537F097D.8010501@linaro.org> Date: Fri, 23 May 2014 10:40:29 +0200 From: Eric Auger MIME-Version: 1.0 References: <1399828415-13007-1-git-send-email-a.rigo@virtualopensystems.com> <1399828415-13007-2-git-send-email-a.rigo@virtualopensystems.com> In-Reply-To: <1399828415-13007-2-git-send-email-a.rigo@virtualopensystems.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alvise Rigo , qemu-devel@nongnu.org, a.motakis@virtualopensystems.com, eric.auger@st.com, kim.phillips@linaro.org Cc: Andrew Jones , Alexey Kardashevskiy , Michael Tokarev , Alex Williamson , Paolo Bonzini , tech@virtualopensystems.com 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 > --- > 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) */ >