xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Xu, Quan" <quan.xu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Tian, Kevin" <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" <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	"Nakajima, Jun" <jun.nakajima@intel.com>
Subject: Re: [Patch v6 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping
Date: Thu, 2 Jun 2016 06:00:12 +0000	[thread overview]
Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8C498A@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <574ECF7602000078000F0518@prv-mh.provo.novell.com>

On June 01, 2016 6:05 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> 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 try to modify these code in a slight way, but if you point out some extra issue, I am pleased to fix it.
Furthermore, I was not sure whether I made an arbitrary decision here to add the condition 'rc == 0' or not,
Even I was aware of that the 'rc' is zero when the code run here..

> 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?
> 

What about this fix:

--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -670,7 +670,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
          && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
         p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;

-    if ( iommu_enabled && need_iommu(p2m->domain) &&
+    if ( rc == 0 && iommu_enabled && need_iommu(p2m->domain) &&
          (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
     {
         if ( iommu_use_hap_pt(p2m->domain) )
@@ -678,13 +678,33 @@ p2m_pt_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
             if ( iommu_old_flags )
                 amd_iommu_flush_pages(p2m->domain, gfn, page_order);
         }
-        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);
         else
-            for ( i = 0; i < (1UL << page_order); i++ )
-                iommu_unmap_page(p2m->domain, gfn + i);
+        {
+            if ( iommu_pte_flags )
+                for ( i = 0; i < (1UL << page_order); i++ )
+                {
+                    rc = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
+                                        iommu_pte_flags);
+                    if ( unlikely(rc) )
+                    {
+                        while ( i-- )
+                            iommu_unmap_page(p2m->domain, gfn + i);
+
+                        break;
+                    }
+                }
+            else
+            {
+                int ret;
+
+                for ( i = 0; i < (1UL << page_order); i++ )
+                {
+                    ret = iommu_unmap_page(p2m->domain, gfn + i);
+                    if ( !rc )
+                        rc = ret;
+                }
+            }
+        }
     }

Quan

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

  reply	other threads:[~2016-06-02  6:00 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-31 13:57 [Patch v6 00/11] Check VT-d Device-TLB flush error Xu, Quan
2016-05-31 13:57 ` [Patch v6 01/11] IOMMU: handle IOMMU mapping and unmapping failures Xu, Quan
2016-06-01  9:52   ` Jan Beulich
2016-05-31 13:57 ` [Patch v6 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Xu, Quan
2016-06-01 10:05   ` Jan Beulich
2016-06-02  6:00     ` Xu, Quan [this message]
2016-06-02  9:13       ` Jan Beulich
2016-05-31 13:57 ` [Patch v6 03/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU unmapping (top level ones) Xu, Quan
2016-05-31 13:57 ` [Patch v6 04/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU mapping " Xu, Quan
2016-06-01 10:24   ` Jan Beulich
2016-06-02  7:25     ` Xu, Quan
2016-06-02  9:21       ` Jan Beulich
2016-06-02 12:43         ` Xu, Quan
2016-06-07  7:51         ` Xu, Quan
2016-06-07  8:19           ` Jan Beulich
2016-06-07  8:40             ` Xu, Quan
2016-06-07 10:11               ` Jan Beulich
2016-05-31 13:57 ` [Patch v6 05/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} " Xu, Quan
2016-05-31 13:57 ` [Patch v6 06/11] propagate IOMMU Device-TLB flush error up to EPT update " Xu, Quan
2016-05-31 13:57 ` [Patch v6 07/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending " Xu, Quan
2016-06-01 10:39   ` Jan Beulich
2016-06-02  2:58     ` Xu, Quan
2016-06-02  9:22       ` Jan Beulich
2016-05-31 13:57 ` [Patch v6 08/11] IOMMU: propagate IOMMU Device-TLB flush error (leaf ones) Xu, Quan
2016-05-31 13:57 ` [Patch v6 09/11] vt-d: fix the IOMMU flush issue Xu, Quan
2016-06-01 15:36   ` Jan Beulich
2016-06-02  2:50     ` Xu, Quan
2016-05-31 13:57 ` [Patch v6 10/11] vt-d: propagate the IOMMU Device-TLB flush error up to ME phantom functions Xu, Quan
2016-05-31 13:57 ` [Patch v6 11/11] vt-d: add __must_check annotation to IOMMU flush pointers and handlers Xu, Quan
2016-06-02 10:06   ` Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=945CA011AD5F084CBEA3E851C0AB28894B8C498A@SHSMSX101.ccr.corp.intel.com \
    --to=quan.xu@intel.com \
    --cc=JBeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dario.faggioli@citrix.com \
    --cc=george.dunlap@eu.citrix.com \
    --cc=jun.nakajima@intel.com \
    --cc=keir@xen.org \
    --cc=kevin.tian@intel.com \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).