Re: -EINTR return in domain_relinquish_resources

[Andrew Cooper <andrew.cooper3@citrix.com>]

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