Re: [PATCH v4 2/3] vfio/iommu_type1: add new VFIO_TYPE1_NESTING_IOMMU IOMMU type

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

Hi Alex,

On Wed, Sep 24, 2014 at 07:23:19PM +0100, Alex Williamson wrote:
> On Wed, 2014-09-24 at 10:21 +0100, Will Deacon wrote:
> > VFIO allows devices to be safely handed off to userspace by putting
> > them behind an IOMMU configured to ensure DMA and interrupt isolation.
> > This enables userspace KVM clients, such as kvmtool and qemu, to further
> > map the device into a virtual machine.
> > 
> > With IOMMUs such as the ARM SMMU, it is then possible to provide SMMU
> > translation services to the guest operating system, which are nested
> > with the existing translation installed by VFIO. However, enabling this
> > feature means that the IOMMU driver must be informed that the VFIO domain
> > is being created for the purposes of nested translation.
> > 
> > This patch adds a new IOMMU type (VFIO_TYPE1_NESTING_IOMMU) to the VFIO
> > type-1 driver. The new IOMMU type acts identically to the
> > VFIO_TYPE1v2_IOMMU type, but additionally sets the DOMAIN_ATTR_NESTING
> > attribute on its IOMMU domains.

[...]

> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index 0734fbe5b651..d15b00700a31 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -57,7 +57,8 @@ struct vfio_iommu {
> >  	struct list_head	domain_list;
> >  	struct mutex		lock;
> >  	struct rb_root		dma_list;
> > -	bool v2;
> > +	bool			v2;
> > +	bool			nesting;
> >  };
> >  
> >  struct vfio_domain {
> > @@ -671,7 +672,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  	struct vfio_group *group, *g;
> >  	struct vfio_domain *domain, *d;
> >  	struct bus_type *bus = NULL;
> > -	int ret;
> > +	int ret, attr = 1;
> >  
> >  	mutex_lock(&iommu->lock);
> >  
> > @@ -705,6 +706,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  		goto out_free;
> >  	}
> >  
> > +	if (iommu->nesting) {
> 
> nit, attr declaration could be moved here

Sure.

> > +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
> > +					    &attr);
> > +		if (ret)
> > +			goto out_domain;
> > +	}
> > +
> >  	ret = iommu_attach_group(domain->domain, iommu_group);
> >  	if (ret)
> >  		goto out_domain;
> > @@ -819,17 +827,25 @@ static void *vfio_iommu_type1_open(unsigned long arg)
> >  {
> >  	struct vfio_iommu *iommu;
> >  
> > -	if (arg != VFIO_TYPE1_IOMMU && arg != VFIO_TYPE1v2_IOMMU)
> > -		return ERR_PTR(-EINVAL);
> > -
> >  	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> >  	if (!iommu)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > +	switch (arg) {
> > +	case VFIO_TYPE1_IOMMU:
> > +		break;
> > +	case VFIO_TYPE1_NESTING_IOMMU:
> > +		iommu->nesting = true;
> > +	case VFIO_TYPE1v2_IOMMU:
> > +		iommu->v2 = true;
> > +		break;
> > +	default:
> 
> Leaks iommu

Yikes, sorry about that. Will fix.

Will