Re: [PATCH 1/2] x86/xen: Set EFER.NX and EFER.SCE in PVH guests

[David Vrabel <david.vrabel@citrix.com>]

On 05/09/14 17:04, Jan Beulich wrote:
>>>> On 05.09.14 at 16:46, <david.vrabel@citrix.com> wrote:
>> On 05/09/14 15:34, Jan Beulich wrote:
>>>>>> On 05.09.14 at 16:11, <david.vrabel@citrix.com> wrote:
>>>> --- a/arch/x86/xen/xen-head.S
>>>> +++ b/arch/x86/xen/xen-head.S
>>>> @@ -47,6 +47,36 @@ ENTRY(startup_xen)
>>>>  
>>>>  	__FINIT
>>>>  
>>>> +#ifdef CONFIG_XEN_PVH
>>>> +
>>>> +/**
>>>> + * xen_pvh_cpu_early_init() - early PVH VCPU initialization
>>>> + * @cpu: this cpu number (%rdi)
>>>> + *
>>>> + * Note: This is called as a function on the boot CPU and as the secondary
>>>> + * CPU entry point.
>>>> + */
>>>> +ENTRY(xen_pvh_cpu_early_init)
>>>> +	/* Gather features to see if NX implemented. */
>>>> +	mov     $0x80000001, %eax
>>>> +	cpuid
>>>> +	mov     %edx,%esi
>>>> +
>>>> +	mov     $MSR_EFER, %ecx
>>>> +	rdmsr
>>>> +	bts     $_EFER_SCE, %eax
>>>> +
>>>> +	bt      $20,%esi
>>>> +	jnc     1f      /* No NX, skip setting it */
>>>> +	bts     $_EFER_NX, %eax
>>>> +1:	wrmsr
>>>> +
>>>> +	cmp     $0,%rdi /* non-zero => secondary cpu */
>>>
>>> Isn't Linux is specifically moving away from this assumption?
>>
>> I don't think this is relevant.  Is Xen ever going boot with a non-zero
>> VCPU?
> 
> That's not the question here. The question is - will CPU 0 ever be
> possible to be brought down and back up? And I hope the answer
> to this isn't "No, never".

Oh, that's a good point. I'll add the flag back in.

>>>> +	jne     cpu_bringup_and_idle
>>>> +	ret
>>>> +
>>>> +#endif /* CONFIG_XEN_PVH */
>>>
>>> Also, does all this really need to be done in assembly?
>>
>> There's no usable stack until EFER.NX is set.  I couldn't think of a way
>> to write this in C that would guarantee no stack use, so I didn't
>> suggest it.
>>
>> Can you think of a way?
> 
> No stack use is a very convincing argument for using assembly?

Is that question mark a typo?  If not, I'm not really sure how to answer.

> I must have overlooked it if that was said somewhere in a comment.

>From the commit message:

"Secondary VCPUs are started with pagetables with the NX bit set so
EFER.NX must be set before using any stack or data segment.
xen_pvh_cpu_early_init() is the new secondary VCPU entry point that
sets EFER before jumping to cpu_bringup_and_idle()."

And a comment:

+	 * The vcpu comes on kernel page tables which have the NX pte
+	 * bit set. This means before DS/SS is touched, NX in
+	 * EFER must be set. Hence the following assembly glue code.

If this isn't sufficiently clear, can you suggest some improved wording?

David