xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: Quan Xu <quan.xu@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Feng Wu <feng.wu@intel.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Liu Jinsong <jinsong.liu@alibaba-inc.com>,
	DarioFaggioli <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Keir Fraser <keir@xen.org>
Subject: Re: [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error.
Date: Fri, 18 Mar 2016 02:19:46 -0600	[thread overview]
Message-ID: <56EBC83202000078000DE391@prv-mh.provo.novell.com> (raw)
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B86730F@SHSMSX101.ccr.corp.intel.com>

>>> On 18.03.16 at 08:54, <quan.xu@intel.com> wrote:
> On March 17, 2016 8:30pm, <dunlapg@gmail.com> wrote:
>> On Thu, Mar 17, 2016 at 6:54 AM, Quan Xu <quan.xu@intel.com> wrote:
>> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index
>> > c997b53..526548e 100644
>> > --- 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 ( !rc )
>> > +        rc = ret;
>> > +
>> 
>> What's this about?  If the iommu_[un]map_page() operation times out,
>> we still go through with calling alloc_page_type(); and if
>> alloc_page_type() fails we return its failure value, but if it
>> succeeds, we return the error from iommu_[un]map_page()?
>> 
> Yes.
> To be honest, to me, it is tricky too. 
> I found it is not right to return directly if the iommu_[un]map_page() 
> operation times out.
> 
>  """if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )"""
> Does IOMMU support pv domain? If not, we'd better remove the " if(...){...}"

What strange a question: Of course it does.

As you can infer form the reply I sent yesterday, you first need to
settle on an abstract model: What do failures mean? If, in the flush
case, a timeout is going to kill the domain anyway, then error
handling can be lighter weight than if you mean to try to keep the
domain running. Of course in this context you also should not forget
that iommu_map_page() could already return errors prior to your
changes (most notably -ENOMEM, but at least the AMD side also
produces others, with -EFAULT generally being accompanied by
domain_crash()). As mentioned elsewhere - it seems extremely
bogus that these errors weren't check for from the beginning.

Jan


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

  reply	other threads:[~2016-03-18  8:19 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-17  6:54 [PATCH 0/2] Check " Quan Xu
2016-03-17  6:54 ` [PATCH 1/2] IOMMU/MMU: Adjust top level functions for " Quan Xu
2016-03-17  7:32   ` Tian, Kevin
2016-03-17  7:58     ` Jan Beulich
2016-03-17  8:00       ` Tian, Kevin
2016-03-17 12:30   ` George Dunlap
2016-03-17 12:33     ` George Dunlap
2016-03-18  3:19       ` Xu, Quan
2016-03-18  8:09         ` Jan Beulich
2016-03-24  6:45           ` Xu, Quan
2016-03-18  7:54     ` Xu, Quan
2016-03-18  8:19       ` Jan Beulich [this message]
2016-03-18  9:09         ` Xu, Quan
2016-03-18  9:29           ` Jan Beulich
2016-03-18  9:38             ` Dario Faggioli
2016-03-18  9:48               ` Jan Beulich
2016-03-21  6:18                 ` Tian, Kevin
2016-03-21 12:22                   ` Jan Beulich
2016-03-24  9:02                 ` Xu, Quan
2016-03-24  9:58                   ` Jan Beulich
2016-03-24 14:12                     ` Xu, Quan
2016-03-24 14:37                       ` Jan Beulich
2016-03-17 17:14   ` Jan Beulich
2016-03-28  3:33     ` Xu, Quan
2016-03-29  7:20       ` Jan Beulich
2016-03-30  2:28         ` Xu, Quan
2016-03-30  2:35           ` Xu, Quan
2016-03-30  8:05           ` Jan Beulich
2016-03-17  6:54 ` [PATCH 2/2] IOMMU/MMU: Adjust low " Quan Xu
2016-03-17  7:37   ` Tian, Kevin
2016-03-18  2:30     ` Xu, Quan
2016-03-18  8:06       ` Jan Beulich
2016-03-21  5:01         ` Tian, Kevin
2016-03-17 15:31   ` George Dunlap
2016-03-18  6:57     ` Xu, Quan
2016-03-18 10:20   ` Jan Beulich
2016-03-25  9:27     ` Xu, Quan
2016-03-29  7:36       ` Jan Beulich
2016-04-11  3:09         ` Xu, Quan
2016-04-11  3:27           ` Xu, Quan
2016-04-11 16:34             ` Jan Beulich
2016-04-12  1:09               ` Xu, Quan

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=56EBC83202000078000DE391@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=George.Dunlap@eu.citrix.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dario.faggioli@citrix.com \
    --cc=feng.wu@intel.com \
    --cc=jinsong.liu@alibaba-inc.com \
    --cc=jun.nakajima@intel.com \
    --cc=keir@xen.org \
    --cc=kevin.tian@intel.com \
    --cc=quan.xu@intel.com \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error.' \
    /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

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).