Re: [PATCH RFC] pvh: clearly specify used parameters in vcpu_guest_context

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Nov 15, 2013 at 05:12:27PM -0800, Mukesh Rathor wrote:
> On Fri, 15 Nov 2013 15:56:52 -0800
> Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> 
> > On Fri, 15 Nov 2013 13:56:54 -0800
> > Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > 
> > > On Fri, 15 Nov 2013 16:45:47 +0000
> > > "Jan Beulich" <JBeulich@suse.com> wrote:
> > > 
> ........
> > > 
> > > We had talked about this while ago, but upon boot, the first thing
> > > a vcpu needs is access to kernel data structure. (A secondary vcpu
> > > is bootstrapped way up into the kernel). It would be possible to
> > > get rid of gs_base_kernel, but will take some work on the linux
> > > side. I can try and test it out, and let you guys know.
> > 
> > Ok, looking at this more, I can hack cpu_bringup_and_idle() in
> > linux to include a static variable for cpuid, which is the least a
> > vcpu needs to know first thing. But, I think that would not work when
> > vcpu hotplug support is added. Another option would be to pass cpuid
> > in one of the registers, say rdi. Thus, rdi == cpuid will be passed
> > to VCPUOP_initialise. In bringup function, the booting vcpu can then
> > load it's own gs based on the cpuid. If linux folks, konrad (CCd), is
> > OK with this, we can remove gs_base_kernel.  Otherwise, it's such a
> > small thing, hopefually it can stay.
> 
> Konrad,
> 
> Here's the changes needed on linux side to remove gs_base_kernel.
> Since, I imagine vcpu hotplug would go thru the same path, it would
> work there also. If this looks OK to everybody, we can remove
> gs_base_kernel from the VCPUOP_initialise call.

I think it is OK.
> 
> thanks,
> Mukesh
> 
> 
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index aa89bbf..54d1022 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -99,14 +99,8 @@ static void cpu_bringup(void)
>  	wmb();			/* make sure everything is out */
>  }
>  
> -/*
> - * The vcpu is coming up with only gs_base_kernel set so we can do 
> - * smp_processor_id() here. We need to set the rest of the context first thing.
> - */
> -static void pvh_set_bringup_context(void)
> +static void pvh_set_bringup_context(int cpu)
>  {
> -	int cpu = smp_processor_id();
> -
>  	load_percpu_segment(cpu);
>  	switch_to_new_gdt(smp_processor_id());
>  
> @@ -121,11 +115,12 @@ static void pvh_set_bringup_context(void)
>  		: : "r" (__KERNEL_DS), "a" (__KERNEL_CS) : "memory");
>  }
>  
> -static void cpu_bringup_and_idle(void)
> +/* Note: cpu parameter is only relevant for PVH */
> +static void cpu_bringup_and_idle(int cpu)
>  {
>  	if (xen_feature(XENFEAT_auto_translated_physmap) &&
>  	    xen_feature(XENFEAT_supervisor_mode_kernel))
> -		pvh_set_bringup_context();
> +		pvh_set_bringup_context(cpu);
>  
>  	cpu_bringup();
>  	cpu_startup_entry(CPUHP_ONLINE);
> @@ -393,8 +388,6 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
>  	/* Note: PVH is not yet supported on x86_32. */
>  	ctxt->user_regs.fs = __KERNEL_PERCPU;
>  	ctxt->user_regs.gs = __KERNEL_STACK_CANARY;
> -#else
> -	ctxt->gs_base_kernel = per_cpu_offset(cpu);
>  #endif
>  	ctxt->user_regs.eip = (unsigned long)cpu_bringup_and_idle;
>  
> @@ -426,6 +419,8 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
>  #ifdef CONFIG_X86_32
>  		ctxt->event_callback_cs     = __KERNEL_CS;
>  		ctxt->failsafe_callback_cs  = __KERNEL_CS;
> +#else
> +		ctxt->gs_base_kernel = per_cpu_offset(cpu);
>  #endif
>  		ctxt->event_callback_eip    =
>  					(unsigned long)xen_hypervisor_callback;
> @@ -433,7 +428,8 @@ cpu_initialize_context(unsigned int cpu, struct task_struct *idle)
>  					(unsigned long)xen_failsafe_callback;
>  		ctxt->user_regs.cs = __KERNEL_CS;
>  		per_cpu(xen_cr3, cpu) = __pa(swapper_pg_dir);
> -	}
> +	} else
> +		ctxt->user_regs.rdi = cpu;
>  
>  	ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
>  	ctxt->ctrlreg[3] = xen_pfn_to_cr3(virt_to_mfn(swapper_pg_dir));
>