Re: [Xen-devel] [[RFC PATCH 2/8]: PVH: changes related to initial boot and irq rewiring

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-08-16 at 02:01 +0100, Mukesh Rathor wrote:
> ---
>  arch/x86/xen/enlighten.c |   67 ++++++++++++++++++++++++++++++++++++++-------
>  arch/x86/xen/irq.c       |   22 ++++++++++++++-
>  2 files changed, 77 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index bf4bda6..3a58c51 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -139,6 +139,8 @@ struct tls_descs {
>   */
>  static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
>  
> +static void __init xen_hvm_init_shared_info(void);
> +
>  static void clamp_max_cpus(void)
>  {
>  #ifdef CONFIG_SMP
> @@ -217,8 +219,8 @@ static void __init xen_banner(void)
>       struct xen_extraversion extra;
>       HYPERVISOR_xen_version(XENVER_extraversion, &extra);
>  
> -     printk(KERN_INFO "Booting paravirtualized kernel on %s\n",
> -            pv_info.name);
> +     printk(KERN_INFO "Booting paravirtualized kernel %son %s\n",
> +             (xen_pvh_domain() ? "in HVM " : ""), pv_info.name);

Please can we avoid HVM  in the context of PVH here. "with PVH
extensions" or something.

>       printk(KERN_INFO "Xen version: %d.%d%s%s\n",
>              version >> 16, version & 0xffff, extra.extraversion,
>              xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " 
> (preserve-AD)" : "");
[...]
> @@ -1034,6 +1039,10 @@ static int xen_write_msr_safe(unsigned int msr, 
> unsigned low, unsigned high)
>  
>  void xen_setup_shared_info(void)
>  {
> +     /* do later in xen_pvh_guest_init() when extend_brk is properly setup*/
> +     if (xen_pvh_domain() && xen_initial_domain())
> +             return;

Could we push this setup later for a pv guest too and reduce the
divergence?

> +
>       if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>               set_fixmap(FIX_PARAVIRT_BOOTMAP,
>                          xen_start_info->shared_info);
[...]
> @@ -1274,6 +1287,10 @@ static const struct machine_ops xen_machine_ops 
> __initconst = {
>   */
>  static void __init xen_setup_stackprotector(void)
>  {
> +     if (xen_pvh_domain()) {
> +             switch_to_new_gdt(0);

This seems to skip calling setup_stack_canary_segment too?

Assuming that's not deliberate I'd be tempted to just put "if
(xen_pv_domain())" around the updates of pv_cpus_ops and leave the main
flow of the code the same. If it was deliberate a comment might be in
order.

Unrelated to PVH, so I guess more a question for Konrad, but it seems
odd to me that "struct pv_cpu_ops xen_cpu_ops" starts of with
xen_write_gdt in it, gets overridden to xen_write_gdt_boot temporarily
here and then gets put back to xen_write_gdt immediately. Having the
struct start off with xen_write_gdt_boot in it would seem more natural
to me. Unless the _boot suffix is really supposed to mean
_while_setting_up_stack_protector?

(same for the load_gdt hook, of course).

> +             return;
> +     }
>       pv_cpu_ops.write_gdt_entry = xen_write_gdt_entry_boot;
>       pv_cpu_ops.load_gdt = xen_load_gdt_boot;
>  
> @@ -1284,6 +1301,25 @@ static void __init xen_setup_stackprotector(void)
>       pv_cpu_ops.load_gdt = xen_load_gdt;
>  }
>  
> +static void __init xen_pvh_guest_init(void)
> +{
> +#ifndef __HAVE_ARCH_PTE_SPECIAL
> +     ("__HAVE_ARCH_PTE_SPECIAL is required for PVH for now\n");
> +     #error("__HAVE_ARCH_PTE_SPECIAL is required for PVH\n");
> +#endif

Isn't this an unconditional feature of arch/x86?

And if not then this check belongs in Kconfig.

> +     /* PVH TBD/FIXME: for now just disable this. */
> +     have_vcpu_info_placement = 0;
> +
> +     if (xen_feature(XENFEAT_hvm_callback_vector))
> +             xen_have_vector_callback = 1;
> +
> +        /* for domU, the library sets start_info.shared_info to pfn, but for
> +         * dom0, it contains mfn. we need to get the pfn for shared_info. PVH
> +      * uses HVM code in many places */
> +     if (xen_initial_domain())
> +             xen_hvm_init_shared_info();
> +}
> +
>  /* First C function to be called on Xen boot */
>  asmlinkage void __init xen_start_kernel(void)
>  {
> @@ -1294,15 +1330,23 @@ asmlinkage void __init xen_start_kernel(void)
>       if (!xen_start_info)
>               return;
>  
> +#ifdef CONFIG_X86_32
> +     xen_raw_printk("ERROR: 32bit PV guest can not run in HVM container\n");

"run with PVH extensions"

I think you also want a panic here somewhere instead/as well as the
printk.

Plus I haven't got to it yet but I guess the kernels features are going
to declare something which the tools would use to error out when trying
to build such a thing? Not that this isn't a good sanity check even so.

[...]
> diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
> index 1573376..7c7dfd1 100644
> --- a/arch/x86/xen/irq.c
> +++ b/arch/x86/xen/irq.c
> @@ -100,6 +100,10 @@ PV_CALLEE_SAVE_REGS_THUNK(xen_irq_enable);
>  
>  static void xen_safe_halt(void)
>  {
> +     /* so event channel can be delivered to us, since in HVM container */
> +     if (xen_pvh_domain())
> +             local_irq_enable();
> +
>       /* Blocking includes an implicit local_irq_enable(). */

So this comment isn't true for a PVH guest? Why not? Should it be?

I'm half wondering if we couldn't use native_safe_halt here, IIRC both
SVN and VTd handle "sti; hlt" in a sensible way on the hypervisor side
by calling hvm_hlt

I suppose that's more of a philosophical question about the nature of
PVH ;-)

>       if (HYPERVISOR_sched_op(SCHEDOP_block, NULL) != 0)
>               BUG();

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel