Re: [Patch v6 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

From: "Jan Beulich" <JBeulich@suse.com>
To: Quan Xu <quan.xu@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	dario.faggioli@citrix.com, xen-devel@lists.xen.org,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [Patch v6 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
Date: Wed, 01 Jun 2016 04:05:10 -0600	[thread overview]
Message-ID: <574ECF7602000078000F0518@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1464703056-4741-3-git-send-email-quan.xu@intel.com>

>>> On 31.05.16 at 15:57, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -673,6 +673,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>      if ( iommu_enabled && need_iommu(p2m->domain) &&
>           (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
>      {
> +        int ret;
> +
>          if ( iommu_use_hap_pt(p2m->domain) )
>          {
>              if ( iommu_old_flags )
> @@ -680,11 +682,27 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>          }
>          else if ( iommu_pte_flags )
>              for ( i = 0; i < (1UL << page_order); i++ )
> -                iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> -                               iommu_pte_flags);
> +            {
> +                ret = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> +                                     iommu_pte_flags);
> +                if ( unlikely(ret) )
> +                {
> +                    while ( i-- )
> +                        iommu_unmap_page(p2m->domain, gfn + i);
> +
> +                    if ( !rc )
> +                        rc = ret;
> +
> +                    break;
> +                }
> +            }

So why do you not use the same code structure here as you do
on the EPT side? I.e. do away with using "ret" altogether, moving
it all into ...

>          else
>              for ( i = 0; i < (1UL << page_order); i++ )
> -                iommu_unmap_page(p2m->domain, gfn + i);
> +            {
> +                ret = iommu_unmap_page(p2m->domain, gfn + i);
> +                if ( !rc )
> +                    rc = ret;
> +            }

... this extremely narrow scope?

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -641,10 +641,21 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
>  
>      if ( !paging_mode_translate(p2m->domain) )
>      {
> +        int rc = 0;
> +
>          if ( need_iommu(p2m->domain) )
> +        {
>              for ( i = 0; i < (1 << page_order); i++ )
> -                iommu_unmap_page(p2m->domain, mfn + i);
> -        return 0;
> +            {
> +                int ret;
> +
> +                ret = iommu_unmap_page(p2m->domain, mfn + i);

In cases like this I'd prefer the assignment to be replaced by its
right side becoming the variable's initializer.

> @@ -2535,6 +2546,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>          if ( mfn_valid(mfn) )
>              p2m_remove_page(ap2m, gfn_x(old_gfn), mfn_x(mfn), PAGE_ORDER_4K);
>          rc = 0;
> +
>          goto out;

Stray addition of a blank line.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -171,20 +171,32 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>      {
>          struct page_info *page;
>          unsigned int i = 0;
> +        int rc = 0;
> +
>          page_list_for_each ( page, &d->page_list )
>          {
>              unsigned long mfn = page_to_mfn(page);
>              unsigned long gfn = mfn_to_gmfn(d, mfn);
>              unsigned int mapping = IOMMUF_readable;
> +            int ret;
>  
>              if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
>                   ((page->u.inuse.type_info & PGT_type_mask)
>                    == PGT_writable_page) )
>                  mapping |= IOMMUF_writable;
> -            hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +
> +            ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +            if ( !rc )
> +                rc = ret;
> +
>              if ( !(i++ & 0xfffff) )
>                  process_pending_softirqs();
>          }
> +
> +        if ( rc )
> +            printk(XENLOG_WARNING
> +                   "d%d: IOMMU mapping failed: %d for hardware domain\n",
> +                   d->domain_id, rc);

No need to mention "hardware domain" here, as that's going to be clear
both from the domain ID and the point in time when this message would
appear.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel