qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings


From: Alvise Rigo
Subject: Re: [Qemu-devel] [RFC v2 1/4] Add EXEC_FLAG to VFIO DMA mappings
Date: Mon, 26 May 2014 14:59:58 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0

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 <address@hidden>
>> ---
>>  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) */
>>
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]