Re: [V0 PATCH] xen/pvh: set some cr flags upon vcpu start

[Konrad Rzeszutek Wilk]

On Fri, Jan 17, 2014 at 06:24:55PM -0800, Mukesh Rathor wrote:
> pvh was designed to start with pv flags, but a commit in xen tree

Thank you for posting this!

> 51e2cac257ec8b4080d89f0855c498cbbd76a5e5 removed some of the flags as

You need to always include the title of said commit.

> they are not necessary. As a result, these CR flags must be set in the
> guest.

I sent out replies to this over the weekend but somehow they are not
showing up.


> 
> Signed-off-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>

Since his SoB is first that implies that he is the author. But the
'From' line is from you. You can modify that by doing:

git commit --amend --author "Roger Pau Monne <roger.pau@xxxxxxxxxx>"

> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  arch/x86/xen/enlighten.c |   43 +++++++++++++++++++++++++++++++++++++------
>  arch/x86/xen/smp.c       |    2 +-
>  arch/x86/xen/xen-ops.h   |    2 +-
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 628099a..4a2aaa6 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1410,12 +1410,8 @@ static void __init xen_boot_params_init_edd(void)
>   * Set up the GDT and segment registers for -fstack-protector.  Until
>   * we do this, we have to be careful not to call any stack-protected
>   * function, which is most of the kernel.
> - *
> - * Note, that it is refok - because the only caller of this after init
> - * is PVH which is not going to use xen_load_gdt_boot or other
> - * __init functions.
>   */
> -void __ref xen_setup_gdt(int cpu)
> +static void xen_setup_gdt(int cpu)
>  {
>  	if (xen_feature(XENFEAT_auto_translated_physmap)) {
>  #ifdef CONFIG_X86_64
> @@ -1463,13 +1459,48 @@ void __ref xen_setup_gdt(int cpu)
>  	pv_cpu_ops.load_gdt = xen_load_gdt;
>  }
>  
> +/*
> + * A pv guest starts with default flags that are not set for pvh, set them
> + * here asap.
> + */
> +static void xen_pvh_set_cr_flags(int cpu)
> +{
> +	write_cr0(read_cr0() | X86_CR0_MP | X86_CR0_WP | X86_CR0_AM);

You seem to be missing the X86_CR0_NE ?

The hypervisor sets "X86_CR0_PG" for PVH and uncondtionally for HVM
X86_CR0_TS, X86_CR0_PE, X86_CR0_ET so from the list of them that
'secondary_startup_64' sets:

208         /* Setup cr0 */
209 #define CR0_STATE       (X86_CR0_PE | X86_CR0_MP | X86_CR0_ET | \
210                          X86_CR0_NE | X86_CR0_WP | X86_CR0_AM | \
211                          X86_CR0_PG)
212         movl    $CR0_STATE, %eax


The one that is missing is NE. I fixed it up.

> +
> +	if (!cpu)
> +		return;

And what happens if don't have this check? Will be bad if do multiple
cr4 writes?

Fyi, this (cr4) should have been a seperate patch. I fixed it up that way.
> +	/*
> +	 * Unlike PV, for pvh xen does not set: PSE PGE OSFXSR OSXMMEXCPT
> +	 * For BSP, PSE PGE will be set in probe_page_size_mask(), for AP
> +	 * set them here. For all, OSFXSR OSXMMEXCPT will be set in fpu_init
> +	 */
> +	if (cpu_has_pse)
> +		set_in_cr4(X86_CR4_PSE);
> +
> +	if (cpu_has_pge)
> +		set_in_cr4(X86_CR4_PGE);
> +}

Seperate patch and since the PGE part is more complicated that just
setting the CR4 - you also have to tweak this:

1512         /* Prevent unwanted bits from being set in PTEs. */                     
1513         __supported_pte_mask &= ~_PAGE_GLOBAL;                                  

I think it should be done once we have actually confirmed that you can
do 2MB pages within the guest. (might need some more tweaking?)

> +
> +/*
> + * Note, that it is refok - because the only caller of this after init

Ah, you based this on a old commit. #linux-next is the one that has
the patches.

> + * is PVH which is not going to use xen_load_gdt_boot or other
> + * __init functions.
> + */
> +void __ref xen_pvh_secondary_vcpu_init(int cpu)
> +{
> +	xen_setup_gdt(cpu);
> +	xen_pvh_set_cr_flags(cpu);
> +}
> +
>  static void __init xen_pvh_early_guest_init(void)
>  {
>  	if (!xen_feature(XENFEAT_auto_translated_physmap))
>  		return;
>  
> -	if (xen_feature(XENFEAT_hvm_callback_vector))
> +	if (xen_feature(XENFEAT_hvm_callback_vector)) {
>  		xen_have_vector_callback = 1;
> +		xen_pvh_set_cr_flags(0);

Why? Why not make it unconditional? Or just change the
code to error out if !hvm_callback_vector?

Anyhow, I changed it in the patch.

> +	}
>  
>  #ifdef CONFIG_X86_32
>  	BUG(); /* PVH: Implement proper support. */
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 5e46190..a18eadd 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -105,7 +105,7 @@ static void cpu_bringup_and_idle(int cpu)
>  #ifdef CONFIG_X86_64
>  	if (xen_feature(XENFEAT_auto_translated_physmap) &&
>  	    xen_feature(XENFEAT_supervisor_mode_kernel))
> -		xen_setup_gdt(cpu);
> +		xen_pvh_secondary_vcpu_init(cpu);
>  #endif
>  	cpu_bringup();
>  	cpu_startup_entry(CPUHP_ONLINE);
> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
> index 9059c24..1cb6f4c 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -123,5 +123,5 @@ __visible void xen_adjust_exception_frame(void);
>  
>  extern int xen_panic_handler_init(void);
>  
> -void xen_setup_gdt(int cpu);
> +void xen_pvh_secondary_vcpu_init(int cpu);
>  #endif /* XEN_OPS_H */
> -- 
> 1.7.2.3
> 


Here is what I am thinking we should have in the tree: