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>,
	Dario Faggioli <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: Wed, 30 Mar 2016 02:05:56 -0600	[thread overview]
Message-ID: <56FBA50402000078000E129B@prv-mh.provo.novell.com> (raw)
In-Reply-To: <945CA011AD5F084CBEA3E851C0AB28894B871005@SHSMSX101.ccr.corp.intel.com>

>>> On 30.03.16 at 04:28, <quan.xu@intel.com> wrote:
> On March 29, 2016 3:21pm, <JBeulich@suse.com> wrote:
>> >>> On 28.03.16 at 05:33, <quan.xu@intel.com> wrote:
>> > On March 18, 2016 1:15am, <JBeulich@suse.com> wrote:
>> >> >>> On 17.03.16 at 07:54, <quan.xu@intel.com> wrote:
>> >> > --- a/xen/common/grant_table.c
>> >> > +++ b/xen/common/grant_table.c
>> >> > @@ -932,8 +932,9 @@ __gnttab_map_grant_ref(
>> >> >              {
>> >> >                  nr_gets++;
>> >> >                  (void)get_page(pg, rd);
>> >> > -                if ( !(op->flags & GNTMAP_readonly) )
>> >> > -                    get_page_type(pg, PGT_writable_page);
>> >> > +                if ( !(op->flags & GNTMAP_readonly) &&
>> >> > +                     !get_page_type(pg, PGT_writable_page) )
>> >> > +                        goto could_not_pin;
>> >>
>> >> This needs explanation, as it doesn't look related to what your
>> >> actual goal is: If an error was possible here, I think this would be
>> >> a security issue. However, as also kind of documented by the
>> >> explicitly ignored return value from get_page(), it is my understanding there
>> here we only obtain an _extra_ reference.
>> >>
>> >
>> > For this point, I inferred from:
>> > map_vcpu_info()
>> > {
>> > ...
>> >     if ( !get_page_type(page, PGT_writable_page) )
>> >     {
>> >         put_page(page);
>> >         return -EINVAL;
>> >     }
>> > ...
>> > }
>> > , then for get_page_type(), I think the return value:
>> >      0 -- error,
>> >      1-- right.
>> >
>> > So if get_page_type() is failed, we should goto could_not_pin.
>> 
>> Did you read my reply at all? The explanation I'm expecting here is why 
> error
>> checking is all of the sudden needed _at all_.
>> 
> 
> Sorry for my stupid reply.
> As in this version, before the open discussion, I try to return the 
> iommu_{,un}map_page() error in this call tree:
>            iommu_{,un}map_page() -- __get_page_type() -- get_page_type()---
> then, in this point, I try to deal with this iommu_{,un}map_page() error.

I still don't get it: We're talking about a get_page_type() invocation
that previously was known to never fail (or at least so we hope,
based on the existing code). What I'm expecting as an explanation
is why this "cannot fail" state is not true any longer. And while
sorting this out, please pay particular attention to the limited set of
cases where __get_page_type() calls iommu_{,un}map_page() in
the first place.

>> > btw, there is another issue in the call path:
>> >     iommu_{,un}map_page() -- __get_page_type() -- get_page_type()---
>> >
>> >
>> > I tried to return iommu_{,un}map_page() error code in
>> > __get_page_type(), is it right?
>> 
>> If the operation got fully rolled back - yes. Whether fully rolling back is feasible
>> there though is - see the respective discussion - an open question.
>> 
> 
> For the open question, does it refer to as below:

Partly.

> """
> As said, we first need
> to settle on an abstract model. Do we want IOMMU mapping
> failures to be fatal to the domain (perhaps with the exception
> of the hardware one)? I think we do, and for the hardware domain
> we'd do things on a best effort basis (always erring on the side
> of unmapping). Which would probably mean crashing the domain
> could be centralized in iommu_{,un}map_page(). How much roll
> back would then still be needed in callers of these functions
> for the hardware domain's sake would need to be seen.
> """
> 
> I hope it is yes.

It is not clear to me what part of the above this is meant to refer to.
Perhaps this is meant to answer the question in the 2nd sentence,
but I think this really ought to take a little more than "yes".

>> >> > --- a/xen/drivers/passthrough/x86/iommu.c
>> >> > +++ b/xen/drivers/passthrough/x86/iommu.c
>> >> > @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct
>> >> domain *d)
>> >> >      this_cpu(iommu_dont_flush_iotlb) = 0;
>> >> >
>> >> >      if ( !rc )
>> >> > -        iommu_iotlb_flush_all(d);
>> >> > +    {
>> >> > +        rc = iommu_iotlb_flush_all(d);
>> >> > +        if ( rc )
>> >> > +            iommu_teardown(d);
>> >> > +    }
>> >> >      else if ( rc != -ERESTART )
>> >> >          iommu_teardown(d);
>> >>
>> >> Why can't you just use the existing call to iommu_teardown(), by
>> >> simply
>> > deleting
>> >> the "else"?
>> >>
>> >
>> > Just check it, could I modify it as below:
>> > --- a/xen/drivers/passthrough/x86/iommu.c
>> > +++ b/xen/drivers/passthrough/x86/iommu.c
>> > @@ -105,7 +105,8 @@ int arch_iommu_populate_page_table(struct domain
>> > *d)
>> >
>> >      if ( !rc )
>> >          iommu_iotlb_flush_all(d);
>> > -    else if ( rc != -ERESTART )
>> > +
>> > +    if ( rc != -ERESTART )
>> >          iommu_teardown(d);
>> 
>> Clearly not - not only are you losing the return value of
>> iommu_iotlb_flush_all() now, you would then also call
>> iommu_teardown() in the "success" case. My comment was related to code
>> structure, yet you seem to have taken it literally.
>> 
> 
> Then, what about this one:
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -104,8 +104,9 @@ int arch_iommu_populate_page_table(struct domain *d)
>      this_cpu(iommu_dont_flush_iotlb) = 0;
> 
>      if ( !rc )
> -        iommu_iotlb_flush_all(d);
> -    else if ( rc != -ERESTART )
> +        rc = iommu_iotlb_flush_all(d);
> +
> +    if ( !rc && rc != -ERESTART )
>          iommu_teardown(d);
> 
> 
> IMO, my original modification is correct and redundant with 2 
> 'iommu_teardown()'..
> If this is still the correct one, could you help me send out the correct 
> one?

The above looks right to me.

Jan

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

  parent reply	other threads:[~2016-03-30  8:05 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
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 [this message]
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=56FBA50402000078000E129B@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dario.faggioli@citrix.com \
    --cc=feng.wu@intel.com \
    --cc=george.dunlap@eu.citrix.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).