Re: [PATCH v2 03/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 v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
Date: Mon, 25 Apr 2016 03:50:32 -0600	[thread overview]
Message-ID: <571E048802000078000E5371@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1460988011-17758-4-git-send-email-quan.xu@intel.com>

>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info *page, unsigned long type,
>                             int preemptible)
>  {
>      unsigned long nx, x, y = page->u.inuse.type_info;
> -    int rc = 0;
> +    int rc = 0, ret = 0;
>  
>      ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>  
> @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type,
>          if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>          {
>              if ( (x & PGT_type_mask) == PGT_writable_page )
> -                iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> +                ret = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
>              else if ( type == PGT_writable_page )
> -                iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> -                               page_to_mfn(page),
> -                               IOMMUF_readable|IOMMUF_writable);
> +                ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> +                                     page_to_mfn(page),
> +                                     IOMMUF_readable|IOMMUF_writable);
>          }
>      }
>  
> @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info *page, unsigned long type,
>      if ( (x & PGT_partial) && !(nx & PGT_partial) )
>          put_page(page);
>  
> +    if ( unlikely(ret) )
> +        rc = ret;

Please don't overwrite the more relevant "rc" value in situations like
this in case that is already non-zero. In this specific case you can
actually get away without introducing a second error code variable,
since the only place rc gets altered is between the two hunks above,
and overwriting the rc value from map/unmap is then exactly what
we want (but I'd much appreciate if you added a comment to this
effect).

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
>      ept_entry_t *table, *ept_entry = NULL;
>      unsigned long gfn_remainder = gfn;
>      unsigned int i, target = order / EPT_TABLE_ORDER;
> -    int ret, rc = 0;
> +    int ret, err = 0, rc = 0;

Just like Kevin, and despite your reply to him I do not see the need
fir the 3rd error indicator variable here:

> @@ -830,10 +830,26 @@ out:
>          {
>              if ( iommu_flags )
>                  for ( i = 0; i < (1 << order); i++ )
> -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> +                {
> +                    ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, iommu_flags);
> +
> +                    if ( unlikely(ret) )
> +                    {
> +                        while (i)

while ( i-- )

> +                            iommu_unmap_page(d, gfn + --i);
> +
> +                        err = ret;
> +                        break;
> +                    }
> +                }
>              else
>                  for ( i = 0; i < (1 << order); i++ )
> -                    iommu_unmap_page(d, gfn + i);
> +                {
> +                    ret = iommu_unmap_page(d, gfn + i);
> +
> +                    if ( unlikely(ret) )
> +                        err = ret;
> +                }
>          }

You can simply utilize that rc is zero upon entering this if(), clearing
it back to zero here.

> @@ -849,6 +865,9 @@ out:
>      if ( rc == 0 && p2m_is_hostp2m(p2m) )
>          p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt, p2ma);
>  
> +    if ( unlikely(err) )
> +        rc = err;

Same comment as for mm.c (and also for mm/p2m-pt.c and
mm/p2m.c below, albeit the impact there is limited - the only
difference is whether to return first or last error encountered, but
I'd still prefer the first one to be used, as for a crashed domain later
errors may end up being kind of meaningless wrt what caused the
domain to get crashed).

> @@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>                   ((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 ( unlikely(ret) )
> +                rc = ret;
> +
>              if ( !(i++ & 0xfffff) )
>                  process_pending_softirqs();
>          }
> +
> +        if ( rc )
> +            printk(XENLOG_G_ERR
> +                   "dom%d: IOMMU mapping is failed for hardware domain.",
> +                   d->domain_id);

Leaving the admin / developer with no indication of at least one
specific case of a page where a failure occurred. As said in various
places before: Log messages should provide useful information for
at least getting started at investigating the issue, without first of
all having to further instrument the code.

In any event the "is" should be dropped from the text.

Jan

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