> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx] > Sent: Saturday, October 03, 2015 4:16 AM > To: Bhushan Bharat-R65777 <Bharat.Bhushan@xxxxxxxxxxxxx> > Cc: kvmarm@xxxxxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; > christoffer.dall@xxxxxxxxxx; eric.auger@xxxxxxxxxx; pranavkumar@xxxxxxxxxx; > marc.zyngier@xxxxxxx; will.deacon@xxxxxxx > 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.Bhushan@xxxxxxxxxxxxx> > > --- > > 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. > > > + > > + 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 > > ��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
