[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH 1/8]: PVH: Basic and preparatory changes



On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:

> ---
>  arch/x86/include/asm/xen/interface.h |    3 +-
>  arch/x86/include/asm/xen/page.h      |    3 ++
>  arch/x86/xen/setup.c                 |   13 ++++++++--
>  arch/x86/xen/smp.c                   |   39 ++++++++++++++++++---------------
>  drivers/xen/cpu_hotplug.c            |    3 +-
>  include/xen/interface/xen.h          |    1 +
>  include/xen/xen.h                    |    4 +++
>  7 files changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/interface.h 
> b/arch/x86/include/asm/xen/interface.h
> index cbf0c9d..1bd5e88 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -136,7 +136,8 @@ struct vcpu_guest_context {
>      struct cpu_user_regs user_regs;         /* User-level CPU registers     
> */
>      struct trap_info trap_ctxt[256];        /* Virtual IDT                  
> */
>      unsigned long ldt_base, ldt_ents;       /* LDT (linear address, # ents) 
> */
> -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) 
> */
> +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents).*
> +                                            * PV in HVM: it's GDTR addr/sz */

I'm not sure I understand this comment. What is "GDTR addr/sz" do you
mean that gdtframes/gdt_ents has a different semantics here?

Might be worthy of a union? Or finding some other way to expand this struct.

> 
>      unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1)   
> */
>      /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
>      unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)  
> */
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index ead8557..936f21d 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -500,10 +500,9 @@ void __cpuinit xen_enable_syscall(void)
>  #endif /* CONFIG_X86_64 */
>  }
>  
> -void __init xen_arch_setup(void)
> +/* Normal PV domain not running in HVM container */

It's a bit of a shame to overload the "HVM" term this way, to mean both
the traditional "providing a full PC like environment" and "PV using
hardware virtualisation facilities".

Perhaps:
        /* Normal PV domain without PVH extensions */

> +static __init void inline xen_non_pvh_arch_setup(void)
>  {
> -       xen_panic_handler_init();
> -
>         HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_4gb_segments);
>         HYPERVISOR_vm_assist(VMASST_CMD_enable, 
> VMASST_TYPE_writable_pagetables);
>  
> @@ -517,6 +516,14 @@ void __init xen_arch_setup(void)
>  
>         xen_enable_sysenter();
>         xen_enable_syscall();
> +}
> +
> +void __init xen_arch_setup(void)
> +{
> +       xen_panic_handler_init();
> +
> +       if (!xen_pvh_domain())
> +               xen_non_pvh_arch_setup();

The negative in the fn name here strikes me as a bit weird. Can't this
just be xen_pv_arch_setup?

Or even just have:
        /* Everything else is specific to PV without hardware support */
        if (xen_pvh_domain())
                return;


>  
>  #ifdef CONFIG_ACPI
>         if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index f58dca7..cdf269d 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu, struct 
> task_struct *idle)
>         gdt = get_cpu_gdt_table(cpu);
>  
>         ctxt->flags = VGCF_IN_KERNEL;
> -       ctxt->user_regs.ds = __USER_DS;
> -       ctxt->user_regs.es = __USER_DS;
>         ctxt->user_regs.ss = __KERNEL_DS;
>  #ifdef CONFIG_X86_32
>         ctxt->user_regs.fs = __KERNEL_PERCPU;
> @@ -314,31 +312,36 @@ cpu_initialize_context(unsigned int cpu, struct 
> task_struct *idle)
>  
>         memset(&ctxt->fpu_ctxt, 0, sizeof(ctxt->fpu_ctxt));
>  
> -       xen_copy_trap_info(ctxt->trap_ctxt);
> +               ctxt->user_regs.ds = __USER_DS;
> +               ctxt->user_regs.es = __USER_DS;
>  
> -       ctxt->ldt_ents = 0;
> +               xen_copy_trap_info(ctxt->trap_ctxt);
>  
> -       BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> +               ctxt->ldt_ents = 0;

Something odd is going on with the indentation here (and below I've just
noticed). I suspect lots of the changes aren't really changing anything
other than whitespace?

> -       gdt_mfn = arbitrary_virt_to_mfn(gdt);
> -       make_lowmem_page_readonly(gdt);
> -       make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
> +               BUG_ON((unsigned long)gdt & ~PAGE_MASK);
>  
> -       ctxt->gdt_frames[0] = gdt_mfn;
> -       ctxt->gdt_ents      = GDT_ENTRIES;
> +               gdt_mfn = arbitrary_virt_to_mfn(gdt);
> +               make_lowmem_page_readonly(gdt);
> +               make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
>  
> -       ctxt->user_regs.cs = __KERNEL_CS;
> -       ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
> +               ctxt->gdt_frames[0] = gdt_mfn;
> +               ctxt->gdt_ents      = GDT_ENTRIES;
>  
> -       ctxt->kernel_ss = __KERNEL_DS;
> -       ctxt->kernel_sp = idle->thread.sp0;
> +               ctxt->kernel_ss = __KERNEL_DS;
> +               ctxt->kernel_sp = idle->thread.sp0;
>  
>  #ifdef CONFIG_X86_32
> -       ctxt->event_callback_cs     = __KERNEL_CS;
> -       ctxt->failsafe_callback_cs  = __KERNEL_CS;
> +               ctxt->event_callback_cs     = __KERNEL_CS;
> +               ctxt->failsafe_callback_cs  = __KERNEL_CS;
>  #endif
> -       ctxt->event_callback_eip    = (unsigned long)xen_hypervisor_callback;
> -       ctxt->failsafe_callback_eip = (unsigned long)xen_failsafe_callback;
> +               ctxt->event_callback_eip    =
> +                                       (unsigned 
> long)xen_hypervisor_callback;
> +               ctxt->failsafe_callback_eip =
> +                                       (unsigned long)xen_failsafe_callback;
> +
> +       ctxt->user_regs.cs = __KERNEL_CS;
> +       ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
>  
>         per_cpu(xen_cr3, cpu) = __pa(swapper_pg_dir);
>         ctxt->ctrlreg[3] = xen_pfn_to_cr3(virt_to_mfn(swapper_pg_dir));

Ian.


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.