[PATCHv7 01/26] iommu/arm-smmu: change IOMMU_EXEC to IOMMU_NOEXEC

[PATCHv7 01/26] iommu/arm-smmu: change IOMMU_EXEC to IOMMU_NOEXEC

[Antonios Motakis]

Exposing the XN flag of the SMMU driver as IOMMU_NOEXEC instead of
IOMMU_EXEC makes it enforceable, since for IOMMUs that don't support
the XN flag pages will always be executable.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/iommu/arm-smmu.c | 9 +++++----
 include/linux/iommu.h    | 2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a83cc2a..c7cbdda 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1252,7 +1252,7 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 				   unsigned long pfn, int prot, int stage)
 {
 	pte_t *pte, *start;
-	pteval_t pteval = ARM_SMMU_PTE_PAGE | ARM_SMMU_PTE_AF | ARM_SMMU_PTE_XN;
+	pteval_t pteval = ARM_SMMU_PTE_PAGE | ARM_SMMU_PTE_AF;
 
 	if (pmd_none(*pmd)) {
 		/* Allocate a new set of tables */
@@ -1286,10 +1286,11 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 			pteval |= ARM_SMMU_PTE_MEMATTR_NC;
 	}
 
+	if (prot & IOMMU_NOEXEC)
+		pteval |= ARM_SMMU_PTE_XN;
+
 	/* If no access, create a faulting entry to avoid TLB fills */
-	if (prot & IOMMU_EXEC)
-		pteval &= ~ARM_SMMU_PTE_XN;
-	else if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
+	if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
 		pteval &= ~ARM_SMMU_PTE_PAGE;
 
 	pteval |= ARM_SMMU_PTE_SH_IS;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a52..e1a644c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -27,7 +27,7 @@
 #define IOMMU_READ	(1 << 0)
 #define IOMMU_WRITE	(1 << 1)
 #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
-#define IOMMU_EXEC	(1 << 3)
+#define IOMMU_NOEXEC	(1 << 3)
 
 struct iommu_ops;
 struct iommu_group;
-- 
1.8.3.2

Re: [PATCHv7 01/26] iommu/arm-smmu: change IOMMU_EXEC to IOMMU_NOEXEC

[Will Deacon]

Hi Antonios,

On Tue, Sep 23, 2014 at 03:46:00PM +0100, Antonios Motakis wrote:
> Exposing the XN flag of the SMMU driver as IOMMU_NOEXEC instead of
> IOMMU_EXEC makes it enforceable, since for IOMMUs that don't support
> the XN flag pages will always be executable.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/iommu/arm-smmu.c | 9 +++++----
>  include/linux/iommu.h    | 2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)

[...]

> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a52..e1a644c 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -27,7 +27,7 @@
>  #define IOMMU_READ	(1 << 0)
>  #define IOMMU_WRITE	(1 << 1)
>  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
> -#define IOMMU_EXEC	(1 << 3)
> +#define IOMMU_NOEXEC	(1 << 3)

This hunk needs to be a separate patch merged by Joerg before I can take the
arm-smmu part (which looks fine).

Will

Re: [PATCHv7 01/26] iommu/arm-smmu: change IOMMU_EXEC to IOMMU_NOEXEC

[Alex Williamson]

On Tue, 2014-09-23 at 15:58 +0100, Will Deacon wrote:
> Hi Antonios,
> 
> On Tue, Sep 23, 2014 at 03:46:00PM +0100, Antonios Motakis wrote:
> > Exposing the XN flag of the SMMU driver as IOMMU_NOEXEC instead of
> > IOMMU_EXEC makes it enforceable, since for IOMMUs that don't support
> > the XN flag pages will always be executable.
> > 
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 9 +++++----
> >  include/linux/iommu.h    | 2 +-
> >  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> [...]
> 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 20f9a52..e1a644c 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -27,7 +27,7 @@
> >  #define IOMMU_READ	(1 << 0)
> >  #define IOMMU_WRITE	(1 << 1)
> >  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
> > -#define IOMMU_EXEC	(1 << 3)
> > +#define IOMMU_NOEXEC	(1 << 3)
> 
> This hunk needs to be a separate patch merged by Joerg before I can take the
> arm-smmu part (which looks fine).

That separate hunk would be unbuildable since arm-smmu depends on the
IOMMU_EXEC define.  Patch 2/ is also in iommu code and gates patch 3/ in
arm-smmu.  The IOMMU-core changes are pretty trivial, so perhaps Joerg
would be willing to ACK 1&2 and let Will include the first 3 patches
through his tree.

These first 3 patches should have been sent on their own since they're
small an obvious so they don't get hung up on the reset of the series.
Thanks,

Alex

[PATCHv7 02/26] iommu: add capability IOMMU_CAP_NOEXEC

[Antonios Motakis]

Some IOMMUs accept an IOMMU_NOEXEC protection flag in addition to
IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 include/linux/iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e1a644c..0433553 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -59,6 +59,7 @@ struct iommu_domain {
 
 #define IOMMU_CAP_CACHE_COHERENCY	0x1
 #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
+#define IOMMU_CAP_NOEXEC		0x3	/* IOMMU_NOEXEC flag */
 
 /*
  * Following constraints are specifc to FSL_PAMUV1:
-- 
1.8.3.2

Re: [PATCHv7 02/26] iommu: add capability IOMMU_CAP_NOEXEC

[Joerg Roedel]

On Tue, Sep 23, 2014 at 04:46:01PM +0200, Antonios Motakis wrote:
> Some IOMMUs accept an IOMMU_NOEXEC protection flag in addition to
> IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  include/linux/iommu.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e1a644c..0433553 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -59,6 +59,7 @@ struct iommu_domain {
>  
>  #define IOMMU_CAP_CACHE_COHERENCY	0x1
>  #define IOMMU_CAP_INTR_REMAP		0x2	/* isolates device intrs */
> +#define IOMMU_CAP_NOEXEC		0x3	/* IOMMU_NOEXEC flag */

This changed recently to an enum, please rebase to latest iommu/next
branch.


With that change:

	Acked-by: Joerg Roedel <jroedel@suse.de>

for patches 1 and 2.

Re: [PATCHv7 02/26] iommu: add capability IOMMU_CAP_NOEXEC

[Antonios Motakis]

On Fri, Sep 26, 2014 at 11:48 AM, Joerg Roedel <joro@8bytes.org> wrote:
> On Tue, Sep 23, 2014 at 04:46:01PM +0200, Antonios Motakis wrote:
>> Some IOMMUs accept an IOMMU_NOEXEC protection flag in addition to
>> IOMMU_READ and IOMMU_WRITE. Expose this as an IOMMU capability.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  include/linux/iommu.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index e1a644c..0433553 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -59,6 +59,7 @@ struct iommu_domain {
>>
>>  #define IOMMU_CAP_CACHE_COHERENCY    0x1
>>  #define IOMMU_CAP_INTR_REMAP         0x2     /* isolates device intrs */
>> +#define IOMMU_CAP_NOEXEC             0x3     /* IOMMU_NOEXEC flag */
>
> This changed recently to an enum, please rebase to latest iommu/next
> branch.
>
>
> With that change:
>
>         Acked-by: Joerg Roedel <jroedel@suse.de>
>
> for patches 1 and 2.
>

Ack


-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 03/26] iommu/arm-smmu: add IOMMU_CAP_NOEXEC to the ARM SMMU driver

[Antonios Motakis]

The ARM SMMU supports the IOMMU_NOEXEC protection flag. Add the
corresponding IOMMU capability.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/iommu/arm-smmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index c7cbdda..7c0fa25 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1539,6 +1539,8 @@ static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
 		return features & ARM_SMMU_FEAT_COHERENT_WALK;
 	case IOMMU_CAP_INTR_REMAP:
 		return 1; /* MSIs are just memory writes */
+	case IOMMU_CAP_NOEXEC:
+		return 1;
 	default:
 		return 0;
 	}
-- 
1.8.3.2

[PATCHv7 04/26] vfio/iommu_type1: support for platform bus devices on ARM

[Antonios Motakis]

This allows to make use of the VFIO_IOMMU_TYPE1 driver with platform
devices on ARM. The driver can then be used with an Exynos SMMU, or
ARM SMMU driver.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index d8c5763..a0abe04 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -16,7 +16,7 @@ config VFIO_SPAPR_EEH
 menuconfig VFIO
 	tristate "VFIO Non-Privileged userspace driver framework"
 	depends on IOMMU_API
-	select VFIO_IOMMU_TYPE1 if X86
+	select VFIO_IOMMU_TYPE1 if X86 || ARM
 	select VFIO_IOMMU_SPAPR_TCE if (PPC_POWERNV || PPC_PSERIES)
 	select VFIO_SPAPR_EEH if (PPC_POWERNV || PPC_PSERIES)
 	select ANON_INODES
-- 
1.8.3.2

[PATCHv7 05/26] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
and expose its availability via the capability VFIO_IOMMU_PROT_NOEXEC.
This way the user can control whether the XN flag will be set on the
requested mappings. The IOMMU_NOEXEC flag needs to be available for all
the IOMMUs of the container used.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 include/uapi/linux/vfio.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 6612974..30f630c 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -29,6 +29,7 @@
  * capability is subject to change as groups are added or removed.
  */
 #define VFIO_DMA_CC_IOMMU		4
+#define VFIO_IOMMU_PROT_NOEXEC		5
 
 /* Check if EEH is supported */
 #define VFIO_EEH			5
@@ -401,6 +402,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_NOEXEC (1 << 2)	/* not executable from device */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
-- 
1.8.3.2

Re: [PATCHv7 05/26] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
> and expose its availability via the capability VFIO_IOMMU_PROT_NOEXEC.
> This way the user can control whether the XN flag will be set on the
> requested mappings. The IOMMU_NOEXEC flag needs to be available for all
> the IOMMUs of the container used.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  include/uapi/linux/vfio.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 6612974..30f630c 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -29,6 +29,7 @@
>   * capability is subject to change as groups are added or removed.
>   */
>  #define VFIO_DMA_CC_IOMMU		4
> +#define VFIO_IOMMU_PROT_NOEXEC		5

Can't we advertise this as a flag bit in vfio_iommu_type1_info instead?
Also, EEH already took 5 as seen immediately below.

>  
>  /* Check if EEH is supported */
>  #define VFIO_EEH			5
> @@ -401,6 +402,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_NOEXEC (1 << 2)	/* not executable from device */
>  	__u64	vaddr;				/* Process virtual address */
>  	__u64	iova;				/* IO virtual address */
>  	__u64	size;				/* Size of mapping (bytes) */

Re: [PATCHv7 05/26] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Alex Williamson]

On Tue, 2014-09-23 at 16:21 -0600, Alex Williamson wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> > We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
> > and expose its availability via the capability VFIO_IOMMU_PROT_NOEXEC.
> > This way the user can control whether the XN flag will be set on the
> > requested mappings. The IOMMU_NOEXEC flag needs to be available for all
> > the IOMMUs of the container used.
> > 
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > ---
> >  include/uapi/linux/vfio.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 6612974..30f630c 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -29,6 +29,7 @@
> >   * capability is subject to change as groups are added or removed.
> >   */
> >  #define VFIO_DMA_CC_IOMMU		4
> > +#define VFIO_IOMMU_PROT_NOEXEC		5
> 
> Can't we advertise this as a flag bit in vfio_iommu_type1_info instead?

Ok, I see in the next patch that it's pretty similar to
VFIO_DMA_CC_IOMMU, so the check extension is probably correct for
determining the current state.  Maybe we could name it more similarly,
VFIO_DMA_NOEXEC_IOMMU.  I guess the intended usage is that once a user
attaches a group to the container they can query whether the
VFIO_DMA_MAP_FLAG_NOEXEC is valid.  Ok.  Thanks,

Alex

> Also, EEH already took 5 as seen immediately below.
> 
> >  
> >  /* Check if EEH is supported */
> >  #define VFIO_EEH			5
> > @@ -401,6 +402,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_NOEXEC (1 << 2)	/* not executable from device */
> >  	__u64	vaddr;				/* Process virtual address */
> >  	__u64	iova;				/* IO virtual address */
> >  	__u64	size;				/* Size of mapping (bytes) */
> 
> 

Re: [PATCHv7 05/26] vfio: introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

On Wed, Sep 24, 2014 at 12:30 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2014-09-23 at 16:21 -0600, Alex Williamson wrote:
>> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> > We introduce the VFIO_DMA_MAP_FLAG_NOEXEC flag to the VFIO dma map call,
>> > and expose its availability via the capability VFIO_IOMMU_PROT_NOEXEC.
>> > This way the user can control whether the XN flag will be set on the
>> > requested mappings. The IOMMU_NOEXEC flag needs to be available for all
>> > the IOMMUs of the container used.
>> >
>> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> > ---
>> >  include/uapi/linux/vfio.h | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> > index 6612974..30f630c 100644
>> > --- a/include/uapi/linux/vfio.h
>> > +++ b/include/uapi/linux/vfio.h
>> > @@ -29,6 +29,7 @@
>> >   * capability is subject to change as groups are added or removed.
>> >   */
>> >  #define VFIO_DMA_CC_IOMMU          4
>> > +#define VFIO_IOMMU_PROT_NOEXEC             5
>>
>> Can't we advertise this as a flag bit in vfio_iommu_type1_info instead?
>
> Ok, I see in the next patch that it's pretty similar to
> VFIO_DMA_CC_IOMMU, so the check extension is probably correct for
> determining the current state.  Maybe we could name it more similarly,
> VFIO_DMA_NOEXEC_IOMMU.  I guess the intended usage is that once a user
> attaches a group to the container they can query whether the
> VFIO_DMA_MAP_FLAG_NOEXEC is valid.  Ok.  Thanks,
>

Sorry for the numbering, ack and ack :)

> Alex
>
>> Also, EEH already took 5 as seen immediately below.
>>
>> >
>> >  /* Check if EEH is supported */
>> >  #define VFIO_EEH                   5
>> > @@ -401,6 +402,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_NOEXEC (1 << 2)  /* not executable from device */
>> >     __u64   vaddr;                          /* Process virtual address */
>> >     __u64   iova;                           /* IO virtual address */
>> >     __u64   size;                           /* Size of mapping (bytes) */
>>
>>
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 06/26] vfio/iommu_type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

Some IOMMU drivers, such as the ARM SMMU driver, make available the
IOMMU_NOEXEC flag, to set the page tables for a device as XN (execute never).
This affects devices such as the ARM PL330 DMA Controller, which respects
this flag and will refuse to fetch DMA instructions from memory where the
XN flag has been set.

The flag can be used only if all IOMMU domains behind the container support
the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
new domains with devices will have to support it as well.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/vfio_iommu_type1.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0734fbe..09e5064 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -81,6 +81,26 @@ struct vfio_group {
 };
 
 /*
+ * This function returns true only if _all_ domains support the capability.
+ */
+static int vfio_all_domains_have_iommu_noexec(struct vfio_iommu *iommu)
+{
+	struct vfio_domain *d;
+	int ret = 1;
+
+	mutex_lock(&iommu->lock);
+	list_for_each_entry(d, &iommu->domain_list, next) {
+		if (!iommu_domain_has_cap(d->domain, IOMMU_CAP_NOEXEC)) {
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&iommu->lock);
+
+	return ret;
+}
+
+/*
  * This code handles mapping and unmapping of user data buffers
  * into DMA'ble space using the IOMMU
  */
@@ -546,6 +566,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		prot |= IOMMU_WRITE;
 	if (map->flags & VFIO_DMA_MAP_FLAG_READ)
 		prot |= IOMMU_READ;
+	if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
+		if (!vfio_all_domains_have_iommu_noexec(iommu))
+			return -EINVAL;
+		prot |= IOMMU_NOEXEC;
+	}
 
 	if (!prot || !size || (size | iova | vaddr) & mask)
 		return -EINVAL;
@@ -636,6 +661,12 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
 		dma = rb_entry(n, struct vfio_dma, node);
 		iova = dma->iova;
 
+		/* if any of the mappings to be replayed has the NOEXEC flag
+		 * set, then the new iommu domain must support it */
+		if ((dma->prot | IOMMU_NOEXEC) &&
+		    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_NOEXEC))
+			return -EINVAL;
+
 		while (iova < dma->iova + dma->size) {
 			phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
 			size_t size;
@@ -890,6 +921,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 			if (!iommu)
 				return 0;
 			return vfio_domains_have_iommu_cache(iommu);
+		case VFIO_IOMMU_PROT_NOEXEC:
+			if (!iommu)
+				return 0;
+			return vfio_all_domains_have_iommu_noexec(iommu);
 		default:
 			return 0;
 		}
@@ -913,7 +948,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
 		struct vfio_iommu_type1_dma_map map;
 		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
-				VFIO_DMA_MAP_FLAG_WRITE;
+				VFIO_DMA_MAP_FLAG_WRITE |
+				VFIO_DMA_MAP_FLAG_NOEXEC;
 
 		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
-- 
1.8.3.2

Re: [PATCHv7 06/26] vfio/iommu_type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> Some IOMMU drivers, such as the ARM SMMU driver, make available the
> IOMMU_NOEXEC flag, to set the page tables for a device as XN (execute never).
> This affects devices such as the ARM PL330 DMA Controller, which respects
> this flag and will refuse to fetch DMA instructions from memory where the
> XN flag has been set.
> 
> The flag can be used only if all IOMMU domains behind the container support
> the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
> new domains with devices will have to support it as well.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 0734fbe..09e5064 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -81,6 +81,26 @@ struct vfio_group {
>  };
>  
>  /*
> + * This function returns true only if _all_ domains support the capability.
> + */
> +static int vfio_all_domains_have_iommu_noexec(struct vfio_iommu *iommu)

Rename to vfio_domains_have_iommu_noexec() for consistency with the
cache version.

> +{
> +	struct vfio_domain *d;
> +	int ret = 1;
> +
> +	mutex_lock(&iommu->lock);
> +	list_for_each_entry(d, &iommu->domain_list, next) {
> +		if (!iommu_domain_has_cap(d->domain, IOMMU_CAP_NOEXEC)) {

Should we cache this in domain->prot like we do for IOMMU_CACHE?

> +			ret = 0;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&iommu->lock);
> +
> +	return ret;
> +}
> +
> +/*
>   * This code handles mapping and unmapping of user data buffers
>   * into DMA'ble space using the IOMMU
>   */
> @@ -546,6 +566,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>  		prot |= IOMMU_WRITE;
>  	if (map->flags & VFIO_DMA_MAP_FLAG_READ)
>  		prot |= IOMMU_READ;
> +	if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
> +		if (!vfio_all_domains_have_iommu_noexec(iommu))
> +			return -EINVAL;
> +		prot |= IOMMU_NOEXEC;
> +	}
>  
>  	if (!prot || !size || (size | iova | vaddr) & mask)
>  		return -EINVAL;
> @@ -636,6 +661,12 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>  		dma = rb_entry(n, struct vfio_dma, node);
>  		iova = dma->iova;
>  
> +		/* if any of the mappings to be replayed has the NOEXEC flag
> +		 * set, then the new iommu domain must support it */

nit, please fix the comment style to match the rest of the file.

> +		if ((dma->prot | IOMMU_NOEXEC) &&
> +		    !iommu_domain_has_cap(domain->domain, IOMMU_CAP_NOEXEC))
> +			return -EINVAL;
> +
>  		while (iova < dma->iova + dma->size) {
>  			phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
>  			size_t size;
> @@ -890,6 +921,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  			if (!iommu)
>  				return 0;
>  			return vfio_domains_have_iommu_cache(iommu);
> +		case VFIO_IOMMU_PROT_NOEXEC:
> +			if (!iommu)
> +				return 0;
> +			return vfio_all_domains_have_iommu_noexec(iommu);
>  		default:
>  			return 0;
>  		}
> @@ -913,7 +948,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  	} else if (cmd == VFIO_IOMMU_MAP_DMA) {
>  		struct vfio_iommu_type1_dma_map map;
>  		uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
> -				VFIO_DMA_MAP_FLAG_WRITE;
> +				VFIO_DMA_MAP_FLAG_WRITE |
> +				VFIO_DMA_MAP_FLAG_NOEXEC;
>  
>  		minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
>  

Re: [PATCHv7 06/26] vfio/iommu_type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

On Wed, Sep 24, 2014 at 12:40 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> Some IOMMU drivers, such as the ARM SMMU driver, make available the
>> IOMMU_NOEXEC flag, to set the page tables for a device as XN (execute never).
>> This affects devices such as the ARM PL330 DMA Controller, which respects
>> this flag and will refuse to fetch DMA instructions from memory where the
>> XN flag has been set.
>>
>> The flag can be used only if all IOMMU domains behind the container support
>> the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
>> new domains with devices will have to support it as well.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 38 +++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 0734fbe..09e5064 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -81,6 +81,26 @@ struct vfio_group {
>>  };
>>
>>  /*
>> + * This function returns true only if _all_ domains support the capability.
>> + */
>> +static int vfio_all_domains_have_iommu_noexec(struct vfio_iommu *iommu)
>
> Rename to vfio_domains_have_iommu_noexec() for consistency with the
> cache version.
>

The logic here is a slightly different logic between the two. For
IOMMU_CACHE we generally check if any domain includes it, for NOEXEC
in contract we need all domains to support it, otherwise we can't
expose the capability. Hence the _all_ addition in the name of the
function.

>> +{
>> +     struct vfio_domain *d;
>> +     int ret = 1;
>> +
>> +     mutex_lock(&iommu->lock);
>> +     list_for_each_entry(d, &iommu->domain_list, next) {
>> +             if (!iommu_domain_has_cap(d->domain, IOMMU_CAP_NOEXEC)) {
>
> Should we cache this in domain->prot like we do for IOMMU_CACHE?
>
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +     mutex_unlock(&iommu->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +/*
>>   * This code handles mapping and unmapping of user data buffers
>>   * into DMA'ble space using the IOMMU
>>   */
>> @@ -546,6 +566,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>>               prot |= IOMMU_WRITE;
>>       if (map->flags & VFIO_DMA_MAP_FLAG_READ)
>>               prot |= IOMMU_READ;
>> +     if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
>> +             if (!vfio_all_domains_have_iommu_noexec(iommu))
>> +                     return -EINVAL;
>> +             prot |= IOMMU_NOEXEC;
>> +     }
>>
>>       if (!prot || !size || (size | iova | vaddr) & mask)
>>               return -EINVAL;
>> @@ -636,6 +661,12 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>>               dma = rb_entry(n, struct vfio_dma, node);
>>               iova = dma->iova;
>>
>> +             /* if any of the mappings to be replayed has the NOEXEC flag
>> +              * set, then the new iommu domain must support it */
>
> nit, please fix the comment style to match the rest of the file.
>

Ack

>> +             if ((dma->prot | IOMMU_NOEXEC) &&
>> +                 !iommu_domain_has_cap(domain->domain, IOMMU_CAP_NOEXEC))
>> +                     return -EINVAL;
>> +
>>               while (iova < dma->iova + dma->size) {
>>                       phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
>>                       size_t size;
>> @@ -890,6 +921,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>                       if (!iommu)
>>                               return 0;
>>                       return vfio_domains_have_iommu_cache(iommu);
>> +             case VFIO_IOMMU_PROT_NOEXEC:
>> +                     if (!iommu)
>> +                             return 0;
>> +                     return vfio_all_domains_have_iommu_noexec(iommu);
>>               default:
>>                       return 0;
>>               }
>> @@ -913,7 +948,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>>       } else if (cmd == VFIO_IOMMU_MAP_DMA) {
>>               struct vfio_iommu_type1_dma_map map;
>>               uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
>> -                             VFIO_DMA_MAP_FLAG_WRITE;
>> +                             VFIO_DMA_MAP_FLAG_WRITE |
>> +                             VFIO_DMA_MAP_FLAG_NOEXEC;
>>
>>               minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
>>
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

Re: [PATCHv7 06/26] vfio/iommu_type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Alex Williamson]

On Fri, 2014-09-26 at 17:39 +0200, Antonios Motakis wrote:
> On Wed, Sep 24, 2014 at 12:40 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> >> Some IOMMU drivers, such as the ARM SMMU driver, make available the
> >> IOMMU_NOEXEC flag, to set the page tables for a device as XN (execute never).
> >> This affects devices such as the ARM PL330 DMA Controller, which respects
> >> this flag and will refuse to fetch DMA instructions from memory where the
> >> XN flag has been set.
> >>
> >> The flag can be used only if all IOMMU domains behind the container support
> >> the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
> >> new domains with devices will have to support it as well.
> >>
> >> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> >> ---
> >>  drivers/vfio/vfio_iommu_type1.c | 38 +++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 37 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> >> index 0734fbe..09e5064 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -81,6 +81,26 @@ struct vfio_group {
> >>  };
> >>
> >>  /*
> >> + * This function returns true only if _all_ domains support the capability.
> >> + */
> >> +static int vfio_all_domains_have_iommu_noexec(struct vfio_iommu *iommu)
> >
> > Rename to vfio_domains_have_iommu_noexec() for consistency with the
> > cache version.
> >
> 
> The logic here is a slightly different logic between the two. For
> IOMMU_CACHE we generally check if any domain includes it,

Not true, all the domains must support IOMMU_CACHE for the function to
return 1.  In fact, the code is so identical that if we were to cache
IOMMU_CAP_NOEXEC into domain->prot, we should probably only have one
function:

static int vfio_domains_have_iommu_flag(struct vfio_iommu *iommu, int flag);

>  for NOEXEC
> in contract we need all domains to support it, otherwise we can't
> expose the capability. Hence the _all_ addition in the name of the
> function.
> 
> >> +{
> >> +     struct vfio_domain *d;
> >> +     int ret = 1;
> >> +
> >> +     mutex_lock(&iommu->lock);
> >> +     list_for_each_entry(d, &iommu->domain_list, next) {
> >> +             if (!iommu_domain_has_cap(d->domain, IOMMU_CAP_NOEXEC)) {
> >
> > Should we cache this in domain->prot like we do for IOMMU_CACHE?
> >
> >> +                     ret = 0;
> >> +                     break;
> >> +             }
> >> +     }
> >> +     mutex_unlock(&iommu->lock);
> >> +
> >> +     return ret;
> >> +}
> >> +
> >> +/*
> >>   * This code handles mapping and unmapping of user data buffers
> >>   * into DMA'ble space using the IOMMU
> >>   */
> >> @@ -546,6 +566,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
> >>               prot |= IOMMU_WRITE;
> >>       if (map->flags & VFIO_DMA_MAP_FLAG_READ)
> >>               prot |= IOMMU_READ;
> >> +     if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
> >> +             if (!vfio_all_domains_have_iommu_noexec(iommu))
> >> +                     return -EINVAL;
> >> +             prot |= IOMMU_NOEXEC;
> >> +     }
> >>
> >>       if (!prot || !size || (size | iova | vaddr) & mask)
> >>               return -EINVAL;
> >> @@ -636,6 +661,12 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
> >>               dma = rb_entry(n, struct vfio_dma, node);
> >>               iova = dma->iova;
> >>
> >> +             /* if any of the mappings to be replayed has the NOEXEC flag
> >> +              * set, then the new iommu domain must support it */
> >
> > nit, please fix the comment style to match the rest of the file.
> >
> 
> Ack
> 
> >> +             if ((dma->prot | IOMMU_NOEXEC) &&
> >> +                 !iommu_domain_has_cap(domain->domain, IOMMU_CAP_NOEXEC))
> >> +                     return -EINVAL;
> >> +
> >>               while (iova < dma->iova + dma->size) {
> >>                       phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
> >>                       size_t size;
> >> @@ -890,6 +921,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>                       if (!iommu)
> >>                               return 0;
> >>                       return vfio_domains_have_iommu_cache(iommu);
> >> +             case VFIO_IOMMU_PROT_NOEXEC:
> >> +                     if (!iommu)
> >> +                             return 0;
> >> +                     return vfio_all_domains_have_iommu_noexec(iommu);
> >>               default:
> >>                       return 0;
> >>               }
> >> @@ -913,7 +948,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >>       } else if (cmd == VFIO_IOMMU_MAP_DMA) {
> >>               struct vfio_iommu_type1_dma_map map;
> >>               uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
> >> -                             VFIO_DMA_MAP_FLAG_WRITE;
> >> +                             VFIO_DMA_MAP_FLAG_WRITE |
> >> +                             VFIO_DMA_MAP_FLAG_NOEXEC;
> >>
> >>               minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
> >>
> >
> >
> >
> 
> 
> 

Re: [PATCHv7 06/26] vfio/iommu_type1: implement the VFIO_DMA_MAP_FLAG_NOEXEC flag

[Antonios Motakis]

On Fri, Sep 26, 2014 at 10:27 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2014-09-26 at 17:39 +0200, Antonios Motakis wrote:
>> On Wed, Sep 24, 2014 at 12:40 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> > On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> >> Some IOMMU drivers, such as the ARM SMMU driver, make available the
>> >> IOMMU_NOEXEC flag, to set the page tables for a device as XN (execute never).
>> >> This affects devices such as the ARM PL330 DMA Controller, which respects
>> >> this flag and will refuse to fetch DMA instructions from memory where the
>> >> XN flag has been set.
>> >>
>> >> The flag can be used only if all IOMMU domains behind the container support
>> >> the IOMMU_NOEXEC flag. Also, if any mappings are created with the flag, any
>> >> new domains with devices will have to support it as well.
>> >>
>> >> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> >> ---
>> >>  drivers/vfio/vfio_iommu_type1.c | 38 +++++++++++++++++++++++++++++++++++++-
>> >>  1 file changed, 37 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> >> index 0734fbe..09e5064 100644
>> >> --- a/drivers/vfio/vfio_iommu_type1.c
>> >> +++ b/drivers/vfio/vfio_iommu_type1.c
>> >> @@ -81,6 +81,26 @@ struct vfio_group {
>> >>  };
>> >>
>> >>  /*
>> >> + * This function returns true only if _all_ domains support the capability.
>> >> + */
>> >> +static int vfio_all_domains_have_iommu_noexec(struct vfio_iommu *iommu)
>> >
>> > Rename to vfio_domains_have_iommu_noexec() for consistency with the
>> > cache version.
>> >
>>
>> The logic here is a slightly different logic between the two. For
>> IOMMU_CACHE we generally check if any domain includes it,
>
> Not true, all the domains must support IOMMU_CACHE for the function to
> return 1.  In fact, the code is so identical that if we were to cache
> IOMMU_CAP_NOEXEC into domain->prot, we should probably only have one
> function:
>
> static int vfio_domains_have_iommu_flag(struct vfio_iommu *iommu, int flag);
>

You are absolutely correct, I managed to confuse myself when switching
from vfio_domains_have_iommu_exec to vfio_domains_have_iommu_noexec.

I will implement the shared function.

>>  for NOEXEC
>> in contract we need all domains to support it, otherwise we can't
>> expose the capability. Hence the _all_ addition in the name of the
>> function.
>>
>> >> +{
>> >> +     struct vfio_domain *d;
>> >> +     int ret = 1;
>> >> +
>> >> +     mutex_lock(&iommu->lock);
>> >> +     list_for_each_entry(d, &iommu->domain_list, next) {
>> >> +             if (!iommu_domain_has_cap(d->domain, IOMMU_CAP_NOEXEC)) {
>> >
>> > Should we cache this in domain->prot like we do for IOMMU_CACHE?
>> >
>> >> +                     ret = 0;
>> >> +                     break;
>> >> +             }
>> >> +     }
>> >> +     mutex_unlock(&iommu->lock);
>> >> +
>> >> +     return ret;
>> >> +}
>> >> +
>> >> +/*
>> >>   * This code handles mapping and unmapping of user data buffers
>> >>   * into DMA'ble space using the IOMMU
>> >>   */
>> >> @@ -546,6 +566,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>> >>               prot |= IOMMU_WRITE;
>> >>       if (map->flags & VFIO_DMA_MAP_FLAG_READ)
>> >>               prot |= IOMMU_READ;
>> >> +     if (map->flags & VFIO_DMA_MAP_FLAG_NOEXEC) {
>> >> +             if (!vfio_all_domains_have_iommu_noexec(iommu))
>> >> +                     return -EINVAL;
>> >> +             prot |= IOMMU_NOEXEC;
>> >> +     }
>> >>
>> >>       if (!prot || !size || (size | iova | vaddr) & mask)
>> >>               return -EINVAL;
>> >> @@ -636,6 +661,12 @@ static int vfio_iommu_replay(struct vfio_iommu *iommu,
>> >>               dma = rb_entry(n, struct vfio_dma, node);
>> >>               iova = dma->iova;
>> >>
>> >> +             /* if any of the mappings to be replayed has the NOEXEC flag
>> >> +              * set, then the new iommu domain must support it */
>> >
>> > nit, please fix the comment style to match the rest of the file.
>> >
>>
>> Ack
>>
>> >> +             if ((dma->prot | IOMMU_NOEXEC) &&
>> >> +                 !iommu_domain_has_cap(domain->domain, IOMMU_CAP_NOEXEC))
>> >> +                     return -EINVAL;
>> >> +
>> >>               while (iova < dma->iova + dma->size) {
>> >>                       phys_addr_t phys = iommu_iova_to_phys(d->domain, iova);
>> >>                       size_t size;
>> >> @@ -890,6 +921,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>> >>                       if (!iommu)
>> >>                               return 0;
>> >>                       return vfio_domains_have_iommu_cache(iommu);
>> >> +             case VFIO_IOMMU_PROT_NOEXEC:
>> >> +                     if (!iommu)
>> >> +                             return 0;
>> >> +                     return vfio_all_domains_have_iommu_noexec(iommu);
>> >>               default:
>> >>                       return 0;
>> >>               }
>> >> @@ -913,7 +948,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>> >>       } else if (cmd == VFIO_IOMMU_MAP_DMA) {
>> >>               struct vfio_iommu_type1_dma_map map;
>> >>               uint32_t mask = VFIO_DMA_MAP_FLAG_READ |
>> >> -                             VFIO_DMA_MAP_FLAG_WRITE;
>> >> +                             VFIO_DMA_MAP_FLAG_WRITE |
>> >> +                             VFIO_DMA_MAP_FLAG_NOEXEC;
>> >>
>> >>               minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
>> >>
>> >
>> >
>> >
>>
>>
>>
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

As already demonstrated with PCI [1] and the platform bus [2], a
driver_override property in sysfs can be used to bypass the id matching
of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
device requested by the user.

[1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
[2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/amba/bus.c       | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |  1 +
 2 files changed, 45 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 3cf61a1..473177c 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -17,6 +17,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/amba/bus.h>
 #include <linux/sizes.h>
+#include <linux/limits.h>
 
 #include <asm/irq.h>
 
@@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct device_driver *drv)
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(drv);
 
+	/* When driver_override is set, only bind to the matching driver */
+	if (pcdev->driver_override)
+		return !strcmp(pcdev->driver_override, drv->name);
+
 	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
 }
 
@@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return retval;
 }
 
+static ssize_t driver_override_show(struct device *_dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct amba_device *dev = to_amba_device(_dev);
+
+	return sprintf(buf, "%s\n", dev->driver_override);
+}
+
+static ssize_t driver_override_store(struct device *_dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct amba_device *dev = to_amba_device(_dev);
+	char *driver_override, *old = dev->driver_override, *cp;
+
+	if (count > PATH_MAX)
+		return -EINVAL;
+
+	driver_override = kstrndup(buf, count, GFP_KERNEL);
+	if (!driver_override)
+		return -ENOMEM;
+
+	cp = strchr(driver_override, '\n');
+	if (cp)
+		*cp = '\0';
+
+	if (strlen(driver_override)) {
+		dev->driver_override = driver_override;
+	} else {
+	       kfree(driver_override);
+	       dev->driver_override = NULL;
+	}
+
+	kfree(old);
+
+	return count;
+}
+
 #define amba_attr_func(name,fmt,arg...)					\
 static ssize_t name##_show(struct device *_dev,				\
 			   struct device_attribute *attr, char *buf)	\
@@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
 static struct device_attribute amba_dev_attrs[] = {
 	__ATTR_RO(id),
 	__ATTR_RO(resource),
+	__ATTR_RW(driver_override),
 	__ATTR_NULL,
 };
 
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index fdd7e1b..7c011e7 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -32,6 +32,7 @@ struct amba_device {
 	struct clk		*pclk;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
+	char			*driver_override;
 };
 
 struct amba_driver {
-- 
1.8.3.2

Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
> 
> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/amba/bus.c       | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/amba/bus.h |  1 +
>  2 files changed, 45 insertions(+)

Why are 7 & 8 being dragged into this series?  Combine them into a
single patch and post it separately.

> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 3cf61a1..473177c 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/amba/bus.h>
>  #include <linux/sizes.h>
> +#include <linux/limits.h>
>  
>  #include <asm/irq.h>
>  
> @@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct device_driver *drv)
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *pcdrv = to_amba_driver(drv);
>  
> +	/* When driver_override is set, only bind to the matching driver */
> +	if (pcdev->driver_override)
> +		return !strcmp(pcdev->driver_override, drv->name);
> +
>  	return amba_lookup(pcdrv->id_table, pcdev) != NULL;
>  }
>  
> @@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	return retval;
>  }
>  
> +static ssize_t driver_override_show(struct device *_dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct amba_device *dev = to_amba_device(_dev);
> +
> +	return sprintf(buf, "%s\n", dev->driver_override);
> +}
> +
> +static ssize_t driver_override_store(struct device *_dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct amba_device *dev = to_amba_device(_dev);
> +	char *driver_override, *old = dev->driver_override, *cp;
> +
> +	if (count > PATH_MAX)
> +		return -EINVAL;
> +
> +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> +	if (!driver_override)
> +		return -ENOMEM;
> +
> +	cp = strchr(driver_override, '\n');
> +	if (cp)
> +		*cp = '\0';
> +
> +	if (strlen(driver_override)) {
> +		dev->driver_override = driver_override;
> +	} else {
> +	       kfree(driver_override);
> +	       dev->driver_override = NULL;
> +	}
> +
> +	kfree(old);
> +
> +	return count;
> +}
> +
>  #define amba_attr_func(name,fmt,arg...)					\
>  static ssize_t name##_show(struct device *_dev,				\
>  			   struct device_attribute *attr, char *buf)	\
> @@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
>  static struct device_attribute amba_dev_attrs[] = {
>  	__ATTR_RO(id),
>  	__ATTR_RO(resource),
> +	__ATTR_RW(driver_override),
>  	__ATTR_NULL,
>  };
>  
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index fdd7e1b..7c011e7 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -32,6 +32,7 @@ struct amba_device {
>  	struct clk		*pclk;
>  	unsigned int		periphid;
>  	unsigned int		irq[AMBA_NR_IRQS];
> +	char			*driver_override;
>  };
>  
>  struct amba_driver {

Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

Ok, they will be posted separately.

Thanks

On Wed, Sep 24, 2014 at 12:45 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/amba/bus.c       | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/amba/bus.h |  1 +
>>  2 files changed, 45 insertions(+)
>
> Why are 7 & 8 being dragged into this series?  Combine them into a
> single patch and post it separately.
>
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 3cf61a1..473177c 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/amba/bus.h>
>>  #include <linux/sizes.h>
>> +#include <linux/limits.h>
>>
>>  #include <asm/irq.h>
>>
>> @@ -42,6 +43,10 @@ static int amba_match(struct device *dev, struct device_driver *drv)
>>       struct amba_device *pcdev = to_amba_device(dev);
>>       struct amba_driver *pcdrv = to_amba_driver(drv);
>>
>> +     /* When driver_override is set, only bind to the matching driver */
>> +     if (pcdev->driver_override)
>> +             return !strcmp(pcdev->driver_override, drv->name);
>> +
>>       return amba_lookup(pcdrv->id_table, pcdev) != NULL;
>>  }
>>
>> @@ -58,6 +63,44 @@ static int amba_uevent(struct device *dev, struct kobj_uevent_env *env)
>>       return retval;
>>  }
>>
>> +static ssize_t driver_override_show(struct device *_dev,
>> +                                 struct device_attribute *attr, char *buf)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +
>> +     return sprintf(buf, "%s\n", dev->driver_override);
>> +}
>> +
>> +static ssize_t driver_override_store(struct device *_dev,
>> +                                  struct device_attribute *attr,
>> +                                  const char *buf, size_t count)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +     char *driver_override, *old = dev->driver_override, *cp;
>> +
>> +     if (count > PATH_MAX)
>> +             return -EINVAL;
>> +
>> +     driver_override = kstrndup(buf, count, GFP_KERNEL);
>> +     if (!driver_override)
>> +             return -ENOMEM;
>> +
>> +     cp = strchr(driver_override, '\n');
>> +     if (cp)
>> +             *cp = '\0';
>> +
>> +     if (strlen(driver_override)) {
>> +             dev->driver_override = driver_override;
>> +     } else {
>> +            kfree(driver_override);
>> +            dev->driver_override = NULL;
>> +     }
>> +
>> +     kfree(old);
>> +
>> +     return count;
>> +}
>> +
>>  #define amba_attr_func(name,fmt,arg...)                                      \
>>  static ssize_t name##_show(struct device *_dev,                              \
>>                          struct device_attribute *attr, char *buf)    \
>> @@ -80,6 +123,7 @@ amba_attr_func(resource, "\t%016llx\t%016llx\t%016lx\n",
>>  static struct device_attribute amba_dev_attrs[] = {
>>       __ATTR_RO(id),
>>       __ATTR_RO(resource),
>> +     __ATTR_RW(driver_override),
>>       __ATTR_NULL,
>>  };
>>
>> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
>> index fdd7e1b..7c011e7 100644
>> --- a/include/linux/amba/bus.h
>> +++ b/include/linux/amba/bus.h
>> @@ -32,6 +32,7 @@ struct amba_device {
>>       struct clk              *pclk;
>>       unsigned int            periphid;
>>       unsigned int            irq[AMBA_NR_IRQS];
>> +     char                    *driver_override;
>>  };
>>
>>  struct amba_driver {
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Russell King - ARM Linux]

On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
> 
> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>

I have to ask why this is even needed in the first place.  To take the
example in [2], what's wrong with:

echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind

and similar for AMBA.

All we would need to do is to introduce a way of having a driver accept
explicit bind requests.

In any case:

> +static ssize_t driver_override_store(struct device *_dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	struct amba_device *dev = to_amba_device(_dev);
> +	char *driver_override, *old = dev->driver_override, *cp;
> +
> +	if (count > PATH_MAX)
> +		return -EINVAL;
> +
> +	driver_override = kstrndup(buf, count, GFP_KERNEL);
> +	if (!driver_override)
> +		return -ENOMEM;
> +
> +	cp = strchr(driver_override, '\n');
> +	if (cp)
> +		*cp = '\0';

I hope that is not replicated everywhere.  This allows up to a page to be
allocated, even when the first byte may be a newline.  This is wasteful.

How about:

	if (count > PATH_MAX)
		return -EINVAL;

	cp = strnchr(buf, count, '\n');
	if (cp)
		count = cp - buf - 1;

	if (count) {
		driver_override = kstrndup(buf, count, GFP_KERNEL);
		if (!driver_override)
			return -ENOMEM;
	} else {
		driver_override = NULL;
	}

	kfree(dev->driver_override);
	dev->driver_override = driver_override;

Also:

> +static ssize_t driver_override_show(struct device *_dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct amba_device *dev = to_amba_device(_dev);
> +
> +	return sprintf(buf, "%s\n", dev->driver_override);
> +}

Do we really want to do a NULL pointer dereference here?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
>> As already demonstrated with PCI [1] and the platform bus [2], a
>> driver_override property in sysfs can be used to bypass the id matching
>> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
>> device requested by the user.
>>
>> [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
>> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>
> I have to ask why this is even needed in the first place.  To take the
> example in [2], what's wrong with:
>
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
>
> and similar for AMBA.
>
> All we would need to do is to introduce a way of having a driver accept
> explicit bind requests.

I don't have strong feelings on whether it should be done one way or
the other, however the approach proposed here is identical to the one
already accepted in mainline for PCI and platform devices. Should we
do something different for AMBA?

>
> In any case:
>
>> +static ssize_t driver_override_store(struct device *_dev,
>> +                                  struct device_attribute *attr,
>> +                                  const char *buf, size_t count)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +     char *driver_override, *old = dev->driver_override, *cp;
>> +
>> +     if (count > PATH_MAX)
>> +             return -EINVAL;
>> +
>> +     driver_override = kstrndup(buf, count, GFP_KERNEL);
>> +     if (!driver_override)
>> +             return -ENOMEM;
>> +
>> +     cp = strchr(driver_override, '\n');
>> +     if (cp)
>> +             *cp = '\0';
>
> I hope that is not replicated everywhere.  This allows up to a page to be
> allocated, even when the first byte may be a newline.  This is wasteful.
>
> How about:
>
>         if (count > PATH_MAX)
>                 return -EINVAL;
>
>         cp = strnchr(buf, count, '\n');
>         if (cp)
>                 count = cp - buf - 1;
>
>         if (count) {
>                 driver_override = kstrndup(buf, count, GFP_KERNEL);
>                 if (!driver_override)
>                         return -ENOMEM;
>         } else {
>                 driver_override = NULL;
>         }
>
>         kfree(dev->driver_override);
>         dev->driver_override = driver_override;

Ack.

>
> Also:
>
>> +static ssize_t driver_override_show(struct device *_dev,
>> +                                 struct device_attribute *attr, char *buf)
>> +{
>> +     struct amba_device *dev = to_amba_device(_dev);
>> +
>> +     return sprintf(buf, "%s\n", dev->driver_override);
>> +}
>
> Do we really want to do a NULL pointer dereference here?

I'll add a check here.

Thanks

-- 
Antonios Motakis
Virtual Open Systems

Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'

[Antonios Motakis]

On Fri, Sep 26, 2014 at 5:37 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
>
> On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
> > As already demonstrated with PCI [1] and the platform bus [2], a
> > driver_override property in sysfs can be used to bypass the id matching
> > of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> > device requested by the user.
> >
> > [1] http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> > [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>
> I have to ask why this is even needed in the first place.  To take the
> example in [2], what's wrong with:
>
> echo fff51000.ethernet > /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
> echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
>
> and similar for AMBA.
>
> All we would need to do is to introduce a way of having a driver accept
> explicit bind requests.
>
> In any case:
>
> > +static ssize_t driver_override_store(struct device *_dev,
> > +                                  struct device_attribute *attr,
> > +                                  const char *buf, size_t count)
> > +{
> > +     struct amba_device *dev = to_amba_device(_dev);
> > +     char *driver_override, *old = dev->driver_override, *cp;
> > +
> > +     if (count > PATH_MAX)
> > +             return -EINVAL;
> > +
> > +     driver_override = kstrndup(buf, count, GFP_KERNEL);
> > +     if (!driver_override)
> > +             return -ENOMEM;
> > +
> > +     cp = strchr(driver_override, '\n');
> > +     if (cp)
> > +             *cp = '\0';
>
> I hope that is not replicated everywhere.  This allows up to a page to be
> allocated, even when the first byte may be a newline.  This is wasteful.
>
> How about:
>
>         if (count > PATH_MAX)
>                 return -EINVAL;
>
>         cp = strnchr(buf, count, '\n');
>         if (cp)
>                 count = cp - buf - 1;
>
>         if (count) {
>                 driver_override = kstrndup(buf, count, GFP_KERNEL);
>                 if (!driver_override)
>                         return -ENOMEM;
>         } else {
>                 driver_override = NULL;
>         }
>
>         kfree(dev->driver_override);
>         dev->driver_override = driver_override;

I implemented something along this lines and tested it a bit. The
kernel already splits the input by newlines, and the function is being
called for each one separately; so this change doesn't save any
memory.

>
> Also:
>
> > +static ssize_t driver_override_show(struct device *_dev,
> > +                                 struct device_attribute *attr, char *buf)
> > +{
> > +     struct amba_device *dev = to_amba_device(_dev);
> > +
> > +     return sprintf(buf, "%s\n", dev->driver_override);
> > +}
>
> Do we really want to do a NULL pointer dereference here?
>
> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.




-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 08/26] driver core: amba: add documentation for binding path 'driver_override'

[Antonios Motakis]

Add documentation for alternative binding path 'driver_override' for
AMBA devices.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 Documentation/ABI/testing/sysfs-bus-amba | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-amba

diff --git a/Documentation/ABI/testing/sysfs-bus-amba b/Documentation/ABI/testing/sysfs-bus-amba
new file mode 100644
index 0000000..e7b5467
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-amba
@@ -0,0 +1,20 @@
+What:		/sys/bus/amba/devices/.../driver_override
+Date:		September 2014
+Contact:	Antonios Motakis <a.motakis@virtualopensystems.com>
+Description:
+		This file allows the driver for a device to be specified which
+		will override standard OF, ACPI, ID table, and name matching.
+		When specified, only a driver with a name matching the value
+		written to driver_override will have an opportunity to bind to
+		the device. The override is specified by writing a string to the
+		driver_override file (echo vfio-amba > driver_override)	and may
+		be cleared with an empty string (echo > driver_override).
+		This returns the device to standard matching rules binding.
+		Writing to driver_override does not automatically unbind the
+		device from its current driver or make any attempt to
+		automatically load the specified driver. If no driver with a
+		matching name is currently loaded in the kernel, the device will
+		not bind to any driver. This also allows devices to opt-out of
+		driver binding using a driver_override name such as "none".
+		Only a single driver may be specified in the override, there is
+		no support for parsing delimiters.
-- 
1.8.3.2

[PATCHv7 09/26] vfio/platform: initial skeleton of VFIO support for platform devices

[Antonios Motakis]

This patch forms the common skeleton code for platform devices support
with VFIO. This will include the core functionality of VFIO_PLATFORM,
however binding to the device and discovering the device resources will
be done with the help of a separate file where any Linux platform bus
specific code will reside.

This will allow us to implement support for also discovering AMBA devices
and their resources, but still reuse a large part of the VFIO_PLATFORM
implementation.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 129 ++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  35 +++++++
 2 files changed, 164 insertions(+)
 create mode 100644 drivers/vfio/platform/vfio_platform_common.c
 create mode 100644 drivers/vfio/platform/vfio_platform_private.h

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
new file mode 100644
index 0000000..07d02dc
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -0,0 +1,129 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/io.h>
+
+#include "vfio_platform_private.h"
+
+static void vfio_platform_release(void *device_data)
+{
+	module_put(THIS_MODULE);
+}
+
+static int vfio_platform_open(void *device_data)
+{
+	if (!try_module_get(THIS_MODULE))
+		return -ENODEV;
+
+	return 0;
+}
+
+static long vfio_platform_ioctl(void *device_data,
+			   unsigned int cmd, unsigned long arg)
+{
+	if (cmd == VFIO_DEVICE_GET_INFO)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_SET_IRQS)
+		return -EINVAL;
+
+	else if (cmd == VFIO_DEVICE_RESET)
+		return -EINVAL;
+
+	return -ENOTTY;
+}
+
+static ssize_t vfio_platform_read(void *device_data, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
+{
+	return -EINVAL;
+}
+
+static const struct vfio_device_ops vfio_platform_ops = {
+	.name		= "vfio-platform",
+	.open		= vfio_platform_open,
+	.release	= vfio_platform_release,
+	.ioctl		= vfio_platform_ioctl,
+	.read		= vfio_platform_read,
+	.write		= vfio_platform_write,
+	.mmap		= vfio_platform_mmap,
+};
+
+int vfio_platform_probe_common(struct vfio_platform_device *vdev,
+			       struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	if (!vdev)
+		return -EINVAL;
+
+	group = iommu_group_get(dev);
+	if (!group) {
+		pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
+		return -EINVAL;
+	}
+
+	ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev);
+	if (ret) {
+		iommu_group_put(group);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
+
+int vfio_platform_remove_common(struct device *dev)
+{
+	struct vfio_platform_device *vdev;
+
+	vdev = vfio_del_group_dev(dev);
+	if (!vdev)
+		return -EINVAL;
+
+	iommu_group_put(dev->iommu_group);
+	kfree(vdev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
new file mode 100644
index 0000000..ef76737
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#ifndef VFIO_PLATFORM_PRIVATE_H
+#define VFIO_PLATFORM_PRIVATE_H
+
+struct vfio_platform_device {
+	/*
+	 * These fields should be filled by the bus driver at binding time
+	 */
+	void		*opaque;
+	const char	*name;
+	uint32_t	flags;
+	/* callbacks to discover device resources */
+	struct resource*
+		(*get_resource)(struct vfio_platform_device *vdev, int i);
+	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
+};
+
+extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
+				      struct device *dev);
+extern int vfio_platform_remove_common(struct device *dev);
+
+#endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

[PATCHv7 10/26] vfio: platform: probe to devices on the platform bus

[Antonios Motakis]

Driver to bind to Linux platform devices, and callbacks to discover their
resources to be used by the main VFIO PLATFORM code.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform.c | 96 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h             |  1 +
 2 files changed, 97 insertions(+)
 create mode 100644 drivers/vfio/platform/vfio_platform.c

diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
new file mode 100644
index 0000000..024c026
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/eventfd.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION  "0.7"
+#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
+#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
+
+/* probing devices from the linux platform bus */
+
+static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
+						int i)
+{
+	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+
+	return platform_get_resource(pdev, IORESOURCE_MEM, i);
+}
+
+static int get_platform_irq(struct vfio_platform_device *vdev, int i)
+{
+	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
+
+	return platform_get_irq(pdev, i);
+}
+
+
+static int vfio_platform_probe(struct platform_device *pdev)
+{
+	struct vfio_platform_device *vdev;
+	int ret;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	vdev->opaque = (void *) pdev;
+	vdev->name = pdev->name;
+	vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
+	vdev->get_resource = get_platform_resource;
+	vdev->get_irq = get_platform_irq;
+
+	ret = vfio_platform_probe_common(vdev, &pdev->dev);
+	if (ret)
+		kfree(vdev);
+
+	return ret;
+}
+
+static int vfio_platform_remove(struct platform_device *pdev)
+{
+	return vfio_platform_remove_common(&pdev->dev);
+}
+
+static struct platform_driver vfio_platform_driver = {
+	.probe		= vfio_platform_probe,
+	.remove		= vfio_platform_remove,
+	.driver	= {
+		.name	= "vfio-platform",
+		.owner	= THIS_MODULE,
+	},
+};
+
+module_platform_driver(vfio_platform_driver);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 30f630c..b022a25 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -158,6 +158,7 @@ struct vfio_device_info {
 	__u32	flags;
 #define VFIO_DEVICE_FLAGS_RESET	(1 << 0)	/* Device supports reset */
 #define VFIO_DEVICE_FLAGS_PCI	(1 << 1)	/* vfio-pci device */
+#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
 };
-- 
1.8.3.2

Re: [PATCHv7 10/26] vfio: platform: probe to devices on the platform bus

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> Driver to bind to Linux platform devices, and callbacks to discover their
> resources to be used by the main VFIO PLATFORM code.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform.c | 96 +++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vfio.h             |  1 +
>  2 files changed, 97 insertions(+)
>  create mode 100644 drivers/vfio/platform/vfio_platform.c
> 
> diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> new file mode 100644
> index 0000000..024c026
> --- /dev/null
> +++ b/drivers/vfio/platform/vfio_platform.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (C) 2013 - Virtual Open Systems
> + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/eventfd.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +#include <linux/vfio.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/irq.h>
> +
> +#include "vfio_platform_private.h"
> +
> +#define DRIVER_VERSION  "0.7"
> +#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
> +#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
> +
> +/* probing devices from the linux platform bus */
> +
> +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> +						int i)
> +{
> +	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> +
> +	return platform_get_resource(pdev, IORESOURCE_MEM, i);

ARM may only support IORESOURCE_MEM, but I don't think platform devices
are limited to MMIO, right?  vfio-platform shouldn't be either.

> +}
> +
> +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
> +{
> +	struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> +
> +	return platform_get_irq(pdev, i);
> +}
> +
> +
> +static int vfio_platform_probe(struct platform_device *pdev)
> +{
> +	struct vfio_platform_device *vdev;
> +	int ret;
> +
> +	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> +	if (!vdev)
> +		return -ENOMEM;
> +
> +	vdev->opaque = (void *) pdev;
> +	vdev->name = pdev->name;
> +	vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
> +	vdev->get_resource = get_platform_resource;
> +	vdev->get_irq = get_platform_irq;
> +
> +	ret = vfio_platform_probe_common(vdev, &pdev->dev);
> +	if (ret)
> +		kfree(vdev);
> +
> +	return ret;
> +}
> +
> +static int vfio_platform_remove(struct platform_device *pdev)
> +{
> +	return vfio_platform_remove_common(&pdev->dev);
> +}
> +
> +static struct platform_driver vfio_platform_driver = {
> +	.probe		= vfio_platform_probe,
> +	.remove		= vfio_platform_remove,
> +	.driver	= {
> +		.name	= "vfio-platform",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(vfio_platform_driver);
> +
> +MODULE_VERSION(DRIVER_VERSION);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 30f630c..b022a25 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -158,6 +158,7 @@ struct vfio_device_info {
>  	__u32	flags;
>  #define VFIO_DEVICE_FLAGS_RESET	(1 << 0)	/* Device supports reset */
>  #define VFIO_DEVICE_FLAGS_PCI	(1 << 1)	/* vfio-pci device */
> +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
>  	__u32	num_regions;	/* Max region index + 1 */
>  	__u32	num_irqs;	/* Max IRQ index + 1 */
>  };

Re: [PATCHv7 10/26] vfio: platform: probe to devices on the platform bus

[Antonios Motakis]

On Wed, Sep 24, 2014 at 1:01 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> > Driver to bind to Linux platform devices, and callbacks to discover their
> > resources to be used by the main VFIO PLATFORM code.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > ---
> >  drivers/vfio/platform/vfio_platform.c | 96 +++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/vfio.h             |  1 +
> >  2 files changed, 97 insertions(+)
> >  create mode 100644 drivers/vfio/platform/vfio_platform.c
> >
> > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> > new file mode 100644
> > index 0000000..024c026
> > --- /dev/null
> > +++ b/drivers/vfio/platform/vfio_platform.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * Copyright (C) 2013 - Virtual Open Systems
> > + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License, version 2, as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/eventfd.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iommu.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/notifier.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/vfio.h>
> > +#include <linux/io.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/irq.h>
> > +
> > +#include "vfio_platform_private.h"
> > +
> > +#define DRIVER_VERSION  "0.7"
> > +#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
> > +#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
> > +
> > +/* probing devices from the linux platform bus */
> > +
> > +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> > +                                             int i)
> > +{
> > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> > +
> > +     return platform_get_resource(pdev, IORESOURCE_MEM, i);
>
> ARM may only support IORESOURCE_MEM, but I don't think platform devices
> are limited to MMIO, right?  vfio-platform shouldn't be either.
>

Indeed. Should we however implement this lacking a target to verify it
is working correctly?

Leaving it out would mean PIO resources for those devices would not be
exposed before an update to VFIO, but we wouldn't have to break
backward compatibility I think.

Would you prefer to have it implemented regardless?

> > +}
> > +
> > +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
> > +{
> > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> > +
> > +     return platform_get_irq(pdev, i);
> > +}
> > +
> > +
> > +static int vfio_platform_probe(struct platform_device *pdev)
> > +{
> > +     struct vfio_platform_device *vdev;
> > +     int ret;
> > +
> > +     vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> > +     if (!vdev)
> > +             return -ENOMEM;
> > +
> > +     vdev->opaque = (void *) pdev;
> > +     vdev->name = pdev->name;
> > +     vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
> > +     vdev->get_resource = get_platform_resource;
> > +     vdev->get_irq = get_platform_irq;
> > +
> > +     ret = vfio_platform_probe_common(vdev, &pdev->dev);
> > +     if (ret)
> > +             kfree(vdev);
> > +
> > +     return ret;
> > +}
> > +
> > +static int vfio_platform_remove(struct platform_device *pdev)
> > +{
> > +     return vfio_platform_remove_common(&pdev->dev);
> > +}
> > +
> > +static struct platform_driver vfio_platform_driver = {
> > +     .probe          = vfio_platform_probe,
> > +     .remove         = vfio_platform_remove,
> > +     .driver = {
> > +             .name   = "vfio-platform",
> > +             .owner  = THIS_MODULE,
> > +     },
> > +};
> > +
> > +module_platform_driver(vfio_platform_driver);
> > +
> > +MODULE_VERSION(DRIVER_VERSION);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR(DRIVER_AUTHOR);
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 30f630c..b022a25 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -158,6 +158,7 @@ struct vfio_device_info {
> >       __u32   flags;
> >  #define VFIO_DEVICE_FLAGS_RESET      (1 << 0)        /* Device supports reset */
> >  #define VFIO_DEVICE_FLAGS_PCI        (1 << 1)        /* vfio-pci device */
> > +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
> >       __u32   num_regions;    /* Max region index + 1 */
> >       __u32   num_irqs;       /* Max IRQ index + 1 */
> >  };
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

Re: [PATCHv7 10/26] vfio: platform: probe to devices on the platform bus

[Alex Williamson]

On Fri, 2014-09-26 at 17:30 +0200, Antonios Motakis wrote:
> On Wed, Sep 24, 2014 at 1:01 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> > > Driver to bind to Linux platform devices, and callbacks to discover their
> > > resources to be used by the main VFIO PLATFORM code.
> > >
> > > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > > ---
> > >  drivers/vfio/platform/vfio_platform.c | 96 +++++++++++++++++++++++++++++++++++
> > >  include/uapi/linux/vfio.h             |  1 +
> > >  2 files changed, 97 insertions(+)
> > >  create mode 100644 drivers/vfio/platform/vfio_platform.c
> > >
> > > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
> > > new file mode 100644
> > > index 0000000..024c026
> > > --- /dev/null
> > > +++ b/drivers/vfio/platform/vfio_platform.c
> > > @@ -0,0 +1,96 @@
> > > +/*
> > > + * Copyright (C) 2013 - Virtual Open Systems
> > > + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License, version 2, as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/eventfd.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/iommu.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/notifier.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/types.h>
> > > +#include <linux/uaccess.h>
> > > +#include <linux/vfio.h>
> > > +#include <linux/io.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/irq.h>
> > > +
> > > +#include "vfio_platform_private.h"
> > > +
> > > +#define DRIVER_VERSION  "0.7"
> > > +#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
> > > +#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
> > > +
> > > +/* probing devices from the linux platform bus */
> > > +
> > > +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
> > > +                                             int i)
> > > +{
> > > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> > > +
> > > +     return platform_get_resource(pdev, IORESOURCE_MEM, i);
> >
> > ARM may only support IORESOURCE_MEM, but I don't think platform devices
> > are limited to MMIO, right?  vfio-platform shouldn't be either.
> >
> 
> Indeed. Should we however implement this lacking a target to verify it
> is working correctly?
> 
> Leaving it out would mean PIO resources for those devices would not be
> exposed before an update to VFIO, but we wouldn't have to break
> backward compatibility I think.
> 
> Would you prefer to have it implemented regardless?

I think we need to have PIO figured out at least enough to have stubbed
read/write handlers that could be filled in by someone with test
hardware.  I'm not sure I fully understand how a user associates a
region index to a device tree description, whether it's ordering or
something more complicated, so I'm not sure if simply listing all the
PIO resources after the MMIO resources is sufficient and compatible.
Maybe you have some thoughts on that.  Thanks,

Alex

> > > +}
> > > +
> > > +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
> > > +{
> > > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
> > > +
> > > +     return platform_get_irq(pdev, i);
> > > +}
> > > +
> > > +
> > > +static int vfio_platform_probe(struct platform_device *pdev)
> > > +{
> > > +     struct vfio_platform_device *vdev;
> > > +     int ret;
> > > +
> > > +     vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> > > +     if (!vdev)
> > > +             return -ENOMEM;
> > > +
> > > +     vdev->opaque = (void *) pdev;
> > > +     vdev->name = pdev->name;
> > > +     vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
> > > +     vdev->get_resource = get_platform_resource;
> > > +     vdev->get_irq = get_platform_irq;
> > > +
> > > +     ret = vfio_platform_probe_common(vdev, &pdev->dev);
> > > +     if (ret)
> > > +             kfree(vdev);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static int vfio_platform_remove(struct platform_device *pdev)
> > > +{
> > > +     return vfio_platform_remove_common(&pdev->dev);
> > > +}
> > > +
> > > +static struct platform_driver vfio_platform_driver = {
> > > +     .probe          = vfio_platform_probe,
> > > +     .remove         = vfio_platform_remove,
> > > +     .driver = {
> > > +             .name   = "vfio-platform",
> > > +             .owner  = THIS_MODULE,
> > > +     },
> > > +};
> > > +
> > > +module_platform_driver(vfio_platform_driver);
> > > +
> > > +MODULE_VERSION(DRIVER_VERSION);
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_AUTHOR(DRIVER_AUTHOR);
> > > +MODULE_DESCRIPTION(DRIVER_DESC);
> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > > index 30f630c..b022a25 100644
> > > --- a/include/uapi/linux/vfio.h
> > > +++ b/include/uapi/linux/vfio.h
> > > @@ -158,6 +158,7 @@ struct vfio_device_info {
> > >       __u32   flags;
> > >  #define VFIO_DEVICE_FLAGS_RESET      (1 << 0)        /* Device supports reset */
> > >  #define VFIO_DEVICE_FLAGS_PCI        (1 << 1)        /* vfio-pci device */
> > > +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
> > >       __u32   num_regions;    /* Max region index + 1 */
> > >       __u32   num_irqs;       /* Max IRQ index + 1 */
> > >  };
> >
> >
> >
> 
> 
> 

Re: [PATCHv7 10/26] vfio: platform: probe to devices on the platform bus

[Antonios Motakis]

On Fri, Sep 26, 2014 at 10:18 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Fri, 2014-09-26 at 17:30 +0200, Antonios Motakis wrote:
>> On Wed, Sep 24, 2014 at 1:01 AM, Alex Williamson
>> <alex.williamson@redhat.com> wrote:
>> >
>> > On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> > > Driver to bind to Linux platform devices, and callbacks to discover their
>> > > resources to be used by the main VFIO PLATFORM code.
>> > >
>> > > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> > > ---
>> > >  drivers/vfio/platform/vfio_platform.c | 96 +++++++++++++++++++++++++++++++++++
>> > >  include/uapi/linux/vfio.h             |  1 +
>> > >  2 files changed, 97 insertions(+)
>> > >  create mode 100644 drivers/vfio/platform/vfio_platform.c
>> > >
>> > > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
>> > > new file mode 100644
>> > > index 0000000..024c026
>> > > --- /dev/null
>> > > +++ b/drivers/vfio/platform/vfio_platform.c
>> > > @@ -0,0 +1,96 @@
>> > > +/*
>> > > + * Copyright (C) 2013 - Virtual Open Systems
>> > > + * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
>> > > + *
>> > > + * This program is free software; you can redistribute it and/or modify
>> > > + * it under the terms of the GNU General Public License, version 2, as
>> > > + * published by the Free Software Foundation.
>> > > + *
>> > > + * 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.
>> > > + */
>> > > +
>> > > +#include <linux/device.h>
>> > > +#include <linux/eventfd.h>
>> > > +#include <linux/interrupt.h>
>> > > +#include <linux/iommu.h>
>> > > +#include <linux/module.h>
>> > > +#include <linux/mutex.h>
>> > > +#include <linux/notifier.h>
>> > > +#include <linux/pm_runtime.h>
>> > > +#include <linux/slab.h>
>> > > +#include <linux/types.h>
>> > > +#include <linux/uaccess.h>
>> > > +#include <linux/vfio.h>
>> > > +#include <linux/io.h>
>> > > +#include <linux/platform_device.h>
>> > > +#include <linux/irq.h>
>> > > +
>> > > +#include "vfio_platform_private.h"
>> > > +
>> > > +#define DRIVER_VERSION  "0.7"
>> > > +#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
>> > > +#define DRIVER_DESC     "VFIO for platform devices - User Level meta-driver"
>> > > +
>> > > +/* probing devices from the linux platform bus */
>> > > +
>> > > +static struct resource *get_platform_resource(struct vfio_platform_device *vdev,
>> > > +                                             int i)
>> > > +{
>> > > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
>> > > +
>> > > +     return platform_get_resource(pdev, IORESOURCE_MEM, i);
>> >
>> > ARM may only support IORESOURCE_MEM, but I don't think platform devices
>> > are limited to MMIO, right?  vfio-platform shouldn't be either.
>> >
>>
>> Indeed. Should we however implement this lacking a target to verify it
>> is working correctly?
>>
>> Leaving it out would mean PIO resources for those devices would not be
>> exposed before an update to VFIO, but we wouldn't have to break
>> backward compatibility I think.
>>
>> Would you prefer to have it implemented regardless?
>
> I think we need to have PIO figured out at least enough to have stubbed
> read/write handlers that could be filled in by someone with test
> hardware.  I'm not sure I fully understand how a user associates a
> region index to a device tree description, whether it's ordering or
> something more complicated, so I'm not sure if simply listing all the
> PIO resources after the MMIO resources is sufficient and compatible.
> Maybe you have some thoughts on that.  Thanks,

You are right. I'm not aware if the Linux calls used are guaranteed to
preserve ordering of PIO/MMIO resources, I will investigate a bit on
that.

>
> Alex
>
>> > > +}
>> > > +
>> > > +static int get_platform_irq(struct vfio_platform_device *vdev, int i)
>> > > +{
>> > > +     struct platform_device *pdev = (struct platform_device *) vdev->opaque;
>> > > +
>> > > +     return platform_get_irq(pdev, i);
>> > > +}
>> > > +
>> > > +
>> > > +static int vfio_platform_probe(struct platform_device *pdev)
>> > > +{
>> > > +     struct vfio_platform_device *vdev;
>> > > +     int ret;
>> > > +
>> > > +     vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
>> > > +     if (!vdev)
>> > > +             return -ENOMEM;
>> > > +
>> > > +     vdev->opaque = (void *) pdev;
>> > > +     vdev->name = pdev->name;
>> > > +     vdev->flags = VFIO_DEVICE_FLAGS_PLATFORM;
>> > > +     vdev->get_resource = get_platform_resource;
>> > > +     vdev->get_irq = get_platform_irq;
>> > > +
>> > > +     ret = vfio_platform_probe_common(vdev, &pdev->dev);
>> > > +     if (ret)
>> > > +             kfree(vdev);
>> > > +
>> > > +     return ret;
>> > > +}
>> > > +
>> > > +static int vfio_platform_remove(struct platform_device *pdev)
>> > > +{
>> > > +     return vfio_platform_remove_common(&pdev->dev);
>> > > +}
>> > > +
>> > > +static struct platform_driver vfio_platform_driver = {
>> > > +     .probe          = vfio_platform_probe,
>> > > +     .remove         = vfio_platform_remove,
>> > > +     .driver = {
>> > > +             .name   = "vfio-platform",
>> > > +             .owner  = THIS_MODULE,
>> > > +     },
>> > > +};
>> > > +
>> > > +module_platform_driver(vfio_platform_driver);
>> > > +
>> > > +MODULE_VERSION(DRIVER_VERSION);
>> > > +MODULE_LICENSE("GPL v2");
>> > > +MODULE_AUTHOR(DRIVER_AUTHOR);
>> > > +MODULE_DESCRIPTION(DRIVER_DESC);
>> > > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> > > index 30f630c..b022a25 100644
>> > > --- a/include/uapi/linux/vfio.h
>> > > +++ b/include/uapi/linux/vfio.h
>> > > @@ -158,6 +158,7 @@ struct vfio_device_info {
>> > >       __u32   flags;
>> > >  #define VFIO_DEVICE_FLAGS_RESET      (1 << 0)        /* Device supports reset */
>> > >  #define VFIO_DEVICE_FLAGS_PCI        (1 << 1)        /* vfio-pci device */
>> > > +#define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)  /* vfio-platform device */
>> > >       __u32   num_regions;    /* Max region index + 1 */
>> > >       __u32   num_irqs;       /* Max IRQ index + 1 */
>> > >  };
>> >
>> >
>> >
>>
>>
>>
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 11/26] vfio: platform: add the VFIO PLATFORM module to Kconfig

[Antonios Motakis]

Enable building the VFIO PLATFORM driver that allows to use Linux platform
devices with VFIO.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Kconfig           | 1 +
 drivers/vfio/Makefile          | 1 +
 drivers/vfio/platform/Kconfig  | 9 +++++++++
 drivers/vfio/platform/Makefile | 4 ++++
 4 files changed, 15 insertions(+)
 create mode 100644 drivers/vfio/platform/Kconfig
 create mode 100644 drivers/vfio/platform/Makefile

diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
index a0abe04..962fb80 100644
--- a/drivers/vfio/Kconfig
+++ b/drivers/vfio/Kconfig
@@ -27,3 +27,4 @@ menuconfig VFIO
 	  If you don't know what to do here, say N.
 
 source "drivers/vfio/pci/Kconfig"
+source "drivers/vfio/platform/Kconfig"
diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index 0b035b1..dadf0ca 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
 obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
 obj-$(CONFIG_VFIO_PCI) += pci/
+obj-$(CONFIG_VFIO_PLATFORM) += platform/
diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
new file mode 100644
index 0000000..c51af17
--- /dev/null
+++ b/drivers/vfio/platform/Kconfig
@@ -0,0 +1,9 @@
+config VFIO_PLATFORM
+	tristate "VFIO support for platform devices"
+	depends on VFIO && EVENTFD && ARM
+	help
+	  Support for platform devices with VFIO. This is required to make
+	  use of platform devices present on the system using the VFIO
+	  framework.
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
new file mode 100644
index 0000000..279862b
--- /dev/null
+++ b/drivers/vfio/platform/Makefile
@@ -0,0 +1,4 @@
+
+vfio-platform-y := vfio_platform.o vfio_platform_common.o
+
+obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
-- 
1.8.3.2

[PATCHv7 12/26] vfio: amba: VFIO support for AMBA devices

[Antonios Motakis]

Add support for discovering AMBA devices with VFIO and handle them
similarly to Linux platform devices.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_amba.c | 108 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h         |   1 +
 2 files changed, 109 insertions(+)
 create mode 100644 drivers/vfio/platform/vfio_amba.c

diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
new file mode 100644
index 0000000..e8c5e1a
--- /dev/null
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -0,0 +1,108 @@
+/*
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/amba/bus.h>
+
+#include "vfio_platform_private.h"
+
+#define DRIVER_VERSION  "0.7"
+#define DRIVER_AUTHOR   "Antonios Motakis <a.motakis@virtualopensystems.com>"
+#define DRIVER_DESC     "VFIO for AMBA devices - User Level meta-driver"
+
+/* probing devices from the AMBA bus */
+
+static struct resource *get_amba_resource(struct vfio_platform_device *vdev,
+						int i)
+{
+	struct amba_device *adev = (struct amba_device *) vdev->opaque;
+
+	if (i == 0)
+		return &adev->res;
+
+	return NULL;
+}
+
+static int get_amba_irq(struct vfio_platform_device *vdev, int i)
+{
+	struct amba_device *adev = (struct amba_device *) vdev->opaque;
+
+	if (i < AMBA_NR_IRQS)
+		return adev->irq[i];
+
+	return 0;
+}
+
+static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
+{
+
+	struct vfio_platform_device *vdev;
+	int ret;
+
+	vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
+	if (!vdev)
+		return -ENOMEM;
+
+	vdev->opaque = (void *) adev;
+	vdev->name = "vfio-amba-dev";
+	vdev->flags = VFIO_DEVICE_FLAGS_AMBA;
+	vdev->get_resource = get_amba_resource;
+	vdev->get_irq = get_amba_irq;
+
+	ret = vfio_platform_probe_common(vdev, &adev->dev);
+	if (ret)
+		kfree(vdev);
+
+	return ret;
+}
+
+static int vfio_amba_remove(struct amba_device *adev)
+{
+	return vfio_platform_remove_common(&adev->dev);
+}
+
+static struct amba_id pl330_ids[] = {
+	{ 0, 0 },
+};
+
+MODULE_DEVICE_TABLE(amba, pl330_ids);
+
+static struct amba_driver vfio_amba_driver = {
+	.probe = vfio_amba_probe,
+	.remove = vfio_amba_remove,
+	.id_table = pl330_ids,
+	.drv = {
+		.name = "vfio-amba",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_amba_driver(vfio_amba_driver);
+
+MODULE_VERSION(DRIVER_VERSION);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index b022a25..72f121f 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -159,6 +159,7 @@ struct vfio_device_info {
 #define VFIO_DEVICE_FLAGS_RESET	(1 << 0)	/* Device supports reset */
 #define VFIO_DEVICE_FLAGS_PCI	(1 << 1)	/* vfio-pci device */
 #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2)	/* vfio-platform device */
+#define VFIO_DEVICE_FLAGS_AMBA  (1 << 3)	/* vfio-amba device */
 	__u32	num_regions;	/* Max region index + 1 */
 	__u32	num_irqs;	/* Max IRQ index + 1 */
 };
-- 
1.8.3.2

[PATCHv7 13/26] vfio: amba: add the VFIO for AMBA devices module to Kconfig

[Antonios Motakis]

Enable building the VFIO AMBA driver. VFIO_AMBA depends on VFIO_PLATFORM,
since it is sharing a portion of the code, and it is essentially implemented
as a platform device whose resources are discovered via AMBA specific APIs
in the kernel.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/Kconfig  | 10 ++++++++++
 drivers/vfio/platform/Makefile |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index c51af17..8b97786 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig
@@ -7,3 +7,13 @@ config VFIO_PLATFORM
 	  framework.
 
 	  If you don't know what to do here, say N.
+
+config VFIO_AMBA
+	tristate "VFIO support for AMBA devices"
+	depends on VFIO && VFIO_PLATFORM && EVENTFD && ARM_AMBA
+	help
+	  Support for ARM AMBA devices with VFIO. This is required to make
+	  use of ARM AMBA devices present on the system using the VFIO
+	  framework.
+
+	  If you don't know what to do here, say N.
diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 279862b..1957170 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -2,3 +2,7 @@
 vfio-platform-y := vfio_platform.o vfio_platform_common.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
+
+vfio-amba-y := vfio_amba.o
+
+obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
-- 
1.8.3.2

Re: [PATCHv7 13/26] vfio: amba: add the VFIO for AMBA devices module to Kconfig

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> Enable building the VFIO AMBA driver. VFIO_AMBA depends on VFIO_PLATFORM,
> since it is sharing a portion of the code, and it is essentially implemented
> as a platform device whose resources are discovered via AMBA specific APIs
> in the kernel.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/Kconfig  | 10 ++++++++++
>  drivers/vfio/platform/Makefile |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> index c51af17..8b97786 100644
> --- a/drivers/vfio/platform/Kconfig
> +++ b/drivers/vfio/platform/Kconfig
> @@ -7,3 +7,13 @@ config VFIO_PLATFORM
>  	  framework.
>  
>  	  If you don't know what to do here, say N.
> +
> +config VFIO_AMBA
> +	tristate "VFIO support for AMBA devices"
> +	depends on VFIO && VFIO_PLATFORM && EVENTFD && ARM_AMBA

nit, VFIO_PLATFORM already depends on VFIO && EVENTFD

> +	help
> +	  Support for ARM AMBA devices with VFIO. This is required to make
> +	  use of ARM AMBA devices present on the system using the VFIO
> +	  framework.
> +
> +	  If you don't know what to do here, say N.
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 279862b..1957170 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -2,3 +2,7 @@
>  vfio-platform-y := vfio_platform.o vfio_platform_common.o
>  
>  obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
> +
> +vfio-amba-y := vfio_amba.o
> +
> +obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o

Re: [PATCHv7 13/26] vfio: amba: add the VFIO for AMBA devices module to Kconfig

[Antonios Motakis]

On Wed, Sep 24, 2014 at 1:11 AM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> Enable building the VFIO AMBA driver. VFIO_AMBA depends on VFIO_PLATFORM,
>> since it is sharing a portion of the code, and it is essentially implemented
>> as a platform device whose resources are discovered via AMBA specific APIs
>> in the kernel.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/Kconfig  | 10 ++++++++++
>>  drivers/vfio/platform/Makefile |  4 ++++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
>> index c51af17..8b97786 100644
>> --- a/drivers/vfio/platform/Kconfig
>> +++ b/drivers/vfio/platform/Kconfig
>> @@ -7,3 +7,13 @@ config VFIO_PLATFORM
>>         framework.
>>
>>         If you don't know what to do here, say N.
>> +
>> +config VFIO_AMBA
>> +     tristate "VFIO support for AMBA devices"
>> +     depends on VFIO && VFIO_PLATFORM && EVENTFD && ARM_AMBA
>
> nit, VFIO_PLATFORM already depends on VFIO && EVENTFD

Ack

>
>> +     help
>> +       Support for ARM AMBA devices with VFIO. This is required to make
>> +       use of ARM AMBA devices present on the system using the VFIO
>> +       framework.
>> +
>> +       If you don't know what to do here, say N.
>> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
>> index 279862b..1957170 100644
>> --- a/drivers/vfio/platform/Makefile
>> +++ b/drivers/vfio/platform/Makefile
>> @@ -2,3 +2,7 @@
>>  vfio-platform-y := vfio_platform.o vfio_platform_common.o
>>
>>  obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
>> +
>> +vfio-amba-y := vfio_amba.o
>> +
>> +obj-$(CONFIG_VFIO_AMBA) += vfio-amba.o
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 14/26] vfio/platform: return info for bound device

[Antonios Motakis]

A VFIO userspace driver will start by opening the VFIO device
that corresponds to an IOMMU group, and will use the ioctl interface
to get the basic device info, such as number of memory regions and
interrupts, and their properties. This patch enables the
VFIO_DEVICE_GET_INFO ioctl call.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 07d02dc..ba5edfd 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -43,10 +43,27 @@ static int vfio_platform_open(void *device_data)
 static long vfio_platform_ioctl(void *device_data,
 			   unsigned int cmd, unsigned long arg)
 {
-	if (cmd == VFIO_DEVICE_GET_INFO)
-		return -EINVAL;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned long minsz;
+
+	if (cmd == VFIO_DEVICE_GET_INFO) {
+		struct vfio_device_info info;
+
+		minsz = offsetofend(struct vfio_device_info, num_irqs);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		info.flags = vdev->flags;
+		info.num_regions = 0;
+		info.num_irqs = 0;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
-	else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
+	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
 		return -EINVAL;
 
 	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
-- 
1.8.3.2

[PATCHv7 15/26] vfio/platform: return info for device memory mapped IO regions

[Antonios Motakis]

This patch enables the IOCTLs VFIO_DEVICE_GET_REGION_INFO ioctl call,
which allows the user to learn about the available MMIO resources of
a device.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 84 +++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h | 19 ++++++
 2 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index ba5edfd..469bdcb 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -27,17 +27,73 @@
 
 #include "vfio_platform_private.h"
 
+static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
+{
+	int cnt = 0, i;
+
+	while (vdev->get_resource(vdev, cnt))
+		cnt++;
+
+	vdev->regions = kcalloc(cnt, sizeof(struct vfio_platform_region),
+				GFP_KERNEL);
+	if (!vdev->regions)
+		return -ENOMEM;
+
+	for (i = 0; i < cnt;  i++) {
+		struct resource *res =
+			vdev->get_resource(vdev, i);
+
+		if (!res)
+			goto err;
+
+		vdev->regions[i].addr = res->start;
+		vdev->regions[i].size = resource_size(res);
+		vdev->regions[i].flags = 0;
+	}
+
+	vdev->num_regions = cnt;
+
+	return 0;
+err:
+	kfree(vdev->regions);
+	return -EINVAL;
+}
+
+static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
+{
+	vdev->num_regions = 0;
+	kfree(vdev->regions);
+}
+
 static void vfio_platform_release(void *device_data)
 {
+	struct vfio_platform_device *vdev = device_data;
+
+	if (atomic_dec_and_test(&vdev->refcnt))
+		vfio_platform_regions_cleanup(vdev);
+
 	module_put(THIS_MODULE);
 }
 
 static int vfio_platform_open(void *device_data)
 {
+	struct vfio_platform_device *vdev = device_data;
+	int ret;
+
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
+	if (atomic_inc_return(&vdev->refcnt) == 1) {
+		ret = vfio_platform_regions_init(vdev);
+		if (ret)
+			goto err_reg;
+	}
+
 	return 0;
+
+err_reg:
+	module_put(THIS_MODULE);
+	return ret;
 }
 
 static long vfio_platform_ioctl(void *device_data,
@@ -58,18 +114,36 @@ static long vfio_platform_ioctl(void *device_data,
 			return -EINVAL;
 
 		info.flags = vdev->flags;
-		info.num_regions = 0;
+		info.num_regions = vdev->num_regions;
 		info.num_irqs = 0;
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
-	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_GET_REGION_INFO) {
+		struct vfio_region_info info;
+
+		minsz = offsetofend(struct vfio_region_info, offset);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if (info.index >= vdev->num_regions)
+			return -EINVAL;
+
+		/* map offset to the physical address  */
+		info.offset = VFIO_PLATFORM_INDEX_TO_OFFSET(info.index);
+		info.size = vdev->regions[info.index].size;
+		info.flags = vdev->regions[info.index].flags;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
-	else if (cmd == VFIO_DEVICE_GET_IRQ_INFO)
+	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
 		return -EINVAL;
 
-	else if (cmd == VFIO_DEVICE_SET_IRQS)
+	} else if (cmd == VFIO_DEVICE_SET_IRQS)
 		return -EINVAL;
 
 	else if (cmd == VFIO_DEVICE_RESET)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index ef76737..383164a 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -15,7 +15,26 @@
 #ifndef VFIO_PLATFORM_PRIVATE_H
 #define VFIO_PLATFORM_PRIVATE_H
 
+#define VFIO_PLATFORM_OFFSET_SHIFT   40
+#define VFIO_PLATFORM_OFFSET_MASK (((u64)(1) << VFIO_PLATFORM_OFFSET_SHIFT) - 1)
+
+#define VFIO_PLATFORM_OFFSET_TO_INDEX(off)	\
+	(off >> VFIO_PLATFORM_OFFSET_SHIFT)
+
+#define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
+	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
+
+struct vfio_platform_region {
+	u64			addr;
+	resource_size_t		size;
+	u32			flags;
+};
+
 struct vfio_platform_device {
+	struct vfio_platform_region	*regions;
+	u32				num_regions;
+	atomic_t			refcnt;
+
 	/*
 	 * These fields should be filled by the bus driver at binding time
 	 */
-- 
1.8.3.2

[PATCHv7 16/26] vfio/platform: read and write support for the device fd

[Antonios Motakis]

VFIO returns a file descriptor which we can use to manipulate the memory
regions of the device. Usually, the user will mmap memory regions that are
addressable on page boundaries, however for memory regions where this is
not the case we cannot provide mmap functionality due to security concerns.
For this reason we also need allow to read and write to the memory regions
via the file descriptor.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 121 +++++++++++++++++++++++++-
 drivers/vfio/platform/vfio_platform_private.h |   1 +
 2 files changed, 119 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 469bdcb..589b226 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -48,7 +48,8 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 
 		vdev->regions[i].addr = res->start;
 		vdev->regions[i].size = resource_size(res);
-		vdev->regions[i].flags = 0;
+		vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_READ
+					| VFIO_REGION_INFO_FLAG_WRITE;
 	}
 
 	vdev->num_regions = cnt;
@@ -61,6 +62,11 @@ err:
 
 static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 {
+	int i;
+
+	for (i = 0; i < vdev->num_regions; i++)
+		iounmap(vdev->regions[i].ioaddr);
+
 	vdev->num_regions = 0;
 	kfree(vdev->regions);
 }
@@ -155,13 +161,122 @@ static long vfio_platform_ioctl(void *device_data,
 static ssize_t vfio_platform_read(void *device_data, char __user *buf,
 			     size_t count, loff_t *ppos)
 {
-	return -EINVAL;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
+	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
+	unsigned int done = 0;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	if (!vdev->regions[index].ioaddr) {
+		vdev->regions[index].ioaddr =
+			ioremap_nocache(vdev->regions[index].addr,
+					vdev->regions[index].size);
+
+		if (!vdev->regions[index].ioaddr)
+			return -ENOMEM;
+	}
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			val = ioread32(vdev->regions[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 4))
+				goto err;
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			val = ioread16(vdev->regions[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 2))
+				goto err;
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			val = ioread8(vdev->regions[index].ioaddr + off);
+			if (copy_to_user(buf, &val, 1))
+				goto err;
+
+			filled = 1;
+		}
+
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
 }
 
 static ssize_t vfio_platform_write(void *device_data, const char __user *buf,
 			      size_t count, loff_t *ppos)
 {
-	return -EINVAL;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index = VFIO_PLATFORM_OFFSET_TO_INDEX(*ppos);
+	loff_t off = *ppos & VFIO_PLATFORM_OFFSET_MASK;
+	unsigned int done = 0;
+
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+
+	if (!vdev->regions[index].ioaddr) {
+		vdev->regions[index].ioaddr =
+			ioremap_nocache(vdev->regions[index].addr,
+					vdev->regions[index].size);
+
+		if (!vdev->regions[index].ioaddr)
+			return -ENOMEM;
+	}
+
+	while (count) {
+		size_t filled;
+
+		if (count >= 4 && !(off % 4)) {
+			u32 val;
+
+			if (copy_from_user(&val, buf, 4))
+				goto err;
+			iowrite32(val, vdev->regions[index].ioaddr + off);
+
+			filled = 4;
+		} else if (count >= 2 && !(off % 2)) {
+			u16 val;
+
+			if (copy_from_user(&val, buf, 2))
+				goto err;
+			iowrite16(val, vdev->regions[index].ioaddr + off);
+
+			filled = 2;
+		} else {
+			u8 val;
+
+			if (copy_from_user(&val, buf, 1))
+				goto err;
+			iowrite8(val, vdev->regions[index].ioaddr + off);
+
+			filled = 1;
+		}
+
+		count -= filled;
+		done += filled;
+		off += filled;
+		buf += filled;
+	}
+
+	return done;
+err:
+	return -EFAULT;
 }
 
 static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 383164a..4145fd6 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -28,6 +28,7 @@ struct vfio_platform_region {
 	u64			addr;
 	resource_size_t		size;
 	u32			flags;
+	void __iomem		*ioaddr;
 };
 
 struct vfio_platform_device {
-- 
1.8.3.2

[PATCHv7 17/26] vfio/platform: support MMAP of MMIO regions

[Antonios Motakis]

Allow to memory map the MMIO regions of the device so userspace can
directly access them.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c | 40 +++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 589b226..5551d32 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -50,6 +50,11 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 		vdev->regions[i].size = resource_size(res);
 		vdev->regions[i].flags = VFIO_REGION_INFO_FLAG_READ
 					| VFIO_REGION_INFO_FLAG_WRITE;
+		/* Only regions addressed with PAGE granularity may be MMAPed
+		 * securely. */
+		if (!(vdev->regions[i].addr & ~PAGE_MASK)
+				&& !(vdev->regions[i].size & ~PAGE_MASK))
+			vdev->regions[i].flags |= VFIO_REGION_INFO_FLAG_MMAP;
 	}
 
 	vdev->num_regions = cnt;
@@ -281,7 +286,40 @@ err:
 
 static int vfio_platform_mmap(void *device_data, struct vm_area_struct *vma)
 {
-	return -EINVAL;
+	struct vfio_platform_device *vdev = device_data;
+	unsigned int index;
+	u64 req_len, pgoff, req_start;
+	struct vfio_platform_region regions;
+
+	index = vma->vm_pgoff >> (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT);
+
+	if (vma->vm_end < vma->vm_start)
+		return -EINVAL;
+	if ((vma->vm_flags & VM_SHARED) == 0)
+		return -EINVAL;
+	if (index >= vdev->num_regions)
+		return -EINVAL;
+	if (vma->vm_start & ~PAGE_MASK)
+		return -EINVAL;
+	if (vma->vm_end & ~PAGE_MASK)
+		return -EINVAL;
+
+	regions = vdev->regions[index];
+
+	req_len = vma->vm_end - vma->vm_start;
+	pgoff = vma->vm_pgoff &
+		((1U << (VFIO_PLATFORM_OFFSET_SHIFT - PAGE_SHIFT)) - 1);
+	req_start = pgoff << PAGE_SHIFT;
+
+	if (regions.size < PAGE_SIZE || req_start + req_len > regions.size)
+		return -EINVAL;
+
+	vma->vm_private_data = vdev;
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	vma->vm_pgoff = (regions.addr >> PAGE_SHIFT) + pgoff;
+
+	return remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
+			       req_len, vma->vm_page_prot);
 }
 
 static const struct vfio_device_ops vfio_platform_ops = {
-- 
1.8.3.2

[PATCHv7 18/26] vfio/platform: return IRQ info

[Antonios Motakis]

Return information for the interrupts exposed by the device.
This patch extends VFIO_DEVICE_GET_INFO with the number of IRQs
and enables VFIO_DEVICE_GET_IRQ_INFO.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/Makefile                |  2 +-
 drivers/vfio/platform/vfio_platform_common.c  | 30 ++++++++++++--
 drivers/vfio/platform/vfio_platform_irq.c     | 59 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h | 10 +++++
 4 files changed, 97 insertions(+), 4 deletions(-)
 create mode 100644 drivers/vfio/platform/vfio_platform_irq.c

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 1957170..81de144 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile
@@ -1,5 +1,5 @@
 
-vfio-platform-y := vfio_platform.o vfio_platform_common.o
+vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
 
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
 
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 5551d32..6dccf22 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -80,8 +80,10 @@ static void vfio_platform_release(void *device_data)
 {
 	struct vfio_platform_device *vdev = device_data;
 
-	if (atomic_dec_and_test(&vdev->refcnt))
+	if (atomic_dec_and_test(&vdev->refcnt)) {
 		vfio_platform_regions_cleanup(vdev);
+		vfio_platform_irq_cleanup(vdev);
+	}
 
 	module_put(THIS_MODULE);
 }
@@ -98,10 +100,16 @@ static int vfio_platform_open(void *device_data)
 		ret = vfio_platform_regions_init(vdev);
 		if (ret)
 			goto err_reg;
+
+		ret = vfio_platform_irq_init(vdev);
+		if (ret)
+			goto err_irq;
 	}
 
 	return 0;
 
+err_irq:
+	vfio_platform_regions_cleanup(vdev);
 err_reg:
 	module_put(THIS_MODULE);
 	return ret;
@@ -126,7 +134,7 @@ static long vfio_platform_ioctl(void *device_data,
 
 		info.flags = vdev->flags;
 		info.num_regions = vdev->num_regions;
-		info.num_irqs = 0;
+		info.num_irqs = vdev->num_irqs;
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
@@ -152,7 +160,23 @@ static long vfio_platform_ioctl(void *device_data,
 		return copy_to_user((void __user *)arg, &info, minsz);
 
 	} else if (cmd == VFIO_DEVICE_GET_IRQ_INFO) {
-		return -EINVAL;
+		struct vfio_irq_info info;
+
+		minsz = offsetofend(struct vfio_irq_info, count);
+
+		if (copy_from_user(&info, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (info.argsz < minsz)
+			return -EINVAL;
+
+		if (info.index >= vdev->num_irqs)
+			return -EINVAL;
+
+		info.flags = vdev->irqs[info.index].flags;
+		info.count = vdev->irqs[info.index].count;
+
+		return copy_to_user((void __user *)arg, &info, minsz);
 
 	} else if (cmd == VFIO_DEVICE_SET_IRQS)
 		return -EINVAL;
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
new file mode 100644
index 0000000..d99c71c
--- /dev/null
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -0,0 +1,59 @@
+/*
+ * VFIO platform devices interrupt handling
+ *
+ * Copyright (C) 2013 - Virtual Open Systems
+ * Author: Antonios Motakis <a.motakis@virtualopensystems.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, version 2, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/eventfd.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+#include <linux/vfio.h>
+#include <linux/platform_device.h>
+#include <linux/irq.h>
+
+#include "vfio_platform_private.h"
+
+int vfio_platform_irq_init(struct vfio_platform_device *vdev)
+{
+	int cnt = 0, i;
+
+	while (vdev->get_irq(vdev, cnt) > 0)
+		cnt++;
+
+	vdev->irqs = kcalloc(cnt, sizeof(struct vfio_platform_irq), GFP_KERNEL);
+	if (!vdev->irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < cnt; i++) {
+		vdev->irqs[i].flags = 0;
+		vdev->irqs[i].count = 1;
+	}
+
+	vdev->num_irqs = cnt;
+
+	return 0;
+}
+
+void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
+{
+	vdev->num_irqs = 0;
+	kfree(vdev->irqs);
+}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 4145fd6..c5c7a7b 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -24,6 +24,11 @@
 #define VFIO_PLATFORM_INDEX_TO_OFFSET(index)	\
 	((u64)(index) << VFIO_PLATFORM_OFFSET_SHIFT)
 
+struct vfio_platform_irq {
+	u32			flags;
+	u32			count;
+};
+
 struct vfio_platform_region {
 	u64			addr;
 	resource_size_t		size;
@@ -34,6 +39,8 @@ struct vfio_platform_region {
 struct vfio_platform_device {
 	struct vfio_platform_region	*regions;
 	u32				num_regions;
+	struct vfio_platform_irq	*irqs;
+	u32				num_irqs;
 	atomic_t			refcnt;
 
 	/*
@@ -52,4 +59,7 @@ extern int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 				      struct device *dev);
 extern int vfio_platform_remove_common(struct device *dev);
 
+extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
+extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

[PATCHv7 19/26] vfio/platform: initial interrupts support code

[Antonios Motakis]

This patch is a skeleton for the VFIO_DEVICE_SET_IRQS IOCTL, around which
most IRQ functionality is implemented in VFIO.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 41 ++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_irq.c     | 56 +++++++++++++++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  6 +++
 3 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6dccf22..8df0912 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -178,10 +178,43 @@ static long vfio_platform_ioctl(void *device_data,
 
 		return copy_to_user((void __user *)arg, &info, minsz);
 
-	} else if (cmd == VFIO_DEVICE_SET_IRQS)
-		return -EINVAL;
+	} else if (cmd == VFIO_DEVICE_SET_IRQS) {
+		struct vfio_irq_set hdr;
+		int ret = 0;
+
+		minsz = offsetofend(struct vfio_irq_set, count);
+
+		if (copy_from_user(&hdr, (void __user *)arg, minsz))
+			return -EFAULT;
+
+		if (hdr.argsz < minsz)
+			return -EINVAL;
+
+		if (hdr.index >= vdev->num_irqs)
+			return -EINVAL;
+
+		if (hdr.start != 0 || hdr.count > 1)
+			return -EINVAL;
 
-	else if (cmd == VFIO_DEVICE_RESET)
+		if (hdr.count == 0 &&
+			(!(hdr.flags & VFIO_IRQ_SET_DATA_NONE) ||
+			 !(hdr.flags & VFIO_IRQ_SET_ACTION_TRIGGER)))
+			return -EINVAL;
+
+		if (hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
+				  VFIO_IRQ_SET_ACTION_TYPE_MASK))
+			return -EINVAL;
+
+		mutex_lock(&vdev->igate);
+
+		ret = vfio_platform_set_irqs_ioctl(vdev, hdr.flags, hdr.index,
+						   hdr.start, hdr.count,
+						   (void *)arg+minsz);
+		mutex_unlock(&vdev->igate);
+
+		return ret;
+
+	} else if (cmd == VFIO_DEVICE_RESET)
 		return -EINVAL;
 
 	return -ENOTTY;
@@ -377,6 +410,8 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
+	mutex_init(&vdev->igate);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index d99c71c..007b386 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -31,6 +31,53 @@
 
 #include "vfio_platform_private.h"
 
+static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
+				    unsigned index, unsigned start,
+				    unsigned count, uint32_t flags, void *data)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
+				    unsigned index, unsigned start,
+				    unsigned count, uint32_t flags, void *data)
+{
+	return -EINVAL;
+}
+
+static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
+				     unsigned index, unsigned start,
+				     unsigned count, uint32_t flags, void *data)
+{
+	return -EINVAL;
+}
+
+int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
+				 uint32_t flags, unsigned index, unsigned start,
+				 unsigned count, void *data)
+{
+	int (*func)(struct vfio_platform_device *vdev, unsigned index,
+		    unsigned start, unsigned count, uint32_t flags,
+		    void *data) = NULL;
+
+	switch (flags & VFIO_IRQ_SET_ACTION_TYPE_MASK) {
+	case VFIO_IRQ_SET_ACTION_MASK:
+		func = vfio_platform_set_irq_mask;
+		break;
+	case VFIO_IRQ_SET_ACTION_UNMASK:
+		func = vfio_platform_set_irq_unmask;
+		break;
+	case VFIO_IRQ_SET_ACTION_TRIGGER:
+		func = vfio_platform_set_irq_trigger;
+		break;
+	}
+
+	if (!func)
+		return -ENOTTY;
+
+	return func(vdev, index, start, count, flags, data);
+}
+
 int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 {
 	int cnt = 0, i;
@@ -43,13 +90,22 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 		return -ENOMEM;
 
 	for (i = 0; i < cnt; i++) {
+		int hwirq = vdev->get_irq(vdev, i);
+
+		if (hwirq < 0)
+			goto err;
+
 		vdev->irqs[i].flags = 0;
 		vdev->irqs[i].count = 1;
+		vdev->irqs[i].hwirq = hwirq;
 	}
 
 	vdev->num_irqs = cnt;
 
 	return 0;
+err:
+	kfree(vdev->irqs);
+	return -EINVAL;
 }
 
 void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index c5c7a7b..4201b94 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -27,6 +27,7 @@
 struct vfio_platform_irq {
 	u32			flags;
 	u32			count;
+	int			hwirq;
 };
 
 struct vfio_platform_region {
@@ -42,6 +43,7 @@ struct vfio_platform_device {
 	struct vfio_platform_irq	*irqs;
 	u32				num_irqs;
 	atomic_t			refcnt;
+	struct mutex			igate;
 
 	/*
 	 * These fields should be filled by the bus driver at binding time
@@ -62,4 +64,8 @@ extern int vfio_platform_remove_common(struct device *dev);
 extern int vfio_platform_irq_init(struct vfio_platform_device *vdev);
 extern void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev);
 
+extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
+			uint32_t flags, unsigned index, unsigned start,
+			unsigned count, void *data);
+
 #endif /* VFIO_PLATFORM_PRIVATE_H */
-- 
1.8.3.2

[PATCHv7 20/26] vfio/platform: trigger an interrupt via eventfd

[Antonios Motakis]

This patch allows to set an eventfd for a patform device's interrupt,
and also to trigger the interrupt eventfd from userspace for testing.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_irq.c     | 89 ++++++++++++++++++++++++++-
 drivers/vfio/platform/vfio_platform_private.h |  2 +
 2 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 007b386..25a7825 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -45,11 +45,91 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
 	return -EINVAL;
 }
 
+static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
+{
+	struct vfio_platform_irq *irq_ctx = dev_id;
+
+	eventfd_signal(irq_ctx->trigger, 1);
+
+	return IRQ_HANDLED;
+}
+
+static int vfio_set_trigger(struct vfio_platform_device *vdev,
+			    int index, int fd)
+{
+	struct vfio_platform_irq *irq = &vdev->irqs[index];
+	struct eventfd_ctx *trigger;
+	int ret;
+
+	if (irq->trigger) {
+		free_irq(irq->hwirq, irq);
+		kfree(irq->name);
+		eventfd_ctx_put(irq->trigger);
+		irq->trigger = NULL;
+	}
+
+	if (fd < 0) /* Disable only */
+		return 0;
+
+	irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
+						irq->hwirq, vdev->name);
+	if (!irq->name)
+		return -ENOMEM;
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		kfree(irq->name);
+		return PTR_ERR(trigger);
+	}
+
+	irq->trigger = trigger;
+
+	ret = request_irq(irq->hwirq, vfio_irq_handler, 0, irq->name, irq);
+	if (ret) {
+		kfree(irq->name);
+		eventfd_ctx_put(trigger);
+		irq->trigger = NULL;
+		return ret;
+	}
+
+	return 0;
+}
+
 static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
 				     unsigned index, unsigned start,
 				     unsigned count, uint32_t flags, void *data)
 {
-	return -EINVAL;
+	struct vfio_platform_irq *irq = &vdev->irqs[index];
+	uint8_t irq_bitmap;
+	int32_t fd;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_NONE:
+		if (count == 0)
+			return vfio_set_trigger(vdev, index, -1);
+
+		vfio_irq_handler(irq->hwirq, irq);
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&irq_bitmap, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (irq_bitmap == 0x1) {
+			vfio_irq_handler(irq->hwirq, irq);
+			return 0;
+		}
+
+		return -EINVAL;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD:
+		if (copy_from_user(&fd, data, sizeof(int32_t)))
+			return -EFAULT;
+
+		return vfio_set_trigger(vdev, index, fd);
+	}
+
+	return -EFAULT;
 }
 
 int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
@@ -95,7 +175,7 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 		if (hwirq < 0)
 			goto err;
 
-		vdev->irqs[i].flags = 0;
+		vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
 		vdev->irqs[i].count = 1;
 		vdev->irqs[i].hwirq = hwirq;
 	}
@@ -110,6 +190,11 @@ err:
 
 void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
 {
+	int i;
+
+	for (i = 0; i < vdev->num_irqs; i++)
+		vfio_set_trigger(vdev, i, -1);
+
 	vdev->num_irqs = 0;
 	kfree(vdev->irqs);
 }
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 4201b94..765b371 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -28,6 +28,8 @@ struct vfio_platform_irq {
 	u32			flags;
 	u32			count;
 	int			hwirq;
+	char			*name;
+	struct eventfd_ctx	*trigger;
 };
 
 struct vfio_platform_region {
-- 
1.8.3.2

Re: [PATCHv7 20/26] vfio/platform: trigger an interrupt via eventfd

[Alex Williamson]

On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
> This patch allows to set an eventfd for a patform device's interrupt,
> and also to trigger the interrupt eventfd from userspace for testing.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/vfio/platform/vfio_platform_irq.c     | 89 ++++++++++++++++++++++++++-
>  drivers/vfio/platform/vfio_platform_private.h |  2 +
>  2 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index 007b386..25a7825 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -45,11 +45,91 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
>  	return -EINVAL;
>  }
>  
> +static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
> +{
> +	struct vfio_platform_irq *irq_ctx = dev_id;
> +
> +	eventfd_signal(irq_ctx->trigger, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int vfio_set_trigger(struct vfio_platform_device *vdev,
> +			    int index, int fd)
> +{
> +	struct vfio_platform_irq *irq = &vdev->irqs[index];
> +	struct eventfd_ctx *trigger;
> +	int ret;
> +
> +	if (irq->trigger) {
> +		free_irq(irq->hwirq, irq);
> +		kfree(irq->name);
> +		eventfd_ctx_put(irq->trigger);
> +		irq->trigger = NULL;
> +	}
> +
> +	if (fd < 0) /* Disable only */
> +		return 0;
> +
> +	irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
> +						irq->hwirq, vdev->name);
> +	if (!irq->name)
> +		return -ENOMEM;
> +
> +	trigger = eventfd_ctx_fdget(fd);
> +	if (IS_ERR(trigger)) {
> +		kfree(irq->name);
> +		return PTR_ERR(trigger);
> +	}
> +
> +	irq->trigger = trigger;
> +
> +	ret = request_irq(irq->hwirq, vfio_irq_handler, 0, irq->name, irq);
> +	if (ret) {
> +		kfree(irq->name);
> +		eventfd_ctx_put(trigger);
> +		irq->trigger = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>  				     unsigned index, unsigned start,
>  				     unsigned count, uint32_t flags, void *data)
>  {
> -	return -EINVAL;
> +	struct vfio_platform_irq *irq = &vdev->irqs[index];
> +	uint8_t irq_bitmap;

Minor nit here and also present in the next patch, I believe when we
have DATA_BOOL, it's defined to be a u8 array, where each byte is a
bool, not each bit.  So it's not actually a bitmap.  The PCI code
handles any non-zero byte as true, not just bit0.  Thanks,

Alex

> +	int32_t fd;
> +
> +	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> +	case VFIO_IRQ_SET_DATA_NONE:
> +		if (count == 0)
> +			return vfio_set_trigger(vdev, index, -1);
> +
> +		vfio_irq_handler(irq->hwirq, irq);
> +		return 0;
> +
> +	case VFIO_IRQ_SET_DATA_BOOL:
> +		if (copy_from_user(&irq_bitmap, data, sizeof(uint8_t)))
> +			return -EFAULT;
> +
> +		if (irq_bitmap == 0x1) {
> +			vfio_irq_handler(irq->hwirq, irq);
> +			return 0;
> +		}
> +
> +		return -EINVAL;
> +
> +	case VFIO_IRQ_SET_DATA_EVENTFD:
> +		if (copy_from_user(&fd, data, sizeof(int32_t)))
> +			return -EFAULT;
> +
> +		return vfio_set_trigger(vdev, index, fd);
> +	}
> +
> +	return -EFAULT;
>  }
>  
>  int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
> @@ -95,7 +175,7 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
>  		if (hwirq < 0)
>  			goto err;
>  
> -		vdev->irqs[i].flags = 0;
> +		vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
>  		vdev->irqs[i].count = 1;
>  		vdev->irqs[i].hwirq = hwirq;
>  	}
> @@ -110,6 +190,11 @@ err:
>  
>  void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
>  {
> +	int i;
> +
> +	for (i = 0; i < vdev->num_irqs; i++)
> +		vfio_set_trigger(vdev, i, -1);
> +
>  	vdev->num_irqs = 0;
>  	kfree(vdev->irqs);
>  }
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 4201b94..765b371 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -28,6 +28,8 @@ struct vfio_platform_irq {
>  	u32			flags;
>  	u32			count;
>  	int			hwirq;
> +	char			*name;
> +	struct eventfd_ctx	*trigger;
>  };
>  
>  struct vfio_platform_region {

Re: [PATCHv7 20/26] vfio/platform: trigger an interrupt via eventfd

[Antonios Motakis]

On Wed, Sep 24, 2014 at 7:00 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 2014-09-23 at 16:46 +0200, Antonios Motakis wrote:
>> This patch allows to set an eventfd for a patform device's interrupt,
>> and also to trigger the interrupt eventfd from userspace for testing.
>>
>> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
>> ---
>>  drivers/vfio/platform/vfio_platform_irq.c     | 89 ++++++++++++++++++++++++++-
>>  drivers/vfio/platform/vfio_platform_private.h |  2 +
>>  2 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
>> index 007b386..25a7825 100644
>> --- a/drivers/vfio/platform/vfio_platform_irq.c
>> +++ b/drivers/vfio/platform/vfio_platform_irq.c
>> @@ -45,11 +45,91 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
>>       return -EINVAL;
>>  }
>>
>> +static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
>> +{
>> +     struct vfio_platform_irq *irq_ctx = dev_id;
>> +
>> +     eventfd_signal(irq_ctx->trigger, 1);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int vfio_set_trigger(struct vfio_platform_device *vdev,
>> +                         int index, int fd)
>> +{
>> +     struct vfio_platform_irq *irq = &vdev->irqs[index];
>> +     struct eventfd_ctx *trigger;
>> +     int ret;
>> +
>> +     if (irq->trigger) {
>> +             free_irq(irq->hwirq, irq);
>> +             kfree(irq->name);
>> +             eventfd_ctx_put(irq->trigger);
>> +             irq->trigger = NULL;
>> +     }
>> +
>> +     if (fd < 0) /* Disable only */
>> +             return 0;
>> +
>> +     irq->name = kasprintf(GFP_KERNEL, "vfio-irq[%d](%s)",
>> +                                             irq->hwirq, vdev->name);
>> +     if (!irq->name)
>> +             return -ENOMEM;
>> +
>> +     trigger = eventfd_ctx_fdget(fd);
>> +     if (IS_ERR(trigger)) {
>> +             kfree(irq->name);
>> +             return PTR_ERR(trigger);
>> +     }
>> +
>> +     irq->trigger = trigger;
>> +
>> +     ret = request_irq(irq->hwirq, vfio_irq_handler, 0, irq->name, irq);
>> +     if (ret) {
>> +             kfree(irq->name);
>> +             eventfd_ctx_put(trigger);
>> +             irq->trigger = NULL;
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  static int vfio_platform_set_irq_trigger(struct vfio_platform_device *vdev,
>>                                    unsigned index, unsigned start,
>>                                    unsigned count, uint32_t flags, void *data)
>>  {
>> -     return -EINVAL;
>> +     struct vfio_platform_irq *irq = &vdev->irqs[index];
>> +     uint8_t irq_bitmap;
>
> Minor nit here and also present in the next patch, I believe when we
> have DATA_BOOL, it's defined to be a u8 array, where each byte is a
> bool, not each bit.  So it's not actually a bitmap.  The PCI code
> handles any non-zero byte as true, not just bit0.  Thanks,
>

Ok, I will correct this. Thanks for the clarification.

> Alex
>
>> +     int32_t fd;
>> +
>> +     switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
>> +     case VFIO_IRQ_SET_DATA_NONE:
>> +             if (count == 0)
>> +                     return vfio_set_trigger(vdev, index, -1);
>> +
>> +             vfio_irq_handler(irq->hwirq, irq);
>> +             return 0;
>> +
>> +     case VFIO_IRQ_SET_DATA_BOOL:
>> +             if (copy_from_user(&irq_bitmap, data, sizeof(uint8_t)))
>> +                     return -EFAULT;
>> +
>> +             if (irq_bitmap == 0x1) {
>> +                     vfio_irq_handler(irq->hwirq, irq);
>> +                     return 0;
>> +             }
>> +
>> +             return -EINVAL;
>> +
>> +     case VFIO_IRQ_SET_DATA_EVENTFD:
>> +             if (copy_from_user(&fd, data, sizeof(int32_t)))
>> +                     return -EFAULT;
>> +
>> +             return vfio_set_trigger(vdev, index, fd);
>> +     }
>> +
>> +     return -EFAULT;
>>  }
>>
>>  int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
>> @@ -95,7 +175,7 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
>>               if (hwirq < 0)
>>                       goto err;
>>
>> -             vdev->irqs[i].flags = 0;
>> +             vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
>>               vdev->irqs[i].count = 1;
>>               vdev->irqs[i].hwirq = hwirq;
>>       }
>> @@ -110,6 +190,11 @@ err:
>>
>>  void vfio_platform_irq_cleanup(struct vfio_platform_device *vdev)
>>  {
>> +     int i;
>> +
>> +     for (i = 0; i < vdev->num_irqs; i++)
>> +             vfio_set_trigger(vdev, i, -1);
>> +
>>       vdev->num_irqs = 0;
>>       kfree(vdev->irqs);
>>  }
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 4201b94..765b371 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -28,6 +28,8 @@ struct vfio_platform_irq {
>>       u32                     flags;
>>       u32                     count;
>>       int                     hwirq;
>> +     char                    *name;
>> +     struct eventfd_ctx      *trigger;
>>  };
>>
>>  struct vfio_platform_region {
>
>
>



-- 
Antonios Motakis
Virtual Open Systems

[PATCHv7 21/26] vfio/platform: support for maskable and automasked interrupts

[Antonios Motakis]

Adds support to mask interrupts, and also for automasked interrupts.
Level sensitive interrupts are exposed as automasked interrupts and
are masked and disabled automatically when they fire.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_irq.c     | 120 ++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |   2 +
 2 files changed, 117 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 25a7825..90fa25a 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -31,27 +31,129 @@
 
 #include "vfio_platform_private.h"
 
+static void vfio_platform_mask(struct vfio_platform_irq *irq_ctx)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&irq_ctx->lock, flags);
+
+	if (!irq_ctx->masked) {
+		disable_irq(irq_ctx->hwirq);
+		irq_ctx->masked = true;
+	}
+
+	spin_unlock_irqrestore(&irq_ctx->lock, flags);
+}
+
 static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
 				    unsigned index, unsigned start,
 				    unsigned count, uint32_t flags, void *data)
 {
-	return -EINVAL;
+	uint8_t irq_bitmap;
+
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&irq_bitmap, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (irq_bitmap != 0x1)
+			return -EINVAL;
+
+		/*
+		 * The following fall through is both intentional and safe.
+		 * VFIO_IRQ_SET_DATA_BOOL allows to handle an array of IRQs
+		 * on the same index. For VFIO platform devices we always have
+		 * one IRQ per index, so as soon as we check that the user
+		 * provided bitmap only refers to one single IRQ, we can safely
+		 * share the rest of the logic with VFIO_IRQ_SET_DATA_NONE.
+		 */
+
+	case VFIO_IRQ_SET_DATA_NONE:
+		vfio_platform_mask(&vdev->irqs[index]);
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static void vfio_platform_unmask(struct vfio_platform_irq *irq_ctx)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&irq_ctx->lock, flags);
+
+	if (irq_ctx->masked) {
+		enable_irq(irq_ctx->hwirq);
+		irq_ctx->masked = false;
+	}
+
+	spin_unlock_irqrestore(&irq_ctx->lock, flags);
 }
 
 static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
 				    unsigned index, unsigned start,
 				    unsigned count, uint32_t flags, void *data)
 {
-	return -EINVAL;
+	uint8_t irq_bitmap;
+
+	if (start != 0 || count != 1)
+		return -EINVAL;
+
+	switch (flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_BOOL:
+		if (copy_from_user(&irq_bitmap, data, sizeof(uint8_t)))
+			return -EFAULT;
+
+		if (irq_bitmap != 0x1)
+			return -EINVAL;
+
+		/*
+		 * The following fall through is both intentional and safe,
+		 * as in vfio_platform_set_irq_mask().
+		 */
+
+	case VFIO_IRQ_SET_DATA_NONE:
+		vfio_platform_unmask(&vdev->irqs[index]);
+		return 0;
+
+	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
 }
 
 static irqreturn_t vfio_irq_handler(int irq, void *dev_id)
 {
 	struct vfio_platform_irq *irq_ctx = dev_id;
+	unsigned long flags;
+	int ret = IRQ_NONE;
 
-	eventfd_signal(irq_ctx->trigger, 1);
+	spin_lock_irqsave(&irq_ctx->lock, flags);
 
-	return IRQ_HANDLED;
+	if (!irq_ctx->masked) {
+		ret = IRQ_HANDLED;
+
+		if (irq_ctx->flags & VFIO_IRQ_INFO_AUTOMASKED) {
+			disable_irq_nosync(irq_ctx->hwirq);
+			irq_ctx->masked = true;
+		}
+	}
+
+	spin_unlock_irqrestore(&irq_ctx->lock, flags);
+
+	if (ret == IRQ_HANDLED)
+		eventfd_signal(irq_ctx->trigger, 1);
+
+	return ret;
 }
 
 static int vfio_set_trigger(struct vfio_platform_device *vdev,
@@ -175,9 +277,17 @@ int vfio_platform_irq_init(struct vfio_platform_device *vdev)
 		if (hwirq < 0)
 			goto err;
 
-		vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD;
+		spin_lock_init(&vdev->irqs[i].lock);
+
+		vdev->irqs[i].flags = VFIO_IRQ_INFO_EVENTFD
+					| VFIO_IRQ_INFO_MASKABLE;
+
+		if (irq_get_trigger_type(hwirq) & IRQ_TYPE_LEVEL_MASK)
+			vdev->irqs[i].flags |= VFIO_IRQ_INFO_AUTOMASKED;
+
 		vdev->irqs[i].count = 1;
 		vdev->irqs[i].hwirq = hwirq;
+		vdev->irqs[i].masked = false;
 	}
 
 	vdev->num_irqs = cnt;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 765b371..500e299 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -30,6 +30,8 @@ struct vfio_platform_irq {
 	int			hwirq;
 	char			*name;
 	struct eventfd_ctx	*trigger;
+	bool			masked;
+	spinlock_t		lock;
 };
 
 struct vfio_platform_region {
-- 
1.8.3.2

[PATCHv7 22/26] vfio: move eventfd support code for VFIO_PCI to a separate file

[Antonios Motakis]

The virqfd functionality that is used by VFIO_PCI to implement interrupt
masking and unmasking via an eventfd, is generic enough and can be reused
by another driver. Move it to a separate file in order to allow the code
to be shared.

Also properly export virqfd_enable and virqfd_disable in the process.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/Makefile               |   4 +-
 drivers/vfio/pci/vfio_pci_intrs.c   | 213 -----------------------------------
 drivers/vfio/pci/vfio_pci_private.h |   3 -
 drivers/vfio/virqfd.c               | 214 ++++++++++++++++++++++++++++++++++++
 include/linux/vfio.h                |  28 +++++
 5 files changed, 245 insertions(+), 217 deletions(-)
 create mode 100644 drivers/vfio/virqfd.c

diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
index dadf0ca..d798b09 100644
--- a/drivers/vfio/Makefile
+++ b/drivers/vfio/Makefile
@@ -1,4 +1,6 @@
-obj-$(CONFIG_VFIO) += vfio.o
+vfio_core-y := vfio.o virqfd.o
+
+obj-$(CONFIG_VFIO) += vfio_core.o
 obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
 obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
 obj-$(CONFIG_VFIO_SPAPR_EEH) += vfio_spapr_eeh.o
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 9dd49c9..3f909bb 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -18,226 +18,13 @@
 #include <linux/eventfd.h>
 #include <linux/pci.h>
 #include <linux/file.h>
-#include <linux/poll.h>
 #include <linux/vfio.h>
 #include <linux/wait.h>
-#include <linux/workqueue.h>
 #include <linux/slab.h>
 
 #include "vfio_pci_private.h"
 
 /*
- * IRQfd - generic
- */
-struct virqfd {
-	struct vfio_pci_device	*vdev;
-	struct eventfd_ctx	*eventfd;
-	int			(*handler)(struct vfio_pci_device *, void *);
-	void			(*thread)(struct vfio_pci_device *, void *);
-	void			*data;
-	struct work_struct	inject;
-	wait_queue_t		wait;
-	poll_table		pt;
-	struct work_struct	shutdown;
-	struct virqfd		**pvirqfd;
-};
-
-static struct workqueue_struct *vfio_irqfd_cleanup_wq;
-
-int __init vfio_pci_virqfd_init(void)
-{
-	vfio_irqfd_cleanup_wq =
-		create_singlethread_workqueue("vfio-irqfd-cleanup");
-	if (!vfio_irqfd_cleanup_wq)
-		return -ENOMEM;
-
-	return 0;
-}
-
-void vfio_pci_virqfd_exit(void)
-{
-	destroy_workqueue(vfio_irqfd_cleanup_wq);
-}
-
-static void virqfd_deactivate(struct virqfd *virqfd)
-{
-	queue_work(vfio_irqfd_cleanup_wq, &virqfd->shutdown);
-}
-
-static int virqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
-{
-	struct virqfd *virqfd = container_of(wait, struct virqfd, wait);
-	unsigned long flags = (unsigned long)key;
-
-	if (flags & POLLIN) {
-		/* An event has been signaled, call function */
-		if ((!virqfd->handler ||
-		     virqfd->handler(virqfd->vdev, virqfd->data)) &&
-		    virqfd->thread)
-			schedule_work(&virqfd->inject);
-	}
-
-	if (flags & POLLHUP) {
-		unsigned long flags;
-		spin_lock_irqsave(&virqfd->vdev->irqlock, flags);
-
-		/*
-		 * The eventfd is closing, if the virqfd has not yet been
-		 * queued for release, as determined by testing whether the
-		 * vdev pointer to it is still valid, queue it now.  As
-		 * with kvm irqfds, we know we won't race against the virqfd
-		 * going away because we hold wqh->lock to get here.
-		 */
-		if (*(virqfd->pvirqfd) == virqfd) {
-			*(virqfd->pvirqfd) = NULL;
-			virqfd_deactivate(virqfd);
-		}
-
-		spin_unlock_irqrestore(&virqfd->vdev->irqlock, flags);
-	}
-
-	return 0;
-}
-
-static void virqfd_ptable_queue_proc(struct file *file,
-				     wait_queue_head_t *wqh, poll_table *pt)
-{
-	struct virqfd *virqfd = container_of(pt, struct virqfd, pt);
-	add_wait_queue(wqh, &virqfd->wait);
-}
-
-static void virqfd_shutdown(struct work_struct *work)
-{
-	struct virqfd *virqfd = container_of(work, struct virqfd, shutdown);
-	u64 cnt;
-
-	eventfd_ctx_remove_wait_queue(virqfd->eventfd, &virqfd->wait, &cnt);
-	flush_work(&virqfd->inject);
-	eventfd_ctx_put(virqfd->eventfd);
-
-	kfree(virqfd);
-}
-
-static void virqfd_inject(struct work_struct *work)
-{
-	struct virqfd *virqfd = container_of(work, struct virqfd, inject);
-	if (virqfd->thread)
-		virqfd->thread(virqfd->vdev, virqfd->data);
-}
-
-static int virqfd_enable(struct vfio_pci_device *vdev,
-			 int (*handler)(struct vfio_pci_device *, void *),
-			 void (*thread)(struct vfio_pci_device *, void *),
-			 void *data, struct virqfd **pvirqfd, int fd)
-{
-	struct fd irqfd;
-	struct eventfd_ctx *ctx;
-	struct virqfd *virqfd;
-	int ret = 0;
-	unsigned int events;
-
-	virqfd = kzalloc(sizeof(*virqfd), GFP_KERNEL);
-	if (!virqfd)
-		return -ENOMEM;
-
-	virqfd->pvirqfd = pvirqfd;
-	virqfd->vdev = vdev;
-	virqfd->handler = handler;
-	virqfd->thread = thread;
-	virqfd->data = data;
-
-	INIT_WORK(&virqfd->shutdown, virqfd_shutdown);
-	INIT_WORK(&virqfd->inject, virqfd_inject);
-
-	irqfd = fdget(fd);
-	if (!irqfd.file) {
-		ret = -EBADF;
-		goto err_fd;
-	}
-
-	ctx = eventfd_ctx_fileget(irqfd.file);
-	if (IS_ERR(ctx)) {
-		ret = PTR_ERR(ctx);
-		goto err_ctx;
-	}
-
-	virqfd->eventfd = ctx;
-
-	/*
-	 * virqfds can be released by closing the eventfd or directly
-	 * through ioctl.  These are both done through a workqueue, so
-	 * we update the pointer to the virqfd under lock to avoid
-	 * pushing multiple jobs to release the same virqfd.
-	 */
-	spin_lock_irq(&vdev->irqlock);
-
-	if (*pvirqfd) {
-		spin_unlock_irq(&vdev->irqlock);
-		ret = -EBUSY;
-		goto err_busy;
-	}
-	*pvirqfd = virqfd;
-
-	spin_unlock_irq(&vdev->irqlock);
-
-	/*
-	 * Install our own custom wake-up handling so we are notified via
-	 * a callback whenever someone signals the underlying eventfd.
-	 */
-	init_waitqueue_func_entry(&virqfd->wait, virqfd_wakeup);
-	init_poll_funcptr(&virqfd->pt, virqfd_ptable_queue_proc);
-
-	events = irqfd.file->f_op->poll(irqfd.file, &virqfd->pt);
-
-	/*
-	 * Check if there was an event already pending on the eventfd
-	 * before we registered and trigger it as if we didn't miss it.
-	 */
-	if (events & POLLIN) {
-		if ((!handler || handler(vdev, data)) && thread)
-			schedule_work(&virqfd->inject);
-	}
-
-	/*
-	 * Do not drop the file until the irqfd is fully initialized,
-	 * otherwise we might race against the POLLHUP.
-	 */
-	fdput(irqfd);
-
-	return 0;
-err_busy:
-	eventfd_ctx_put(ctx);
-err_ctx:
-	fdput(irqfd);
-err_fd:
-	kfree(virqfd);
-
-	return ret;
-}
-
-static void virqfd_disable(struct vfio_pci_device *vdev,
-			   struct virqfd **pvirqfd)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&vdev->irqlock, flags);
-
-	if (*pvirqfd) {
-		virqfd_deactivate(*pvirqfd);
-		*pvirqfd = NULL;
-	}
-
-	spin_unlock_irqrestore(&vdev->irqlock, flags);
-
-	/*
-	 * Block until we know all outstanding shutdown jobs have completed.
-	 * Even if we don't queue the job, flush the wq to be sure it's
-	 * been released.
-	 */
-	flush_workqueue(vfio_irqfd_cleanup_wq);
-}
-
-/*
  * INTx
  */
 static void vfio_send_intx_eventfd(struct vfio_pci_device *vdev, void *unused)
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index 671c17a..4c89e0e 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -86,9 +86,6 @@ extern ssize_t vfio_pci_vga_rw(struct vfio_pci_device *vdev, char __user *buf,
 extern int vfio_pci_init_perm_bits(void);
 extern void vfio_pci_uninit_perm_bits(void);
 
-extern int vfio_pci_virqfd_init(void);
-extern void vfio_pci_virqfd_exit(void);
-
 extern int vfio_config_init(struct vfio_pci_device *vdev);
 extern void vfio_config_free(struct vfio_pci_device *vdev);
 #endif /* VFIO_PCI_PRIVATE_H */
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
new file mode 100644
index 0000000..243eb61
--- /dev/null
+++ b/drivers/vfio/virqfd.c
@@ -0,0 +1,214 @@
+/*
+ * VFIO generic eventfd code for IRQFD support.
+ * Derived from drivers/vfio/pci/vfio_pci_intrs.c
+ *
+ * Copyright (C) 2012 Red Hat, Inc.  All rights reserved.
+ *     Author: Alex Williamson <alex.williamson@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/vfio.h>
+#include <linux/eventfd.h>
+#include <linux/file.h>
+#include <linux/slab.h>
+#include "pci/vfio_pci_private.h"
+
+static struct workqueue_struct *vfio_irqfd_cleanup_wq;
+
+int __init vfio_pci_virqfd_init(void)
+{
+	vfio_irqfd_cleanup_wq =
+		create_singlethread_workqueue("vfio-irqfd-cleanup");
+	if (!vfio_irqfd_cleanup_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void vfio_pci_virqfd_exit(void)
+{
+	destroy_workqueue(vfio_irqfd_cleanup_wq);
+}
+
+static void virqfd_deactivate(struct virqfd *virqfd)
+{
+	queue_work(vfio_irqfd_cleanup_wq, &virqfd->shutdown);
+}
+
+static int virqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct virqfd *virqfd = container_of(wait, struct virqfd, wait);
+	unsigned long flags = (unsigned long)key;
+
+	if (flags & POLLIN) {
+		/* An event has been signaled, call function */
+		if ((!virqfd->handler ||
+		     virqfd->handler(virqfd->vdev, virqfd->data)) &&
+		    virqfd->thread)
+			schedule_work(&virqfd->inject);
+	}
+
+	if (flags & POLLHUP) {
+		unsigned long flags;
+		spin_lock_irqsave(&virqfd->vdev->irqlock, flags);
+
+		/*
+		 * The eventfd is closing, if the virqfd has not yet been
+		 * queued for release, as determined by testing whether the
+		 * vdev pointer to it is still valid, queue it now.  As
+		 * with kvm irqfds, we know we won't race against the virqfd
+		 * going away because we hold wqh->lock to get here.
+		 */
+		if (*(virqfd->pvirqfd) == virqfd) {
+			*(virqfd->pvirqfd) = NULL;
+			virqfd_deactivate(virqfd);
+		}
+
+		spin_unlock_irqrestore(&virqfd->vdev->irqlock, flags);
+	}
+
+	return 0;
+}
+
+static void virqfd_ptable_queue_proc(struct file *file,
+				     wait_queue_head_t *wqh, poll_table *pt)
+{
+	struct virqfd *virqfd = container_of(pt, struct virqfd, pt);
+	add_wait_queue(wqh, &virqfd->wait);
+}
+
+static void virqfd_shutdown(struct work_struct *work)
+{
+	struct virqfd *virqfd = container_of(work, struct virqfd, shutdown);
+	u64 cnt;
+
+	eventfd_ctx_remove_wait_queue(virqfd->eventfd, &virqfd->wait, &cnt);
+	flush_work(&virqfd->inject);
+	eventfd_ctx_put(virqfd->eventfd);
+
+	kfree(virqfd);
+}
+
+static void virqfd_inject(struct work_struct *work)
+{
+	struct virqfd *virqfd = container_of(work, struct virqfd, inject);
+	if (virqfd->thread)
+		virqfd->thread(virqfd->vdev, virqfd->data);
+}
+
+int virqfd_enable(struct vfio_pci_device *vdev,
+			 int (*handler)(struct vfio_pci_device *, void *),
+			 void (*thread)(struct vfio_pci_device *, void *),
+			 void *data, struct virqfd **pvirqfd, int fd)
+{
+	struct fd irqfd;
+	struct eventfd_ctx *ctx;
+	struct virqfd *virqfd;
+	int ret = 0;
+	unsigned int events;
+
+	virqfd = kzalloc(sizeof(*virqfd), GFP_KERNEL);
+	if (!virqfd)
+		return -ENOMEM;
+
+	virqfd->pvirqfd = pvirqfd;
+	virqfd->vdev = vdev;
+	virqfd->handler = handler;
+	virqfd->thread = thread;
+	virqfd->data = data;
+
+	INIT_WORK(&virqfd->shutdown, virqfd_shutdown);
+	INIT_WORK(&virqfd->inject, virqfd_inject);
+
+	irqfd = fdget(fd);
+	if (!irqfd.file) {
+		ret = -EBADF;
+		goto err_fd;
+	}
+
+	ctx = eventfd_ctx_fileget(irqfd.file);
+	if (IS_ERR(ctx)) {
+		ret = PTR_ERR(ctx);
+		goto err_ctx;
+	}
+
+	virqfd->eventfd = ctx;
+
+	/*
+	 * virqfds can be released by closing the eventfd or directly
+	 * through ioctl.  These are both done through a workqueue, so
+	 * we update the pointer to the virqfd under lock to avoid
+	 * pushing multiple jobs to release the same virqfd.
+	 */
+	spin_lock_irq(&vdev->irqlock);
+
+	if (*pvirqfd) {
+		spin_unlock_irq(&vdev->irqlock);
+		ret = -EBUSY;
+		goto err_busy;
+	}
+	*pvirqfd = virqfd;
+
+	spin_unlock_irq(&vdev->irqlock);
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd.
+	 */
+	init_waitqueue_func_entry(&virqfd->wait, virqfd_wakeup);
+	init_poll_funcptr(&virqfd->pt, virqfd_ptable_queue_proc);
+
+	events = irqfd.file->f_op->poll(irqfd.file, &virqfd->pt);
+
+	/*
+	 * Check if there was an event already pending on the eventfd
+	 * before we registered and trigger it as if we didn't miss it.
+	 */
+	if (events & POLLIN) {
+		if ((!handler || handler(vdev, data)) && thread)
+			schedule_work(&virqfd->inject);
+	}
+
+	/*
+	 * Do not drop the file until the irqfd is fully initialized,
+	 * otherwise we might race against the POLLHUP.
+	 */
+	fdput(irqfd);
+
+	return 0;
+err_busy:
+	eventfd_ctx_put(ctx);
+err_ctx:
+	fdput(irqfd);
+err_fd:
+	kfree(virqfd);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(virqfd_enable);
+
+void virqfd_disable(struct vfio_pci_device *vdev,
+			   struct virqfd **pvirqfd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vdev->irqlock, flags);
+
+	if (*pvirqfd) {
+		virqfd_deactivate(*pvirqfd);
+		*pvirqfd = NULL;
+	}
+
+	spin_unlock_irqrestore(&vdev->irqlock, flags);
+
+	/*
+	 * Block until we know all outstanding shutdown jobs have completed.
+	 * Even if we don't queue the job, flush the wq to be sure it's
+	 * been released.
+	 */
+	flush_workqueue(vfio_irqfd_cleanup_wq);
+}
+EXPORT_SYMBOL_GPL(virqfd_disable);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index d320411..a077c48 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -14,6 +14,8 @@
 
 #include <linux/iommu.h>
 #include <linux/mm.h>
