Re: [Xen-devel] [PATCH 2/2] x86: package up context switch hook pointers

[Jan Beulich]

>>> On 15.02.17 at 12:34,  wrote:
> On 15/02/17 08:42, Jan Beulich wrote:
> On 14.02.17 at 16:26,  wrote:
>>> On 14/02/17 10:29, Jan Beulich wrote:
 @@ -2066,6 +2073,15 @@ static void __context_switch(void)
  per_cpu(curr_vcpu, cpu) = n;
  }
  
 +/*
 + * Schedule tail *should* be a terminal function pointer, but leave a 
> bugframe
 + * around just incase it returns, to save going back into the context
 + * switching code and leaving a far more subtle crash to diagnose.
 + */
 +#define schedule_tail(vcpu) do {  \
 +(((vcpu)->domain->arch.ctxt_switch->tail)(vcpu)); \
 +BUG();\
 +} while (0)
>>> schedule_tail() is used only twice.  I'd suggest dropping it entirely
>>> and calling the ->tail() function pointer normally, rather than hiding
>>> it this.
>> I had considered this too, and now that you ask for it I'll happily
>> do so.
> 
> Thinking more, it would be a good idea to annotate the respective
> functions noreturn, so the compiler will catch anyone who accidently
> puts a return statement in.

Right, but in another patch I would say.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86: package up context switch hook pointers

[Andrew Cooper]

On 15/02/17 08:42, Jan Beulich wrote:
 On 14.02.17 at 16:26,  wrote:
>> On 14/02/17 10:29, Jan Beulich wrote:
>>> @@ -2066,6 +2073,15 @@ static void __context_switch(void)
>>>  per_cpu(curr_vcpu, cpu) = n;
>>>  }
>>>  
>>> +/*
>>> + * Schedule tail *should* be a terminal function pointer, but leave a 
>>> bugframe
>>> + * around just incase it returns, to save going back into the context
>>> + * switching code and leaving a far more subtle crash to diagnose.
>>> + */
>>> +#define schedule_tail(vcpu) do {  \
>>> +(((vcpu)->domain->arch.ctxt_switch->tail)(vcpu)); \
>>> +BUG();\
>>> +} while (0)
>> schedule_tail() is used only twice.  I'd suggest dropping it entirely
>> and calling the ->tail() function pointer normally, rather than hiding
>> it this.
> I had considered this too, and now that you ask for it I'll happily
> do so.

Thinking more, it would be a good idea to annotate the respective
functions noreturn, so the compiler will catch anyone who accidently
puts a return statement in.

>
>> Upon reviewing, this patch, don't we also need a ->same() call in the
>> continue_same() path in the previous patch?
> No, I did specifically check this already: The call to continue_same()
> sits (far) ahead of the clearing of ->is_running, and as long as that
> flag is set, vcpu_pause() (or vcpu_sleep_sync(), to be precise) will
> spin as necessary.

Ok.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86: package up context switch hook pointers

[Jan Beulich]

>>> On 14.02.17 at 16:26,  wrote:
> On 14/02/17 10:29, Jan Beulich wrote:
>> @@ -2066,6 +2073,15 @@ static void __context_switch(void)
>>  per_cpu(curr_vcpu, cpu) = n;
>>  }
>>  
>> +/*
>> + * Schedule tail *should* be a terminal function pointer, but leave a 
>> bugframe
>> + * around just incase it returns, to save going back into the context
>> + * switching code and leaving a far more subtle crash to diagnose.
>> + */
>> +#define schedule_tail(vcpu) do {  \
>> +(((vcpu)->domain->arch.ctxt_switch->tail)(vcpu)); \
>> +BUG();\
>> +} while (0)
> 
> schedule_tail() is used only twice.  I'd suggest dropping it entirely
> and calling the ->tail() function pointer normally, rather than hiding
> it this.

I had considered this too, and now that you ask for it I'll happily
do so.

> Upon reviewing, this patch, don't we also need a ->same() call in the
> continue_same() path in the previous patch?

No, I did specifically check this already: The call to continue_same()
sits (far) ahead of the clearing of ->is_running, and as long as that
flag is set, vcpu_pause() (or vcpu_sleep_sync(), to be precise) will
spin as necessary.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86: package up context switch hook pointers

[Boris Ostrovsky]

On 02/14/2017 05:29 AM, Jan Beulich wrote:

They're all solely dependent on guest type, so we don't need to repeat
all the same four pointers in every vCPU control structure. Instead use
static const structures, and store pointers to them in the domain
control structure.

Since touching it anyway, take the opportunity and move schedule_tail()
into the only C file needing it.

Signed-off-by: Jan Beulich 


Reviewed-by: Boris Ostrovsky 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/2] x86: package up context switch hook pointers

[Andrew Cooper]

On 14/02/17 10:29, Jan Beulich wrote:
> They're all solely dependent on guest type, so we don't need to repeat
> all the same four pointers in every vCPU control structure. Instead use
> static const structures, and store pointers to them in the domain
> control structure.
>
> Since touching it anyway, take the opportunity and move schedule_tail()
> into the only C file needing it.
>
> Signed-off-by: Jan Beulich 

+1.  This has been a niggle on my todo list for ages.

> @@ -2066,6 +2073,15 @@ static void __context_switch(void)
>  per_cpu(curr_vcpu, cpu) = n;
>  }
>  
> +/*
> + * Schedule tail *should* be a terminal function pointer, but leave a 
> bugframe
> + * around just incase it returns, to save going back into the context
> + * switching code and leaving a far more subtle crash to diagnose.
> + */
> +#define schedule_tail(vcpu) do {  \
> +(((vcpu)->domain->arch.ctxt_switch->tail)(vcpu)); \
> +BUG();\
> +} while (0)

schedule_tail() is used only twice.  I'd suggest dropping it entirely
and calling the ->tail() function pointer normally, rather than hiding
it this.

Upon reviewing, this patch, don't we also need a ->same() call in the
continue_same() path in the previous patch?

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel