-EINTR return in domain_relinquish_resources

-EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

As I was looking at some of the XSA I realized that the
call-chain of:

 domain_relinquish_resources
   ->vcpu_destroy_pagetables
     -> put_page_and_type_preemptible
        -> __put_page_type
		returns -EINTR

which means we end up at:
 618         rc = domain_relinquish_resources(d);                                    
 619         if ( rc != 0 )                                                          
 620         {                                                                       
 621             if ( rc == -ERESTART )                                              
 622                 rc = -EAGAIN;                                                   
 623             break;             <=== with rc=-EINTR
 624         }                                         

And return -EINTR to user-space - which loop in 
'xc_domain_destroy' is only looking for:

 112 int xc_domain_destroy(xc_interface *xch,                                        
 113                       uint32_t domid)                                           
 114 {                                                                               
 115     int ret;                                                                    
 116     DECLARE_DOMCTL;                                                             
 117     domctl.cmd = XEN_DOMCTL_destroydomain;                                      
 118     domctl.domain = (domid_t)domid;                                             
 119     do {                                                                        
 120         ret = do_domctl(xch, &domctl);                                          
 121     } while ( ret && (errno == EAGAIN) );                                       
 122     return ret;                                                                 
 123 }                                

which to my reading looks like we would exit out and leave
an DOMDYING_dying domain. Looking at the code it seems possible
to continue on if the user does 'xl destroy <X>' guest again,
but I was wondering if:

 a). Should the toolstack (libxl or libxc) have the code to
     handle -EINTR?

 b). Or should the hypervisor convert the -EINTR to -ERESTART
     (or -EAGAIN) - which most of the code (see users of
     get_page_type_preemptible) do right now?

Thanks!

Re: -EINTR return in domain_relinquish_resources

[Andrew Cooper]

On 21/01/2015 21:27, Konrad Rzeszutek Wilk wrote:
> As I was looking at some of the XSA I realized that the
> call-chain of:
>
>  domain_relinquish_resources
>    ->vcpu_destroy_pagetables
>      -> put_page_and_type_preemptible
>         -> __put_page_type
> 		returns -EINTR
>
> which means we end up at:
>  618         rc = domain_relinquish_resources(d);                                    
>  619         if ( rc != 0 )                                                          
>  620         {                                                                       
>  621             if ( rc == -ERESTART )                                              
>  622                 rc = -EAGAIN;                                                   
>  623             break;             <=== with rc=-EINTR
>  624         }                                         
>
> And return -EINTR to user-space - which loop in 
> 'xc_domain_destroy' is only looking for:
>
>  112 int xc_domain_destroy(xc_interface *xch,                                        
>  113                       uint32_t domid)                                           
>  114 {                                                                               
>  115     int ret;                                                                    
>  116     DECLARE_DOMCTL;                                                             
>  117     domctl.cmd = XEN_DOMCTL_destroydomain;                                      
>  118     domctl.domain = (domid_t)domid;                                             
>  119     do {                                                                        
>  120         ret = do_domctl(xch, &domctl);                                          
>  121     } while ( ret && (errno == EAGAIN) );                                       
>  122     return ret;                                                                 
>  123 }                                
>
> which to my reading looks like we would exit out and leave
> an DOMDYING_dying domain. Looking at the code it seems possible
> to continue on if the user does 'xl destroy <X>' guest again,
> but I was wondering if:
>
>  a). Should the toolstack (libxl or libxc) have the code to
>      handle -EINTR?
>
>  b). Or should the hypervisor convert the -EINTR to -ERESTART
>      (or -EAGAIN) - which most of the code (see users of
>      get_page_type_preemptible) do right now?

Good spot.

Other areas of similar code condense EINTR into ERESTART.  I think in
this case it is Xen's job to turn -EINTR into -EAGAIN as this hypercall
specifically has preemptibility  built into its normal use.

I wonder if there are other similar hypercall paths which need to catch
EINTR as well as ERESTART?

~Andrew

Re: -EINTR return in domain_relinquish_resources