+#include <linux/workqueue.h>
+#include <linux/poll.h>
 #include <uapi/linux/vfio.h>
 
 /**
@@ -121,4 +123,30 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 	return -ENOTTY;
 }
 #endif /* CONFIG_EEH */
+
+/*
+ * IRQFD support
+ */
+struct virqfd {
+	struct vfio_pci_device	*vdev;
+	struct eventfd_ctx	*eventfd;
+	int			(*handler)(struct vfio_pci_device *, void *);
+	void			(*thread)(struct vfio_pci_device *, void *);
+	void			*data;
+	struct work_struct	inject;
+	wait_queue_t		wait;
+	poll_table		pt;
+	struct work_struct	shutdown;
+	struct virqfd		**pvirqfd;
+};
+
+extern int vfio_pci_virqfd_init(void);
+extern void vfio_pci_virqfd_exit(void);
+extern int virqfd_enable(struct vfio_pci_device *vdev,
+			 int (*handler)(struct vfio_pci_device *, void *),
+			 void (*thread)(struct vfio_pci_device *, void *),
+			 void *data, struct virqfd **pvirqfd, int fd);
+extern void virqfd_disable(struct vfio_pci_device *vdev,
+			   struct virqfd **pvirqfd);
+
 #endif /* VFIO_H */
