Forgot to respond to one of the comment ..
> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Monday, October 05, 2015 10:47 AM
> To: 'Alex Williamson' <alex.william...@redhat.com>
> Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> christoffer.d...@linaro.org; eric.au...@linaro.org; pranavku...@linaro.org;
> marc.zyng...@arm.com; will.dea...@arm.com
> Subject: RE: [RFC PATCH 2/6] iommu: Add interface to get msi-pages
> mapping attributes
>
>
>
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Saturday, October 03, 2015 4:16 AM
> > To: Bhushan Bharat-R65777 <bharat.bhus...@freescale.com>
> > Cc: kvm...@lists.cs.columbia.edu; kvm@vger.kernel.org;
> > christoffer.d...@linaro.org; eric.au...@linaro.org;
> > pranavku...@linaro.org; marc.zyng...@arm.com; will.dea...@arm.com
> > Subject: Re: [RFC PATCH 2/6] iommu: Add interface to get msi-pages
> > mapping attributes
> >
> > [really ought to consider cc'ing the iommu list]
> >
> > On Wed, 2015-09-30 at 20:26 +0530, Bharat Bhushan wrote:
> > > This APIs return the capability of automatically mapping msi-pages
> > > in iommu with some magic iova. Which is what currently most of
> > > iommu's does and is the default behaviour.
> > >
> > > Further API returns whether iommu allows the user to define
> > > different iova for mai-page mapping for the domain. This is required
> > > when a msi capable device is directly assigned to user-space/VM and
> > > user-space/VM need to define a non-overlapping (from other dma-able
> > > address space) iova for msi-pages mapping in iommu.
> > >
> > > This patch just define the interface and follow up patches will
> > > extend this interface.
> >
> > This is backwards, generally you want to add the infrastructure and
> > only expose it once all the pieces are in place for it to work. For
> > instance, patch
> > 1/6 exposes a new userspace interface for vfio that doesn't do anything
> yet.
> > How does the user know if it's there, *and* works?
>
> Ok, I will reorder the patches.
>
> >
> > > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > > ---
> > > drivers/iommu/arm-smmu.c | 3 +++
> > > drivers/iommu/fsl_pamu_domain.c | 3 +++
> > > drivers/iommu/iommu.c | 14 ++++++++++++++
> > > include/linux/iommu.h | 9 ++++++++-
> > > 4 files changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index
> > > 66a803b..a3956fb 100644
> > > --- a/drivers/iommu/arm-smmu.c
> > > +++ b/drivers/iommu/arm-smmu.c
> > > @@ -1406,6 +1406,9 @@ static int arm_smmu_domain_get_attr(struct
> > iommu_domain *domain,
> > > case DOMAIN_ATTR_NESTING:
> > > *(int *)data = (smmu_domain->stage ==
> > ARM_SMMU_DOMAIN_NESTED);
> > > return 0;
> > > + case DOMAIN_ATTR_MSI_MAPPING:
> > > + /* Dummy handling added */
> > > + return 0;
> > > default:
> > > return -ENODEV;
> > > }
> > > diff --git a/drivers/iommu/fsl_pamu_domain.c
> > > b/drivers/iommu/fsl_pamu_domain.c index 1d45293..9a94430 100644
> > > --- a/drivers/iommu/fsl_pamu_domain.c
> > > +++ b/drivers/iommu/fsl_pamu_domain.c
> > > @@ -856,6 +856,9 @@ static int fsl_pamu_get_domain_attr(struct
> > iommu_domain *domain,
> > > case DOMAIN_ATTR_FSL_PAMUV1:
> > > *(int *)data = DOMAIN_ATTR_FSL_PAMUV1;
> > > break;
> > > + case DOMAIN_ATTR_MSI_MAPPING:
> > > + /* Dummy handling added */
> > > + break;
> > > default:
> > > pr_debug("Unsupported attribute type\n");
> > > ret = -EINVAL;
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> > > d4f527e..16c2eab 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -1216,6 +1216,7 @@ int iommu_domain_get_attr(struct
> > iommu_domain *domain,
> > > bool *paging;
> > > int ret = 0;
> > > u32 *count;
> > > + struct iommu_domain_msi_maps *msi_maps;
> > >
> > > switch (attr) {
> > > case DOMAIN_ATTR_GEOMETRY:
> > > @@ -1236,6 +1237,19 @@ int iommu_domain_get_attr(struct
> > iommu_domain *domain,
> > > ret = -ENODEV;
> > >
> > > break;
> > > + case DOMAIN_ATTR_MSI_MAPPING:
> > > + msi_maps = data;
> > > +
> > > + /* Default MSI-pages are magically mapped with some iova
> > and
> > > + * do now allow to configure with different iova.
> > > + */
> > > + msi_maps->automap = true;
> > > + msi_maps->override_automap = false;
> >
> > There's no magic. I think what you're trying to express is the
> > difference between platforms that support MSI within the IOMMU IOVA
> > space and thus need explicit IOMMU mappings vs platforms where MSI
> > mappings either bypass the IOMMU entirely or are setup implicitly with
> > interrupt remapping support.
>
> Yes, I wants to differentiate the platforms which requires explicit iommu
> mapping for MSI with other platforms.
> I will change the comment and use better name
> (need_mapping/need_iommu_mapping/require_mapping).
>
> >
> > Why does it make sense to impose any sort of defaults? If the IOMMU
> > driver doesn't tell us what to do, I don't think we want to assume anything.
Agree, in this patch series I restricted the change to smmu only. I will try
extending this to other iommu's as well.
Thanks
-Bharat
> >
> > > +
> > > + if (domain->ops->domain_get_attr)
> > > + ret = domain->ops->domain_get_attr(domain, attr,
> > data);
> > > +
> > > + break;
> > > default:
> > > if (!domain->ops->domain_get_attr)
> > > return -EINVAL;
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > > 0546b87..6d49f3f 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -83,6 +83,13 @@ struct iommu_domain {
> > > struct iommu_domain_geometry geometry; };
> > >
> > > +struct iommu_domain_msi_maps {
> > > + dma_addr_t base_address;
> > > + dma_addr_t size;
> >
> > size_t?
>
> Will remove above two fields as they are redundant.
>
> Thanks
> -Bharat
>
> >
> > > + bool automap;
> > > + bool override_automap;
> > > +};
> > > +
> > > enum iommu_cap {
> > > IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache
> > coherent DMA
> > > transactions */
> > > @@ -111,6 +118,7 @@ enum iommu_attr {
> > > DOMAIN_ATTR_FSL_PAMU_ENABLE,
> > > DOMAIN_ATTR_FSL_PAMUV1,
> > > DOMAIN_ATTR_NESTING, /* two stages of translation */
> > > + DOMAIN_ATTR_MSI_MAPPING, /* Provides MSIs mapping in iommu
> > */
> > > DOMAIN_ATTR_MAX,
> > > };
> > >
> > > @@ -167,7 +175,6 @@ struct iommu_ops {
> > > int (*domain_set_windows)(struct iommu_domain *domain, u32
> > w_count);
> > > /* Get the numer of window per domain */
> > > u32 (*domain_get_windows)(struct iommu_domain *domain);
> > > -
> > > #ifdef CONFIG_OF_IOMMU
> > > int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
> > > #endif
> >
> >