[Andrew Cooper]

On 21/01/2015 21:39, Andrew Cooper wrote:
> On 21/01/2015 21:27, Konrad Rzeszutek Wilk wrote:
>> As I was looking at some of the XSA I realized that the
>> call-chain of:
>>
>>  domain_relinquish_resources
>>    ->vcpu_destroy_pagetables
>>      -> put_page_and_type_preemptible
>>         -> __put_page_type
>> 		returns -EINTR
>>
>> which means we end up at:
>>  618         rc = domain_relinquish_resources(d);                                    
>>  619         if ( rc != 0 )                                                          
>>  620         {                                                                       
>>  621             if ( rc == -ERESTART )                                              
>>  622                 rc = -EAGAIN;                                                   
>>  623             break;             <=== with rc=-EINTR
>>  624         }                                         
>>
>> And return -EINTR to user-space - which loop in 
>> 'xc_domain_destroy' is only looking for:
>>
>>  112 int xc_domain_destroy(xc_interface *xch,                                        
>>  113                       uint32_t domid)                                           
>>  114 {                                                                               
>>  115     int ret;                                                                    
>>  116     DECLARE_DOMCTL;                                                             
>>  117     domctl.cmd = XEN_DOMCTL_destroydomain;                                      
>>  118     domctl.domain = (domid_t)domid;                                             
>>  119     do {                                                                        
>>  120         ret = do_domctl(xch, &domctl);                                          
>>  121     } while ( ret && (errno == EAGAIN) );                                       
>>  122     return ret;                                                                 
>>  123 }                                
>>
>> which to my reading looks like we would exit out and leave
>> an DOMDYING_dying domain. Looking at the code it seems possible
>> to continue on if the user does 'xl destroy <X>' guest again,
>> but I was wondering if:
>>
>>  a). Should the toolstack (libxl or libxc) have the code to
>>      handle -EINTR?
>>
>>  b). Or should the hypervisor convert the -EINTR to -ERESTART
>>      (or -EAGAIN) - which most of the code (see users of
>>      get_page_type_preemptible) do right now?
> Good spot.
>
> Other areas of similar code condense EINTR into ERESTART.  I think in
> this case it is Xen's job to turn -EINTR into -EAGAIN as this hypercall
> specifically has preemptibility  built into its normal use.
>
> I wonder if there are other similar hypercall paths which need to catch
> EINTR as well as ERESTART?
>
> ~Andrew

Thinking about this, it occurs to me that, along with parameter
clobbering in debug builds, we should assert that internal error codes
never escape to guests.

It also occurs to me that the PV hypercall paths would both be far more
simple (particularly the register clobbering bits) if they were written
in C like their HVM counterparts, rather than ASM.  I will see whether I
can find some copious free time to see about making this happen.

~Andrew

Re: -EINTR return in domain_relinquish_resources

[Jan Beulich]

>>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
> As I was looking at some of the XSA I realized that the
> call-chain of:
> 
>  domain_relinquish_resources
>    ->vcpu_destroy_pagetables
>      -> put_page_and_type_preemptible
>         -> __put_page_type
> 		returns -EINTR
> [...]
>  b). Or should the hypervisor convert the -EINTR to -ERESTART
>      (or -EAGAIN) - which most of the code (see users of
>      get_page_type_preemptible) do right now?

This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
this when adding the preemption capability here.

Jan

Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

On Wed, Jan 21, 2015 at 10:57:05PM +0000, Andrew Cooper wrote:
> On 21/01/2015 21:39, Andrew Cooper wrote:
> > On 21/01/2015 21:27, Konrad Rzeszutek Wilk wrote:
> >> As I was looking at some of the XSA I realized that the
> >> call-chain of:
> >>
> >>  domain_relinquish_resources
> >>    ->vcpu_destroy_pagetables
> >>      -> put_page_and_type_preemptible
> >>         -> __put_page_type
> >> 		returns -EINTR
> >>
> >> which means we end up at:
> >>  618         rc = domain_relinquish_resources(d);                                    
> >>  619         if ( rc != 0 )                                                          
> >>  620         {                                                                       
> >>  621             if ( rc == -ERESTART )                                              
> >>  622                 rc = -EAGAIN;                                                   
> >>  623             break;             <=== with rc=-EINTR
> >>  624         }                                         
> >>
> >> And return -EINTR to user-space - which loop in 
> >> 'xc_domain_destroy' is only looking for:
> >>
> >>  112 int xc_domain_destroy(xc_interface *xch,                                        
> >>  113                       uint32_t domid)                                           
> >>  114 {                                                                               
> >>  115     int ret;                                                                    
> >>  116     DECLARE_DOMCTL;                                                             
> >>  117     domctl.cmd = XEN_DOMCTL_destroydomain;                                      
> >>  118     domctl.domain = (domid_t)domid;                                             
> >>  119     do {                                                                        
> >>  120         ret = do_domctl(xch, &domctl);                                          
> >>  121     } while ( ret && (errno == EAGAIN) );                                       
> >>  122     return ret;                                                                 
> >>  123 }                                
> >>
> >> which to my reading looks like we would exit out and leave
> >> an DOMDYING_dying domain. Looking at the code it seems possible
> >> to continue on if the user does 'xl destroy <X>' guest again,
> >> but I was wondering if:
> >>
> >>  a). Should the toolstack (libxl or libxc) have the code to
> >>      handle -EINTR?
> >>
> >>  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >>      (or -EAGAIN) - which most of the code (see users of
> >>      get_page_type_preemptible) do right now?
> > Good spot.
> >
> > Other areas of similar code condense EINTR into ERESTART.  I think in
> > this case it is Xen's job to turn -EINTR into -EAGAIN as this hypercall
> > specifically has preemptibility  built into its normal use.
> >
> > I wonder if there are other similar hypercall paths which need to catch
> > EINTR as well as ERESTART?

I did not see them.
> >
> > ~Andrew
> 
> Thinking about this, it occurs to me that, along with parameter
> clobbering in debug builds, we should assert that internal error codes
> never escape to guests.

Are there any other ones besides ERESTART/EINTR ?
> 
> It also occurs to me that the PV hypercall paths would both be far more
> simple (particularly the register clobbering bits) if they were written
> in C like their HVM counterparts, rather than ASM.  I will see whether I
> can find some copious free time to see about making this happen.
> 
> ~Andrew

Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
> >>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
> > As I was looking at some of the XSA I realized that the
> > call-chain of:
> > 
> >  domain_relinquish_resources
> >    ->vcpu_destroy_pagetables
> >      -> put_page_and_type_preemptible
> >         -> __put_page_type
> > 		returns -EINTR
> > [...]
> >  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >      (or -EAGAIN) - which most of the code (see users of
> >      get_page_type_preemptible) do right now?
> 
> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
> this when adding the preemption capability here.

OK. Would this RFC patch be OK? (I can send it off as normal - just
want to make sure you are OK with this being put there)

>From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 22 Jan 2015 11:34:21 -0500
Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
 instead of -EAGAIN

which has the side effect that domain_relinquish_resources will stop
and return to user-space -EINTR - which it is not equipped to deal with.
The preemption mechanism we have to domain destruction is to return
-EAGAIN and as such we need to catch the case of:

domain_relinquish_resources
  ->vcpu_destroy_pagetables
    -> put_page_and_type_preemptible
       -> __put_page_type
           returns -EINTR

and convert it to the proper type.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/x86/mm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6e9c2c0..3ac2ae5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2677,6 +2677,9 @@ int vcpu_destroy_pagetables(struct vcpu *v)
 
     v->arch.cr3 = 0;
 
+    if ( rc == -EINTR )
+        rc = -EAGAIN;
+
     return rc;
 }
 
-- 
2.1.0

Re: -EINTR return in domain_relinquish_resources

[Jan Beulich]

>>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
>> >>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
>> > As I was looking at some of the XSA I realized that the
>> > call-chain of:
>> > 
>> >  domain_relinquish_resources
>> >    ->vcpu_destroy_pagetables
>> >      -> put_page_and_type_preemptible
>> >         -> __put_page_type
>> > 		returns -EINTR
>> > [...]
>> >  b). Or should the hypervisor convert the -EINTR to -ERESTART
>> >      (or -EAGAIN) - which most of the code (see users of
>> >      get_page_type_preemptible) do right now?
>> 
>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
>> this when adding the preemption capability here.
> 
> OK. Would this RFC patch be OK? (I can send it off as normal - just
> want to make sure you are OK with this being put there)

Conceptually yes, but there are issues:

> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Thu, 22 Jan 2015 11:34:21 -0500
> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
>  instead of -EAGAIN

You should stop sending such conversions to EAGAIN. We switched
to ERESTART, and you (just guessing) taking the patch from an
older Xen version shouldn't result in this recurring mistake.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2677,6 +2677,9 @@ int vcpu_destroy_pagetables(struct vcpu *v)
>  
>      v->arch.cr3 = 0;
>  
> +    if ( rc == -EINTR )
> +        rc = -EAGAIN;
> +

Either (my preference) you attach this directly to the two
put_page_and_type_preemptible() invocations, or you add a
comment here explaining that this catches those two specific
cases in one central place. This is because while right now only
those two invocations can lead to rc being non-zero, there's
nothing preventing other error generating code to be added to
this function later on, and we shouldn't blindly convert -EINTR
to some other error code, as that may lead to overlooking
necessary conversions elsewhere (as happened while I made
this function preemptible).

Jan

Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
> >>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
> >> >>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
> >> > As I was looking at some of the XSA I realized that the
> >> > call-chain of:
> >> > 
> >> >  domain_relinquish_resources
> >> >    ->vcpu_destroy_pagetables
> >> >      -> put_page_and_type_preemptible
> >> >         -> __put_page_type
> >> > 		returns -EINTR
> >> > [...]
> >> >  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >> >      (or -EAGAIN) - which most of the code (see users of
> >> >      get_page_type_preemptible) do right now?
> >> 
> >> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
> >> this when adding the preemption capability here.
> > 
> > OK. Would this RFC patch be OK? (I can send it off as normal - just
> > want to make sure you are OK with this being put there)
> 
> Conceptually yes, but there are issues:
> 
> > From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Thu, 22 Jan 2015 11:34:21 -0500
> > Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
> >  instead of -EAGAIN
> 
> You should stop sending such conversions to EAGAIN. We switched
> to ERESTART, and you (just guessing) taking the patch from an
> older Xen version shouldn't result in this recurring mistake.

Nah, Andrew said in his email EAGAIN so that is what I picked.
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2677,6 +2677,9 @@ int vcpu_destroy_pagetables(struct vcpu *v)
> >  
> >      v->arch.cr3 = 0;
> >  
> > +    if ( rc == -EINTR )
> > +        rc = -EAGAIN;
> > +
> 
> Either (my preference) you attach this directly to the two
> put_page_and_type_preemptible() invocations, or you add a
> comment here explaining that this catches those two specific
> cases in one central place. This is because while right now only

I will add explanation. The other users of put_page_.. all have
catch the -EINTR and do the conversion.

> those two invocations can lead to rc being non-zero, there's
> nothing preventing other error generating code to be added to
> this function later on, and we shouldn't blindly convert -EINTR
> to some other error code, as that may lead to overlooking
> necessary conversions elsewhere (as happened while I made
> this function preemptible).
> 
> Jan
> 

Re: -EINTR return in domain_relinquish_resources

[Andrew Cooper]

On 23/01/15 14:32, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
>>>>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
>>> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
>>>>>>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
>>>>> As I was looking at some of the XSA I realized that the
>>>>> call-chain of:
>>>>>
>>>>>  domain_relinquish_resources
>>>>>    ->vcpu_destroy_pagetables
>>>>>      -> put_page_and_type_preemptible
>>>>>         -> __put_page_type
>>>>> 		returns -EINTR
>>>>> [...]
>>>>>  b). Or should the hypervisor convert the -EINTR to -ERESTART
>>>>>      (or -EAGAIN) - which most of the code (see users of
>>>>>      get_page_type_preemptible) do right now?
>>>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
>>>> this when adding the preemption capability here.
>>> OK. Would this RFC patch be OK? (I can send it off as normal - just
>>> want to make sure you are OK with this being put there)
>> Conceptually yes, but there are issues:
>>
>>> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> Date: Thu, 22 Jan 2015 11:34:21 -0500
>>> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
>>>  instead of -EAGAIN
>> You should stop sending such conversions to EAGAIN. We switched
>> to ERESTART, and you (just guessing) taking the patch from an
>> older Xen version shouldn't result in this recurring mistake.
> Nah, Andrew said in his email EAGAIN so that is what I picked.

EAGAIN was correct for the domain_destroy hypercall, at this hypercall
explicitly has continuation built into its API via EAGAIN.

However, you patched a more generic path which affects callers, and is
therefore incomplete.  I am sorry if I did not explain this very well.

~Andrew

Re: -EINTR return in domain_relinquish_resources

[Jan Beulich]

>>> On 23.01.15 at 15:46, <andrew.cooper3@citrix.com> wrote:
> On 23/01/15 14:32, Konrad Rzeszutek Wilk wrote:
>> On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
>>>>>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
>>>> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
>>>>>>>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
>>>>>> As I was looking at some of the XSA I realized that the
>>>>>> call-chain of:
>>>>>>
>>>>>>  domain_relinquish_resources
>>>>>>    ->vcpu_destroy_pagetables
>>>>>>      -> put_page_and_type_preemptible
>>>>>>         -> __put_page_type
>>>>>> 		returns -EINTR
>>>>>> [...]
>>>>>>  b). Or should the hypervisor convert the -EINTR to -ERESTART
>>>>>>      (or -EAGAIN) - which most of the code (see users of
>>>>>>      get_page_type_preemptible) do right now?
>>>>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
>>>>> this when adding the preemption capability here.
>>>> OK. Would this RFC patch be OK? (I can send it off as normal - just
>>>> want to make sure you are OK with this being put there)
>>> Conceptually yes, but there are issues:
>>>
>>>> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
>>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>> Date: Thu, 22 Jan 2015 11:34:21 -0500
>>>> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
>>>>  instead of -EAGAIN
>>> You should stop sending such conversions to EAGAIN. We switched
>>> to ERESTART, and you (just guessing) taking the patch from an
>>> older Xen version shouldn't result in this recurring mistake.
>> Nah, Andrew said in his email EAGAIN so that is what I picked.
> 
> EAGAIN was correct for the domain_destroy hypercall, at this hypercall
> explicitly has continuation built into its API via EAGAIN.

Ouch - I based the comment on code resulting from a patch I
didn't send out yet (largely because Konrad indicated issues with
XEN_DOMCTL_destroydomain that I'd need to look into in more
detail before doing so), doing away with the tool stack based
continuations. Yet based on what domain_kill() does with
domain_relinquish_resources()'s return value, it should
nevertheless be -ERESTART here to ease future changes.

Jan

Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