-- 
1.8.3.2

[PATCHv7 23/26] vfio: add local lock in virqfd instead of depending on VFIO PCI

[Antonios Motakis]

Virqfd just needs to keep accesses to any struct *virqfd safe, but this
comes into play only when creating or destroying eventfds, so sharing
the same spinlock with the VFIO bus driver is not necessary.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 10 +++++-----
 drivers/vfio/virqfd.c             | 24 +++++++++++++-----------
 include/linux/vfio.h              |  3 +--
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 3f909bb..e56c814 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -226,8 +226,8 @@ static int vfio_intx_set_signal(struct vfio_pci_device *vdev, int fd)
 static void vfio_intx_disable(struct vfio_pci_device *vdev)
 {
 	vfio_intx_set_signal(vdev, -1);
-	virqfd_disable(vdev, &vdev->ctx[0].unmask);
-	virqfd_disable(vdev, &vdev->ctx[0].mask);
+	virqfd_disable(&vdev->ctx[0].unmask);
+	virqfd_disable(&vdev->ctx[0].mask);
 	vdev->irq_type = VFIO_PCI_NUM_IRQS;
 	vdev->num_ctx = 0;
 	kfree(vdev->ctx);
@@ -377,8 +377,8 @@ static void vfio_msi_disable(struct vfio_pci_device *vdev, bool msix)
 	vfio_msi_set_block(vdev, 0, vdev->num_ctx, NULL, msix);
 
 	for (i = 0; i < vdev->num_ctx; i++) {
-		virqfd_disable(vdev, &vdev->ctx[i].unmask);
-		virqfd_disable(vdev, &vdev->ctx[i].mask);
+		virqfd_disable(&vdev->ctx[i].unmask);
+		virqfd_disable(&vdev->ctx[i].mask);
 	}
 
 	if (msix) {
@@ -415,7 +415,7 @@ static int vfio_pci_set_intx_unmask(struct vfio_pci_device *vdev,
 					     vfio_send_intx_eventfd, NULL,
 					     &vdev->ctx[0].unmask, fd);
 
-		virqfd_disable(vdev, &vdev->ctx[0].unmask);
+		virqfd_disable(&vdev->ctx[0].unmask);
 	}
 
 	return 0;
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 243eb61..27fa2f0 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -17,6 +17,7 @@
 #include "pci/vfio_pci_private.h"
 
 static struct workqueue_struct *vfio_irqfd_cleanup_wq;
+static spinlock_t lock;
 
 int __init vfio_pci_virqfd_init(void)
 {
@@ -25,6 +26,8 @@ int __init vfio_pci_virqfd_init(void)
 	if (!vfio_irqfd_cleanup_wq)
 		return -ENOMEM;
 
+	spin_lock_init(&lock);
+
 	return 0;
 }
 
@@ -53,21 +56,21 @@ static int virqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 
 	if (flags & POLLHUP) {
 		unsigned long flags;
-		spin_lock_irqsave(&virqfd->vdev->irqlock, flags);
+		spin_lock_irqsave(&lock, flags);
 
 		/*
 		 * The eventfd is closing, if the virqfd has not yet been
 		 * queued for release, as determined by testing whether the
-		 * vdev pointer to it is still valid, queue it now.  As
+		 * virqfd pointer to it is still valid, queue it now.  As
 		 * with kvm irqfds, we know we won't race against the virqfd
-		 * going away because we hold wqh->lock to get here.
+		 * going away because we hold the lock to get here.
 		 */
 		if (*(virqfd->pvirqfd) == virqfd) {
 			*(virqfd->pvirqfd) = NULL;
 			virqfd_deactivate(virqfd);
 		}
 
-		spin_unlock_irqrestore(&virqfd->vdev->irqlock, flags);
+		spin_unlock_irqrestore(&lock, flags);
 	}
 
 	return 0;
@@ -143,16 +146,16 @@ int virqfd_enable(struct vfio_pci_device *vdev,
 	 * we update the pointer to the virqfd under lock to avoid
 	 * pushing multiple jobs to release the same virqfd.
 	 */
-	spin_lock_irq(&vdev->irqlock);
+	spin_lock_irq(&lock);
 
 	if (*pvirqfd) {
-		spin_unlock_irq(&vdev->irqlock);
+		spin_unlock_irq(&lock);
 		ret = -EBUSY;
 		goto err_busy;
 	}
 	*pvirqfd = virqfd;
 
-	spin_unlock_irq(&vdev->irqlock);
+	spin_unlock_irq(&lock);
 
 	/*
 	 * Install our own custom wake-up handling so we are notified via
@@ -190,19 +193,18 @@ err_fd:
 }
 EXPORT_SYMBOL_GPL(virqfd_enable);
 
-void virqfd_disable(struct vfio_pci_device *vdev,
-			   struct virqfd **pvirqfd)
+void virqfd_disable(struct virqfd **pvirqfd)
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&vdev->irqlock, flags);
+	spin_lock_irqsave(&lock, flags);
 
 	if (*pvirqfd) {
 		virqfd_deactivate(*pvirqfd);
 		*pvirqfd = NULL;
 	}
 
-	spin_unlock_irqrestore(&vdev->irqlock, flags);
+	spin_unlock_irqrestore(&lock, flags);
 
 	/*
 	 * Block until we know all outstanding shutdown jobs have completed.
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index a077c48..fb6037b 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -146,7 +146,6 @@ extern int virqfd_enable(struct vfio_pci_device *vdev,
 			 int (*handler)(struct vfio_pci_device *, void *),
 			 void (*thread)(struct vfio_pci_device *, void *),
 			 void *data, struct virqfd **pvirqfd, int fd);
-extern void virqfd_disable(struct vfio_pci_device *vdev,
-			   struct virqfd **pvirqfd);
+extern void virqfd_disable(struct virqfd **pvirqfd);
 
 #endif /* VFIO_H */
-- 
1.8.3.2

[PATCHv7 24/26] vfio: pass an opaque pointer on virqfd initialization

[Antonios Motakis]

VFIO_PCI passes the VFIO device structure *vdev via eventfd to the handler
that implements masking/unmasking of IRQs via an eventfd. We can replace
it in the virqfd infrastructure with an opaque type so we can make use
of the mechanism from other VFIO bus drivers.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 11 +++++++----
 drivers/vfio/virqfd.c             | 17 ++++++++---------
 include/linux/vfio.h              | 12 ++++++------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index e56c814..6ca22a8 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -27,8 +27,10 @@
 /*
  * INTx
  */
-static void vfio_send_intx_eventfd(struct vfio_pci_device *vdev, void *unused)
+static void vfio_send_intx_eventfd(void *opaque, void *unused)
 {
+	struct vfio_pci_device *vdev = opaque;
+
 	if (likely(is_intx(vdev) && !vdev->virq_disabled))
 		eventfd_signal(vdev->ctx[0].trigger, 1);
 }
@@ -71,9 +73,9 @@ void vfio_pci_intx_mask(struct vfio_pci_device *vdev)
  * a signal is necessary, which can then be handled via a work queue
  * or directly depending on the caller.
  */
-static int vfio_pci_intx_unmask_handler(struct vfio_pci_device *vdev,
-					void *unused)
+static int vfio_pci_intx_unmask_handler(void *opaque, void *unused)
 {
+	struct vfio_pci_device *vdev = opaque;
 	struct pci_dev *pdev = vdev->pdev;
 	unsigned long flags;
 	int ret = 0;
@@ -411,7 +413,8 @@ static int vfio_pci_set_intx_unmask(struct vfio_pci_device *vdev,
 	} else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
 		int32_t fd = *(int32_t *)data;
 		if (fd >= 0)
-			return virqfd_enable(vdev, vfio_pci_intx_unmask_handler,
+			return virqfd_enable((void *) vdev,
+					     vfio_pci_intx_unmask_handler,
 					     vfio_send_intx_eventfd, NULL,
 					     &vdev->ctx[0].unmask, fd);
 
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 27fa2f0..ac63ec0 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -14,7 +14,6 @@
 #include <linux/eventfd.h>
 #include <linux/file.h>
 #include <linux/slab.h>
-#include "pci/vfio_pci_private.h"
 
 static struct workqueue_struct *vfio_irqfd_cleanup_wq;
 static spinlock_t lock;
@@ -49,7 +48,7 @@ static int virqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 	if (flags & POLLIN) {
 		/* An event has been signaled, call function */
 		if ((!virqfd->handler ||
-		     virqfd->handler(virqfd->vdev, virqfd->data)) &&
+		     virqfd->handler(virqfd->opaque, virqfd->data)) &&
 		    virqfd->thread)
 			schedule_work(&virqfd->inject);
 	}
@@ -99,13 +98,13 @@ static void virqfd_inject(struct work_struct *work)
 {
 	struct virqfd *virqfd = container_of(work, struct virqfd, inject);
 	if (virqfd->thread)
-		virqfd->thread(virqfd->vdev, virqfd->data);
+		virqfd->thread(virqfd->opaque, virqfd->data);
 }
 
-int virqfd_enable(struct vfio_pci_device *vdev,
-			 int (*handler)(struct vfio_pci_device *, void *),
-			 void (*thread)(struct vfio_pci_device *, void *),
-			 void *data, struct virqfd **pvirqfd, int fd)
+int virqfd_enable(void *opaque,
+		  int (*handler)(void *, void *),
+		  void (*thread)(void *, void *),
+		  void *data, struct virqfd **pvirqfd, int fd)
 {
 	struct fd irqfd;
 	struct eventfd_ctx *ctx;
@@ -118,7 +117,7 @@ int virqfd_enable(struct vfio_pci_device *vdev,
 		return -ENOMEM;
 
 	virqfd->pvirqfd = pvirqfd;
-	virqfd->vdev = vdev;
+	virqfd->opaque = opaque;
 	virqfd->handler = handler;
 	virqfd->thread = thread;
 	virqfd->data = data;
@@ -171,7 +170,7 @@ int virqfd_enable(struct vfio_pci_device *vdev,
 	 * before we registered and trigger it as if we didn't miss it.
 	 */
 	if (events & POLLIN) {
-		if ((!handler || handler(vdev, data)) && thread)
+		if ((!handler || handler(opaque, data)) && thread)
 			schedule_work(&virqfd->inject);
 	}
 
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index fb6037b..ce23a42 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -128,10 +128,10 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
  * IRQFD support
  */
 struct virqfd {
-	struct vfio_pci_device	*vdev;
+	void			*opaque;
 	struct eventfd_ctx	*eventfd;
-	int			(*handler)(struct vfio_pci_device *, void *);
-	void			(*thread)(struct vfio_pci_device *, void *);
+	int			(*handler)(void *, void *);
+	void			(*thread)(void *, void *);
 	void			*data;
 	struct work_struct	inject;
 	wait_queue_t		wait;
@@ -142,9 +142,9 @@ struct virqfd {
 
 extern int vfio_pci_virqfd_init(void);
 extern void vfio_pci_virqfd_exit(void);
-extern int virqfd_enable(struct vfio_pci_device *vdev,
-			 int (*handler)(struct vfio_pci_device *, void *),
-			 void (*thread)(struct vfio_pci_device *, void *),
+extern int virqfd_enable(void *opaque,
+			 int (*handler)(void *, void *),
+			 void (*thread)(void *, void *),
 			 void *data, struct virqfd **pvirqfd, int fd);
 extern void virqfd_disable(struct virqfd **pvirqfd);
 
-- 
1.8.3.2

[PATCHv7 25/26] vfio: initialize the virqfd workqueue in VFIO generic code

[Antonios Motakis]

Now we have finally completely decoupled virqfd from VFIO_PCI. We can
initialize it from the VFIO generic code, in order to safely use it from
multiple independent VFIO bus drivers.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/pci/vfio_pci.c | 8 --------
 drivers/vfio/vfio.c         | 8 ++++++++
 drivers/vfio/virqfd.c       | 4 ++--
 include/linux/vfio.h        | 4 ++--
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index f782533..40e176d 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1034,7 +1034,6 @@ static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev)
 static void __exit vfio_pci_cleanup(void)
 {
 	pci_unregister_driver(&vfio_pci_driver);
-	vfio_pci_virqfd_exit();
 	vfio_pci_uninit_perm_bits();
 }
 
@@ -1047,11 +1046,6 @@ static int __init vfio_pci_init(void)
 	if (ret)
 		return ret;
 
-	/* Start the virqfd cleanup handler */
-	ret = vfio_pci_virqfd_init();
-	if (ret)
-		goto out_virqfd;
-
 	/* Register and scan for devices */
 	ret = pci_register_driver(&vfio_pci_driver);
 	if (ret)
@@ -1060,8 +1054,6 @@ static int __init vfio_pci_init(void)
 	return 0;
 
 out_driver:
-	vfio_pci_virqfd_exit();
-out_virqfd:
 	vfio_pci_uninit_perm_bits();
 	return ret;
 }
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index f018d8d..8e84471 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1464,6 +1464,11 @@ static int __init vfio_init(void)
 	if (ret)
 		goto err_cdev_add;
 
+	/* Start the virqfd cleanup handler used by some VFIO bus drivers */
+	ret = vfio_virqfd_init();
+	if (ret)
+		goto err_virqfd;
+
 	pr_info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
 
 	/*
@@ -1476,6 +1481,8 @@ static int __init vfio_init(void)
 
 	return 0;
 
+err_virqfd:
+	cdev_del(&vfio.group_cdev);
 err_cdev_add:
 	unregister_chrdev_region(vfio.group_devt, MINORMASK);
 err_alloc_chrdev:
@@ -1490,6 +1497,7 @@ static void __exit vfio_cleanup(void)
 {
 	WARN_ON(!list_empty(&vfio.group_list));
 
+	vfio_virqfd_exit();
 	idr_destroy(&vfio.group_idr);
 	cdev_del(&vfio.group_cdev);
 	unregister_chrdev_region(vfio.group_devt, MINORMASK);
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index ac63ec0..c28882f 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -18,7 +18,7 @@
 static struct workqueue_struct *vfio_irqfd_cleanup_wq;
 static spinlock_t lock;
 
-int __init vfio_pci_virqfd_init(void)
+int __init vfio_virqfd_init(void)
 {
 	vfio_irqfd_cleanup_wq =
 		create_singlethread_workqueue("vfio-irqfd-cleanup");
@@ -30,7 +30,7 @@ int __init vfio_pci_virqfd_init(void)
 	return 0;
 }
 
-void vfio_pci_virqfd_exit(void)
+void vfio_virqfd_exit(void)
 {
 	destroy_workqueue(vfio_irqfd_cleanup_wq);
 }
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index ce23a42..9fa02c8 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -140,8 +140,8 @@ struct virqfd {
 	struct virqfd		**pvirqfd;
 };
 
-extern int vfio_pci_virqfd_init(void);
-extern void vfio_pci_virqfd_exit(void);
+extern int vfio_virqfd_init(void);
+extern void vfio_virqfd_exit(void);
 extern int virqfd_enable(void *opaque,
 			 int (*handler)(void *, void *),
 			 void (*thread)(void *, void *),
-- 
1.8.3.2

[PATCHv7 26/26] vfio/platform: implement IRQ masking/unmasking via an eventfd

[Antonios Motakis]

With this patch the VFIO user will be able to set an eventfd that can be
used in order to mask and unmask IRQs of platform devices.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/vfio/platform/vfio_platform_irq.c     | 48 +++++++++++++++++++++++++--
 drivers/vfio/platform/vfio_platform_private.h |  2 ++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index 90fa25a..4ea3d5a 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -45,11 +45,21 @@ static void vfio_platform_mask(struct vfio_platform_irq *irq_ctx)
 	spin_unlock_irqrestore(&irq_ctx->lock, flags);
 }
 
+static int vfio_platform_mask_handler(void *opaque, void *unused)
+{
+	struct vfio_platform_irq *irq_ctx = opaque;
+
+	vfio_platform_mask(irq_ctx);
+
+	return 0;
+}
+
 static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
 				    unsigned index, unsigned start,
 				    unsigned count, uint32_t flags, void *data)
 {
 	uint8_t irq_bitmap;
+	int32_t fd;
 
 	if (start != 0 || count != 1)
 		return -EINVAL;
@@ -75,7 +85,19 @@ static int vfio_platform_set_irq_mask(struct vfio_platform_device *vdev,
 		vfio_platform_mask(&vdev->irqs[index]);
 		return 0;
 
-	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	case VFIO_IRQ_SET_DATA_EVENTFD:
+		if (copy_from_user(&fd, data, sizeof(int32_t)))
+			return -EFAULT;
+
+		if (fd >= 0)
+			return virqfd_enable((void *) &vdev->irqs[index],
+					     vfio_platform_mask_handler,
+					     NULL, NULL,
+					     &vdev->irqs[index].mask, fd);
+
+		virqfd_disable(&vdev->irqs[index].mask);
+		return 0;
+
 	default:
 		return -ENOTTY;
 	}
@@ -97,11 +119,21 @@ static void vfio_platform_unmask(struct vfio_platform_irq *irq_ctx)
 	spin_unlock_irqrestore(&irq_ctx->lock, flags);
 }
 
+static int vfio_platform_unmask_handler(void *opaque, void *unused)
+{
+	struct vfio_platform_irq *irq_ctx = opaque;
+
+	vfio_platform_unmask(irq_ctx);
+
+	return 0;
+}
+
 static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
 				    unsigned index, unsigned start,
 				    unsigned count, uint32_t flags, void *data)
 {
 	uint8_t irq_bitmap;
+	int32_t fd;
 
 	if (start != 0 || count != 1)
 		return -EINVAL;
@@ -123,7 +155,19 @@ static int vfio_platform_set_irq_unmask(struct vfio_platform_device *vdev,
 		vfio_platform_unmask(&vdev->irqs[index]);
 		return 0;
 
-	case VFIO_IRQ_SET_DATA_EVENTFD: /* XXX not implemented yet */
+	case VFIO_IRQ_SET_DATA_EVENTFD:
+		if (copy_from_user(&fd, data, sizeof(int32_t)))
+			return -EFAULT;
+
+		if (fd >= 0)
+			return virqfd_enable((void *) &vdev->irqs[index],
+					     vfio_platform_unmask_handler,
+					     NULL, NULL,
+					     &vdev->irqs[index].unmask, fd);
+
+		virqfd_disable(&vdev->irqs[index].unmask);
+		return 0;
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 500e299..dd1beda 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -32,6 +32,8 @@ struct vfio_platform_irq {
 	struct eventfd_ctx	*trigger;
 	bool			masked;
 	spinlock_t		lock;
+	struct virqfd		*unmask;
+	struct virqfd		*mask;
 };
 
 struct vfio_platform_region {
-- 
1.8.3.2