[V0 PATCH 0/2] AMD PVH domU support

[V0 PATCH 0/2] AMD PVH domU support

[Mukesh Rathor]

Hi,

Here's first stab at AMD PVH domU support. Pretty much the only thing
needed is EFER bits set. Please review.

thanks,
Mukesh

[V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Mukesh Rathor]

On AMD, NX feature must be enabled in the efer for NX to be honored in
the pte entries, otherwise protection fault. We also set SC for
system calls to be enabled.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 arch/x86/xen/enlighten.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index c0cb11f..4af512d 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1499,6 +1499,17 @@ void __ref xen_pvh_secondary_vcpu_init(int cpu)
 	xen_pvh_set_cr_flags(cpu);
 }
 
+/* This is done in secondary_startup_64 for hvm guests. */
+static void __init xen_configure_efer(void)
+{
+	u64 efer;
+
+	rdmsrl(MSR_EFER, efer);
+	efer |= EFER_SCE;
+	efer |= (cpuid_edx(0x80000001) & (1 << 20)) ? EFER_NX : 0;
+	wrmsrl(MSR_EFER, efer);
+}
+
 static void __init xen_pvh_early_guest_init(void)
 {
 	if (!xen_feature(XENFEAT_auto_translated_physmap))
@@ -1508,6 +1519,7 @@ static void __init xen_pvh_early_guest_init(void)
 		return;
 
 	xen_have_vector_callback = 1;
+	xen_configure_efer();
 	xen_pvh_set_cr_flags(0);
 
 #ifdef CONFIG_X86_32
-- 
1.8.3.1

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Konrad Rzeszutek Wilk]

On Wed, Aug 20, 2014 at 07:16:39PM -0700, Mukesh Rathor wrote:
> On AMD, NX feature must be enabled in the efer for NX to be honored in
> the pte entries, otherwise protection fault. We also set SC for
> system calls to be enabled.

How come we don't need to do that for Intel (that is set the NX bit)?
Could you include the explanation here please?


> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  arch/x86/xen/enlighten.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index c0cb11f..4af512d 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1499,6 +1499,17 @@ void __ref xen_pvh_secondary_vcpu_init(int cpu)
>  	xen_pvh_set_cr_flags(cpu);
>  }
>  
> +/* This is done in secondary_startup_64 for hvm guests. */
> +static void __init xen_configure_efer(void)
> +{
> +	u64 efer;
> +
> +	rdmsrl(MSR_EFER, efer);
> +	efer |= EFER_SCE;
> +	efer |= (cpuid_edx(0x80000001) & (1 << 20)) ? EFER_NX : 0;

Ahem? #defines for these magic values please?

Or could you use 'boot_cpu_has'?

> +	wrmsrl(MSR_EFER, efer);
> +}
> +
>  static void __init xen_pvh_early_guest_init(void)
>  {
>  	if (!xen_feature(XENFEAT_auto_translated_physmap))
> @@ -1508,6 +1519,7 @@ static void __init xen_pvh_early_guest_init(void)
>  		return;
>  
>  	xen_have_vector_callback = 1;
> +	xen_configure_efer();
>  	xen_pvh_set_cr_flags(0);
>  
>  #ifdef CONFIG_X86_32
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Mukesh Rathor]

On Thu, 21 Aug 2014 21:39:04 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Wed, Aug 20, 2014 at 07:16:39PM -0700, Mukesh Rathor wrote:
> > On AMD, NX feature must be enabled in the efer for NX to be honored
> > in the pte entries, otherwise protection fault. We also set SC for
> > system calls to be enabled.
> 
> How come we don't need to do that for Intel (that is set the NX bit)?
> Could you include the explanation here please?

Intel doesn't have EFER.NX bit. The SC bit is being set in xen, but it
doesn't need to be, and I'm going to submit a patch to undo it.

> 
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  arch/x86/xen/enlighten.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > index c0cb11f..4af512d 100644
> > --- a/arch/x86/xen/enlighten.c
> > +++ b/arch/x86/xen/enlighten.c
> > @@ -1499,6 +1499,17 @@ void __ref xen_pvh_secondary_vcpu_init(int
> > cpu) xen_pvh_set_cr_flags(cpu);
> >  }
> >  
> > +/* This is done in secondary_startup_64 for hvm guests. */
> > +static void __init xen_configure_efer(void)
> > +{
> > +	u64 efer;
> > +
> > +	rdmsrl(MSR_EFER, efer);
> > +	efer |= EFER_SCE;
> > +	efer |= (cpuid_edx(0x80000001) & (1 << 20)) ? EFER_NX : 0;
> 
> Ahem? #defines for these magic values please?

Linux uses these directly all over the code as they are set in stone
pretty much, and I didn't find any #defines. See cpu/common.c for one of
the places. Also see secondary_startup_64, and others...

> Or could you use 'boot_cpu_has'?

Nop, it's not initialized at this point.

thanks,
Mukesh

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Konrad Rzeszutek Wilk]

On Thu, Aug 21, 2014 at 07:46:56PM -0700, Mukesh Rathor wrote:
> On Thu, 21 Aug 2014 21:39:04 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > On Wed, Aug 20, 2014 at 07:16:39PM -0700, Mukesh Rathor wrote:
> > > On AMD, NX feature must be enabled in the efer for NX to be honored
> > > in the pte entries, otherwise protection fault. We also set SC for
> > > system calls to be enabled.
> > 
> > How come we don't need to do that for Intel (that is set the NX bit)?
> > Could you include the explanation here please?
> 
> Intel doesn't have EFER.NX bit. The SC bit is being set in xen, but it
> doesn't need to be, and I'm going to submit a patch to undo it.

I understand that it does not have an EFER.NX bit. What I was trying
to figure out is _where_ do we set the NX bit for Intel CPUs? Is
it done in the hypervisor? In the Linux code?

Thank you.
> 
> > 
> > > 
> > > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > > ---
> > >  arch/x86/xen/enlighten.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > > index c0cb11f..4af512d 100644
> > > --- a/arch/x86/xen/enlighten.c
> > > +++ b/arch/x86/xen/enlighten.c
> > > @@ -1499,6 +1499,17 @@ void __ref xen_pvh_secondary_vcpu_init(int
> > > cpu) xen_pvh_set_cr_flags(cpu);
> > >  }
> > >  
> > > +/* This is done in secondary_startup_64 for hvm guests. */
> > > +static void __init xen_configure_efer(void)
> > > +{
> > > +	u64 efer;
> > > +
> > > +	rdmsrl(MSR_EFER, efer);
> > > +	efer |= EFER_SCE;
> > > +	efer |= (cpuid_edx(0x80000001) & (1 << 20)) ? EFER_NX : 0;
> > 
> > Ahem? #defines for these magic values please?
> 
> Linux uses these directly all over the code as they are set in stone
> pretty much, and I didn't find any #defines. See cpu/common.c for one of
> the places. Also see secondary_startup_64, and others...
> 
> > Or could you use 'boot_cpu_has'?
> 
> Nop, it's not initialized at this point.
> 
> thanks,
> Mukesh

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Borislav Petkov]

On Thu, Aug 21, 2014 at 07:46:56PM -0700, Mukesh Rathor wrote:
> Intel doesn't have EFER.NX bit.

Of course it does.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Mukesh Rathor]

On Fri, 22 Aug 2014 06:41:40 +0200
Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Aug 21, 2014 at 07:46:56PM -0700, Mukesh Rathor wrote:
> > Intel doesn't have EFER.NX bit.
> 
> Of course it does.
> 

Right, it does. Some code/comment is misleading... Anyways, reading
intel SDMs, if I understand the convoluted text correctly, EFER.NX is
not required to be set for l1.nx to be set, thus allowing for page
level protection. Where as on AMD, EFER.NX must be set for l1.nx to
be used. So, in the end, this patch would apply to both amd/intel.... 

I'll reword and submit.

Thanks,
Mukesh

Re: [Xen-devel] [V0 PATCH 1/2] AMD-PVH: set EFER.NX and EFER.SCE for the boot vcpu

[Mukesh Rathor]

On Fri, 22 Aug 2014 12:09:27 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Fri, 22 Aug 2014 06:41:40 +0200
> Borislav Petkov <bp@alien8.de> wrote:
> 
> > On Thu, Aug 21, 2014 at 07:46:56PM -0700, Mukesh Rathor wrote:
> > > Intel doesn't have EFER.NX bit.
> > 
> > Of course it does.
> > 
> 
> Right, it does. Some code/comment is misleading... Anyways, reading
> intel SDMs, if I understand the convoluted text correctly, EFER.NX is
> not required to be set for l1.nx to be set, thus allowing for page
> level protection. Where as on AMD, EFER.NX must be set for l1.nx to
> be used. So, in the end, this patch would apply to both amd/intel.... 
> 
> I'll reword and submit.

Err, try again, the section "4.1.1 Three Paging Modes" says:

"Execute-disable access rights are applied only if IA32_EFER.NXE = 1"

So, I guess NX is broken on Intel PVH because EFER.NX is currently 
not being set.  While AMD will #GP if l1.NX is set and EFER.NX is not, 
I guess Intel just ignores the l1.XD if EFER.NX is not set. 

Mukesh

[V0 PATCH 2/2] AMD-PVH: set EFER.NX and EFER.SCE for secondary vcpus

[Mukesh Rathor]

The secondary vcpus come on kernel page tables which have the NX bit set
in pte entries for DS/SS. On AMD, EFER.NX must be set to avoid protection
fault.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 arch/x86/xen/smp.c      | 28 ++++++++++++++++++++--------
 arch/x86/xen/smp.h      |  1 +
 arch/x86/xen/xen-head.S | 21 +++++++++++++++++++++
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 7005974..66058b9 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -37,6 +37,7 @@
 #include <xen/hvc-console.h>
 #include "xen-ops.h"
 #include "mmu.h"
+#include "smp.h"
 
 cpumask_var_t xen_cpu_initialized_map;
 
@@ -99,8 +100,12 @@ static void cpu_bringup(void)
 	wmb();			/* make sure everything is out */
 }
 
-/* Note: cpu parameter is only relevant for PVH */
-static void cpu_bringup_and_idle(int cpu)
+/*
+ * Note: cpu parameter is only relevant for PVH. The reason for passing it
+ * is we can't do smp_processor_id until the percpu segments are loaded, for
+ * which we need the cpu number! So we pass it in rdi as first parameter.
+ */
+asmlinkage __visible void cpu_bringup_and_idle(int cpu)
 {
 #ifdef CONFIG_X86_64
 	if (xen_feature(XENFEAT_auto_translated_physmap) &&
@@ -374,11 +379,10 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
 	ctxt->user_regs.fs = __KERNEL_PERCPU;
 	ctxt->user_regs.gs = __KERNEL_STACK_CANARY;
 #endif
-	ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
-
 	memset(&ctxt->fpu_ctxt, 0, sizeof(ctxt->fpu_ctxt));
 
 	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
+		ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
 		ctxt->flags = VGCF_IN_KERNEL;
 		ctxt->user_regs.eflags = 0x1000; /* IOPL_RING1 */
 		ctxt->user_regs.ds = __USER_DS;
@@ -416,12 +420,20 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
 #ifdef CONFIG_X86_32
 	}
 #else
-	} else
-		/* N.B. The user_regs.eip (cpu_bringup_and_idle) is called with
-		 * %rdi having the cpu number - which means are passing in
-		 * as the first parameter the cpu. Subtle!
+	} else {
+		/*
+		 * The vcpu comes on kernel page tables which have the NX pte
+		 * bit set on AMD. This means before DS/SS is touched, NX in
+		 * EFER must be set. Hence the following assembly glue code.
+		 */
+		ctxt->user_regs.eip = (unsigned long)pvh_cpu_bringup;
+
+		/* N.B. The bringup function cpu_bringup_and_idle is called with
+		 * %rdi having the cpu number - which means we are passing it in
+		 * as the first parameter. Subtle!
 		 */
 		ctxt->user_regs.rdi = cpu;
+	}
 #endif
 	ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
 	ctxt->ctrlreg[3] = xen_pfn_to_cr3(virt_to_mfn(swapper_pg_dir));
