Re: [RFC PATCH 2/7] iommu: add iommu_lookup_page to lookup guest gfn for a particular IOMMU mapping

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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