| Message ID | 1455099035-17649-3-git-send-email-malcolm.crossley@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 10, 2016 at 10:10:30AM +0000, Malcolm Crossley wrote: > If IOMMU driver does not implement lookup_page function then it returns -ENOMEM. That is a very odd return code. Could you explain why -ENOMEM? > > Returns 0 on success and any other value on failure. > > Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> > -- > Cc: jbeulich@suse.com > Cc: xen-devel@lists.xen.org > --- > xen/drivers/passthrough/iommu.c | 21 +++++++++++++++++++++ > xen/include/xen/iommu.h | 2 ++ > 2 files changed, 23 insertions(+) > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 0b2abf4..06f21ee 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -257,6 +257,27 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn) > return hd->platform_ops->unmap_page(d, gfn); > } > > +int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn) > +{ > + struct hvm_iommu *hd = domain_hvm_iommu(d); > + > + /* > + * BFN maps 1:1 to GFN when iommu passthrough is enabled or > + * when IOMMU shared page tables is in use Missing full stop. > + */ > + if ( iommu_use_hap_pt(d) || (iommu_passthrough && is_hardware_domain(d)) ) The comment is not in sync with the code. But what I am curious - if the dom0 is PVH and the shared EPT is disabled, what path should we take? I would think we would need to skip this and go to the ->lookup_page? > + { > + *gfn = bfn; > + return 0; > + } > + > + if ( !iommu_enabled || !hd->platform_ops || > + !hd->platform_ops->lookup_page ) > + return -ENOMEM; -ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops is not set nor ->lookup_page? > + > + return hd->platform_ops->lookup_page(d, bfn, gfn); > +} > + > static void iommu_free_pagetables(unsigned long unused) > { > do { > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 8217cb7..49ca087 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -77,6 +77,7 @@ void iommu_teardown(struct domain *d); > int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, > unsigned int flags); > int iommu_unmap_page(struct domain *d, unsigned long gfn); > +int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn); > > enum iommu_feature > { > @@ -151,6 +152,7 @@ struct iommu_ops { > int (*map_page)(struct domain *d, unsigned long gfn, unsigned long mfn, > unsigned int flags); > int (*unmap_page)(struct domain *d, unsigned long gfn); > + int (*lookup_page)(struct domain *d, unsigned long bfn, unsigned long *gfn); > void (*free_page_table)(struct page_info *); > #ifdef CONFIG_X86 > void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value); > -- > 1.7.12.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Wed, Feb 17, 2016 at 8:22 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: >> + { >> + *gfn = bfn; >> + return 0; >> + } >> + >> + if ( !iommu_enabled || !hd->platform_ops || >> + !hd->platform_ops->lookup_page ) >> + return -ENOMEM; > > -ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops > is not set nor ->lookup_page? I think ENOTSUPP, perhaps? ENOMEM means a memory allocation failed (which it hasn't); ENOSYS means this is an invalid system call (which it isn't). ESRCH could be close too, but it seems like that would be more appropriate when you try to lookup a non-existent BFN. -George
On 24/02/16 15:08, George Dunlap wrote: > On Wed, Feb 17, 2016 at 8:22 PM, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: >>> + { >>> + *gfn = bfn; >>> + return 0; >>> + } >>> + >>> + if ( !iommu_enabled || !hd->platform_ops || >>> + !hd->platform_ops->lookup_page ) >>> + return -ENOMEM; >> -ENOMEM ? ENXIO ? Or maybe -ENOSYS for the case when hd->platform_ops >> is not set nor ->lookup_page? > I think ENOTSUPP, perhaps? ENOMEM means a memory allocation failed > (which it hasn't); ENOSYS means this is an invalid system call (which > it isn't). ESRCH could be close too, but it seems like that would be > more appropriate when you try to lookup a non-existent BFN. Xen (fairly) consistently uses ESRCH for "domain not found". I would recommend -ENOTSUPP here. ~Andrew
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 0b2abf4..06f21ee 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -257,6 +257,27 @@ int iommu_unmap_page(struct domain *d, unsigned long gfn) return hd->platform_ops->unmap_page(d, gfn); } +int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn) +{ + struct hvm_iommu *hd = domain_hvm_iommu(d); + + /* + * BFN maps 1:1 to GFN when iommu passthrough is enabled or + * when IOMMU shared page tables is in use + */ + if ( iommu_use_hap_pt(d) || (iommu_passthrough && is_hardware_domain(d)) ) + { + *gfn = bfn; + return 0; + } + + if ( !iommu_enabled || !hd->platform_ops || + !hd->platform_ops->lookup_page ) + return -ENOMEM; + + return hd->platform_ops->lookup_page(d, bfn, gfn); +} + static void iommu_free_pagetables(unsigned long unused) { do { diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 8217cb7..49ca087 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -77,6 +77,7 @@ void iommu_teardown(struct domain *d); int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); int iommu_unmap_page(struct domain *d, unsigned long gfn); +int iommu_lookup_page(struct domain *d, unsigned long bfn, unsigned long *gfn); enum iommu_feature { @@ -151,6 +152,7 @@ struct iommu_ops { int (*map_page)(struct domain *d, unsigned long gfn, unsigned long mfn, unsigned int flags); int (*unmap_page)(struct domain *d, unsigned long gfn); + int (*lookup_page)(struct domain *d, unsigned long bfn, unsigned long *gfn); void (*free_page_table)(struct page_info *); #ifdef CONFIG_X86 void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, unsigned int value);
If IOMMU driver does not implement lookup_page function then it returns -ENOMEM. Returns 0 on success and any other value on failure. Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com> -- Cc: jbeulich@suse.com Cc: xen-devel@lists.xen.org --- xen/drivers/passthrough/iommu.c | 21 +++++++++++++++++++++ xen/include/xen/iommu.h | 2 ++ 2 files changed, 23 insertions(+)