diff --git a/arch/x86/xen/smp.h b/arch/x86/xen/smp.h
index c7c2d89..b20ba68 100644
--- a/arch/x86/xen/smp.h
+++ b/arch/x86/xen/smp.h
@@ -7,5 +7,6 @@ extern void xen_send_IPI_mask_allbutself(const struct cpumask *mask,
 extern void xen_send_IPI_allbutself(int vector);
 extern void xen_send_IPI_all(int vector);
 extern void xen_send_IPI_self(int vector);
+extern void pvh_cpu_bringup(int cpu);
 
 #endif
diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 485b695..db8dca5 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -47,6 +47,27 @@ ENTRY(startup_xen)
 
 	__FINIT
 
+#ifdef CONFIG_XEN_PVH
+#ifdef CONFIG_X86_64
+/* Note that rdi contains the cpu number and must be preserved */
+ENTRY(pvh_cpu_bringup)
+	/* Gather features to see if NX implemented. (no EFER.NX on intel) */
+	movl    $0x80000001, %eax
+	cpuid
+	movl    %edx,%esi
+
+	movl    $MSR_EFER, %ecx
+	rdmsr
+	btsl    $_EFER_SCE, %eax
+
+	btl     $20,%esi
+	jnc     1f	/* No NX, skip it */
+	btsl    $_EFER_NX, %eax
+1:	wrmsr
+	jmp cpu_bringup_and_idle
+#endif /* CONFIG_X86_64 */
+#endif /* CONFIG_XEN_PVH */
+
 .pushsection .text
 	.balign PAGE_SIZE
 ENTRY(hypercall_page)
-- 
1.8.3.1

Re: [V0 PATCH 0/2] AMD PVH domU support

[David Vrabel]

On 21/08/14 03:16, Mukesh Rathor wrote:
> Hi,
> 
> Here's first stab at AMD PVH domU support. Pretty much the only thing
> needed is EFER bits set. Please review.

I'm not going to accept this until there is some ABI documentation
stating explicitly what state non-boot CPUs will be in.

I'm particularly concerned that: a) there is a difference between AMD
and Intel; and b) you want to change the ABI by clearing a the EFER.SCE bit.

David

Re: [V0 PATCH 0/2] AMD PVH domU support

[David Vrabel]

On 22/08/14 14:52, David Vrabel wrote:
> On 21/08/14 03:16, Mukesh Rathor wrote:
>> Hi,
>>
>> Here's first stab at AMD PVH domU support. Pretty much the only thing
>> needed is EFER bits set. Please review.
> 
> I'm not going to accept this until there is some ABI documentation
> stating explicitly what state non-boot CPUs will be in.

Also the boot CPU.

David

Re: [V0 PATCH 0/2] AMD PVH domU support

[Mukesh Rathor]

On Fri, 22 Aug 2014 14:55:21 +0100
David Vrabel <david.vrabel@citrix.com> wrote:

> On 22/08/14 14:52, David Vrabel wrote:
> > On 21/08/14 03:16, Mukesh Rathor wrote:
> >> Hi,
> >>
> >> Here's first stab at AMD PVH domU support. Pretty much the only
> >> thing needed is EFER bits set. Please review.
> > 
> > I'm not going to accept this until there is some ABI documentation
> > stating explicitly what state non-boot CPUs will be in.
> 
> Also the boot CPU.
> 
> David

Sure, but looks like Roger already beat me to it... 

>From Roger's "very initial PVH design document" :

And finally on `EFER` the following features are enabled:

  * LME (bit 8): Long mode enable.
  * LMA (bit 10): Long mode active.


LMK if anything additional needs to be done.

Mukesh

Re: [V0 PATCH 0/2] AMD PVH domU support

[Mukesh Rathor]

On Fri, 22 Aug 2014 14:52:41 +0100
David Vrabel <david.vrabel@citrix.com> wrote:

> On 21/08/14 03:16, Mukesh Rathor wrote:
> > Hi,
> > 
> > Here's first stab at AMD PVH domU support. Pretty much the only
> > thing needed is EFER bits set. Please review.
> 
> I'm not going to accept this until there is some ABI documentation
> stating explicitly what state non-boot CPUs will be in.
> 
> I'm particularly concerned that: a) there is a difference between AMD
> and Intel; and b) you want to change the ABI by clearing a the
> EFER.SCE bit.

Correct, I realize it changes the ABI, but I believe that is the right 
thing to do while we can, specially, since we need to fix the EFER for
NX anyways. Looking at the code, it appears this would be the final
cleanup for this ABI... :)..

However, if that's not possible, I suppose we can just leave it as is
too for the SC bit.

thanks
Mukesh