On Fri, Jan 23, 2015 at 03:34:51PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 15:46, <andrew.cooper3@citrix.com> wrote:
> > On 23/01/15 14:32, Konrad Rzeszutek Wilk wrote:
> >> On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
> >>>>>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
> >>>> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
> >>>>>>>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
> >>>>>> As I was looking at some of the XSA I realized that the
> >>>>>> call-chain of:
> >>>>>>
> >>>>>>  domain_relinquish_resources
> >>>>>>    ->vcpu_destroy_pagetables
> >>>>>>      -> put_page_and_type_preemptible
> >>>>>>         -> __put_page_type
> >>>>>> 		returns -EINTR
> >>>>>> [...]
> >>>>>>  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >>>>>>      (or -EAGAIN) - which most of the code (see users of
> >>>>>>      get_page_type_preemptible) do right now?
> >>>>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
> >>>>> this when adding the preemption capability here.
> >>>> OK. Would this RFC patch be OK? (I can send it off as normal - just
> >>>> want to make sure you are OK with this being put there)
> >>> Conceptually yes, but there are issues:
> >>>
> >>>> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
> >>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>> Date: Thu, 22 Jan 2015 11:34:21 -0500
> >>>> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
> >>>>  instead of -EAGAIN
> >>> You should stop sending such conversions to EAGAIN. We switched
> >>> to ERESTART, and you (just guessing) taking the patch from an
> >>> older Xen version shouldn't result in this recurring mistake.
> >> Nah, Andrew said in his email EAGAIN so that is what I picked.
> > 
> > EAGAIN was correct for the domain_destroy hypercall, at this hypercall
> > explicitly has continuation built into its API via EAGAIN.
> 
> Ouch - I based the comment on code resulting from a patch I
> didn't send out yet (largely because Konrad indicated issues with
> XEN_DOMCTL_destroydomain that I'd need to look into in more
> detail before doing so), doing away with the tool stack based
> continuations. Yet based on what domain_kill() does with
> domain_relinquish_resources()'s return value, it should
> nevertheless be -ERESTART here to ease future changes.

OK :-)


>From ac642805a96261f518fbba7d47f3ca38c950b3c8 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 22 Jan 2015 11:34:21 -0500
Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART
 instead of -EINTR

which has the side effect that domain_relinquish_resources will stop
and return to user-space -EINTR - which it is not equipped to deal with.
The preemption mechanism we have to domain destruction is to return
-EAGAIN and as such we need to catch the case of:

domain_relinquish_resources
  ->vcpu_destroy_pagetables
    -> put_page_and_type_preemptible
       -> __put_page_type
           returns -EINTR

and convert it to the proper type.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Add comment and s/ERESTART/EAGAIN/
---
 xen/arch/x86/mm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6e9c2c0..5452c01 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2677,6 +2677,16 @@ int vcpu_destroy_pagetables(struct vcpu *v)
 
     v->arch.cr3 = 0;
 
+    /*
+     * The put_page_and_type_preemptible is liable to return -EINTR. Other
+     * callers of it filter the -EINTR to whatever they deem applicable - in
+     * this case we MUST do it as the caller of this function will return the
+     * error code to userspace. And userspace for domain destruction expects
+     * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN).
+     */
+    if ( rc == -EINTR )
+        rc = -ERESTART;
+
     return rc;
 }
 
-- 
2.1.0

Re: -EINTR return in domain_relinquish_resources

[Jan Beulich]

>>> On 23.01.15 at 16:46, <konrad.wilk@oracle.com> wrote:
> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART
>  instead of -EINTR
> 
> which has the side effect that domain_relinquish_resources will stop
> and return to user-space -EINTR - which it is not equipped to deal with.

The title read wrong, especially on its own, as it appears to
state the inverse thing of what you do in the patch. Perhaps

x86: vcpu_destroy_pagetables() must not return -EINTR

with the initial part of the description adjusted accordingly?

> +    /*
> +     * The put_page_and_type_preemptible is liable to return -EINTR. Other
> +     * callers of it filter the -EINTR to whatever they deem applicable - in
> +     * this case we MUST do it as the caller of this function will return the
> +     * error code to userspace. And userspace for domain destruction expects
> +     * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN).
> +     */

This is still misleading, as it kind of implies that the function has only
that one caller. Don't talk about domain_relinquish_resources() and
EAGAIN at all.

Jan

Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk]

On Fri, Jan 23, 2015 at 04:03:55PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 16:46, <konrad.wilk@oracle.com> wrote:
> > Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART
> >  instead of -EINTR
> > 
> > which has the side effect that domain_relinquish_resources will stop
> > and return to user-space -EINTR - which it is not equipped to deal with.
> 
> The title read wrong, especially on its own, as it appears to
> state the inverse thing of what you do in the patch. Perhaps
> 
> x86: vcpu_destroy_pagetables() must not return -EINTR
> 
> with the initial part of the description adjusted accordingly?
> 
> > +    /*
> > +     * The put_page_and_type_preemptible is liable to return -EINTR. Other
> > +     * callers of it filter the -EINTR to whatever they deem applicable - in
> > +     * this case we MUST do it as the caller of this function will return the
> > +     * error code to userspace. And userspace for domain destruction expects
> > +     * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN).
> > +     */
> 
> This is still misleading, as it kind of implies that the function has only
> that one caller. Don't talk about domain_relinquish_resources() and
> EAGAIN at all.

Right. I somehow managed to miss the other caller of vcpu_destroy_pagetables.

Please see following patch:

>From 3ace309c61805bae546378e37553913231e43cec Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 22 Jan 2015 11:34:21 -0500
Subject: [PATCH] domain: vcpu_destroy_pagetables() must not return -EINTR

.. otherwise it has the side effect that: domain_relinquish_resources
will stop and will return to user-space with -EINTR which it is not
equipped to deal with that error code; or vcpu_reset - which will
ignore it and convert the error to -ENOMEM..

The preemption mechanism we have for domain destruction is to return
-EAGAIN (and then user-space calls the hypercall again) and as such we need
to catch the case of:

domain_relinquish_resources
  ->vcpu_destroy_pagetables
    -> put_page_and_type_preemptible
       -> __put_page_type
           returns -EINTR

and convert it to the proper type. For:

XEN_DOMCTL_setvcpucontext
 -> vcpu_reset
   -> vcpu_destroy_pagetables

we need to return -ERESTART otherwise we end up returning -ENOMEM.

There are also other callers of vcpu_destroy_pagetables: arch_vcpu_reset
(vcpu_reset) are:
 - hvm_s3_suspend (asserts on any return code),
 - vlapic_init_sipi_one (asserts on any return code),

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Add comment and s/ERESTART/EAGAIN/
v3: Also include the hypercall.
---
 xen/arch/x86/mm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6e9c2c0..badbeab 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2677,6 +2677,13 @@ int vcpu_destroy_pagetables(struct vcpu *v)
 
     v->arch.cr3 = 0;
 
+    /*
+     * The put_page_and_type_preemptible is liable to return -EINTR. The
+     * callers of us expect -ERESTART so convert it over.
+     */
+    if ( rc == -EINTR )
+        rc = -ERESTART;
+
     return rc;
 }
 
-- 
2.1.0

> 
> Jan
> 

Re: -EINTR return in domain_relinquish_resources

[Jan Beulich]

>>> On 23.01.15 at 18:21, <konrad.wilk@oracle.com> wrote:
> On Fri, Jan 23, 2015 at 04:03:55PM +0000, Jan Beulich wrote:
>> >>> On 23.01.15 at 16:46, <konrad.wilk@oracle.com> wrote:
>> > Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART
>> >  instead of -EINTR
>> > 
>> > which has the side effect that domain_relinquish_resources will stop
>> > and return to user-space -EINTR - which it is not equipped to deal with.
>> 
>> The title read wrong, especially on its own, as it appears to
>> state the inverse thing of what you do in the patch. Perhaps
>> 
>> x86: vcpu_destroy_pagetables() must not return -EINTR
>> 
>> with the initial part of the description adjusted accordingly?
>> 
>> > +    /*
>> > +     * The put_page_and_type_preemptible is liable to return -EINTR. Other
>> > +     * callers of it filter the -EINTR to whatever they deem applicable - in
>> > +     * this case we MUST do it as the caller of this function will return the
>> > +     * error code to userspace. And userspace for domain destruction expects
>> > +     * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN).
>> > +     */
>> 
>> This is still misleading, as it kind of implies that the function has only
>> that one caller. Don't talk about domain_relinquish_resources() and
>> EAGAIN at all.
> 
> Right. I somehow managed to miss the other caller of 
> vcpu_destroy_pagetables.
> 
> Please see following patch:

Looks good. I don't see a reason not to apply it without you doing
a formal submission.

Jan