Xen devel - Sep 2012 - [PATCH v1 3/8]: PVH startup changes (enlighten.c)

---
 arch/x86/xen/enlighten.c |   99 ++++++++++++++++++++++++++++++++++++---------
 1 files changed, 79 insertions(+), 20 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index bf4bda6..98f1798 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -45,6 +45,7 @@
 #include <xen/hvm.h>
 #include <xen/hvc-console.h>
 #include <xen/acpi.h>
+#include <xen/features.h>
 
 #include <asm/paravirt.h>
 #include <asm/apic.h>
@@ -105,6 +106,9 @@ RESERVE_BRK(shared_info_page_brk, PAGE_SIZE);
 __read_mostly int xen_have_vector_callback;
 EXPORT_SYMBOL_GPL(xen_have_vector_callback);
 
+#define xen_pvh_domain() (xen_pv_domain() && \
+			  xen_feature(XENFEAT_auto_translated_physmap) && \
+			  xen_have_vector_callback)
 /*
  * Point at some empty memory to start with. We map the real shared_info
  * page as soon as fixmap is up and running.
@@ -139,6 +143,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 +223,9 @@ 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_feature(XENFEAT_auto_translated_physmap) ?
+			"with PVH extensions " : "", pv_info.name);
 	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)" : "");
@@ -271,12 +278,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		break;
 	}
 
-	asm(XEN_EMULATE_PREFIX "cpuid"
-		: "=a" (*ax),
-		  "=b" (*bx),
-		  "=c" (*cx),
-		  "=d" (*dx)
-		: "0" (*ax), "2" (*cx));
+	if (xen_pvh_domain())
+		native_cpuid(ax, bx, cx, dx);
+	else
+		asm(XEN_EMULATE_PREFIX "cpuid"
+			: "=a" (*ax),
+			"=b" (*bx),
+			"=c" (*cx),
+			"=d" (*dx)
+			: "0" (*ax), "2" (*cx));
 
 	*bx &= maskebx;
 	*cx &= maskecx;
@@ -1034,6 +1044,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;
+
 	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
 		set_fixmap(FIX_PARAVIRT_BOOTMAP,
 			   xen_start_info->shared_info);
@@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
 		HYPERVISOR_shared_info  			(struct shared_info
*)__va(xen_start_info->shared_info);
 
+	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
+	if (xen_pvh_domain())
+		return;
+
 #ifndef CONFIG_SMP
 	/* In UP this is as good a place as any to set up shared info */
 	xen_setup_vcpu_info_placement();
@@ -1274,6 +1292,11 @@ static const struct machine_ops xen_machine_ops
__initconst = {
  */
 static void __init xen_setup_stackprotector(void)
 {
+	/* PVH TBD/FIXME: investigate setup_stack_canary_segment */
+	if (xen_feature(XENFEAT_auto_translated_physmap)) {
+		switch_to_new_gdt(0);
+		return;
+	}
 	pv_cpu_ops.write_gdt_entry = xen_write_gdt_entry_boot;
 	pv_cpu_ops.load_gdt = xen_load_gdt_boot;
 
@@ -1284,6 +1307,31 @@ static void __init xen_setup_stackprotector(void)
 	pv_cpu_ops.load_gdt = xen_load_gdt;
 }
 
+static void __init xen_pvh_guest_init(void)
+{
+	/* PVH TBD/FIXME: for now just disable this. */
+	have_vcpu_info_placement = 0;
+
+        /* 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();
+}
+
+static void __init xen_pvh_early_guest_init(void)
+{
+	if (xen_feature(XENFEAT_hvm_callback_vector))
+		xen_have_vector_callback = 1;
+
+#ifdef CONFIG_X86_32
+	if (xen_feature(XENFEAT_auto_translated_physmap)) {
+		xen_raw_printk("ERROR: 32bit PVH guests are not supported\n");
+		BUG();
+	}
+#endif
+}
+
 /* First C function to be called on Xen boot */
 asmlinkage void __init xen_start_kernel(void)
 {
@@ -1296,13 +1344,18 @@ asmlinkage void __init xen_start_kernel(void)
 
 	xen_domain_type = XEN_PV_DOMAIN;
 
+	xen_setup_features();
+	xen_pvh_early_guest_init();
 	xen_setup_machphys_mapping();
 
 	/* Install Xen paravirt ops */
 	pv_info = xen_info;
 	pv_init_ops = xen_init_ops;
-	pv_cpu_ops = xen_cpu_ops;
 	pv_apic_ops = xen_apic_ops;
+	if (xen_pvh_domain())
+		pv_cpu_ops.cpuid = xen_cpuid;
+	else
+		pv_cpu_ops = xen_cpu_ops;
 
 	x86_init.resources.memory_setup = xen_memory_setup;
 	x86_init.oem.arch_setup = xen_arch_setup;
@@ -1334,8 +1387,6 @@ asmlinkage void __init xen_start_kernel(void)
 	/* Work out if we support NX */
 	x86_configure_nx();
 
-	xen_setup_features();
-
 	/* Get mfn list */
 	if (!xen_feature(XENFEAT_auto_translated_physmap))
 		xen_build_dynamic_phys_to_machine();
@@ -1408,14 +1459,18 @@ asmlinkage void __init xen_start_kernel(void)
 	/* set the limit of our address space */
 	xen_reserve_top();
 
-	/* We used to do this in xen_arch_setup, but that is too late on AMD
-	 * were early_cpu_init (run before ->arch_setup()) calls early_amd_init
-	 * which pokes 0xcf8 port.
-	 */
-	set_iopl.iopl = 1;
-	rc = HYPERVISOR_physdev_op(PHYSDEVOP_set_iopl, &set_iopl);
-	if (rc != 0)
-		xen_raw_printk("physdev_op failed %d\n", rc);
+	/* PVH: runs at default kernel iopl of 0 */
+	if (!xen_pvh_domain()) {
+		/*
+		 * We used to do this in xen_arch_setup, but that is too late
+		 * on AMD were early_cpu_init (run before ->arch_setup()) calls
+		 * early_amd_init which pokes 0xcf8 port.
+		 */
+		set_iopl.iopl = 1;
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_set_iopl, &set_iopl);
+		if (rc != 0)
+			xen_raw_printk("physdev_op failed %d\n", rc);
+	}
 
 #ifdef CONFIG_X86_32
 	/* set up basic CPUID stuff */
@@ -1462,6 +1517,9 @@ asmlinkage void __init xen_start_kernel(void)
 
 	xen_setup_runstate_info(0);
 
+	if (xen_pvh_domain())
+		xen_pvh_guest_init();
+
 	/* Start the world */
 #ifdef CONFIG_X86_32
 	i386_start_kernel();
@@ -1585,7 +1643,8 @@ static struct syscore_ops xen_hvm_syscore_ops = {
 };
 #endif
 
-/* Use a pfn in RAM, may move to MMIO before kexec. */
+/* Use a pfn in RAM, may move to MMIO before kexec.
+ * This function also called for PVH dom0 */
 static void __init xen_hvm_init_shared_info(void)
 {
 	/* Remember pointer for resume */
-- 
1.7.2.3

On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> ---
>  arch/x86/xen/enlighten.c |   99
++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 79 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index bf4bda6..98f1798 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -45,6 +45,7 @@
>  #include <xen/hvm.h>
>  #include <xen/hvc-console.h>
>  #include <xen/acpi.h>
> +#include <xen/features.h>
>  
>  #include <asm/paravirt.h>
>  #include <asm/apic.h>
> @@ -105,6 +106,9 @@ RESERVE_BRK(shared_info_page_brk, PAGE_SIZE);
>  __read_mostly int xen_have_vector_callback;
>  EXPORT_SYMBOL_GPL(xen_have_vector_callback);
>  
> +#define xen_pvh_domain() (xen_pv_domain() && \
> +			  xen_feature(XENFEAT_auto_translated_physmap) && \
> +			  xen_have_vector_callback)
>  /*
>   * Point at some empty memory to start with. We map the real shared_info
>   * page as soon as fixmap is up and running.
> @@ -139,6 +143,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 +223,9 @@ 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_feature(XENFEAT_auto_translated_physmap) ?
> +			"with PVH extensions " : "", pv_info.name);
>  	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)" : "");
> @@ -271,12 +278,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int
*bx,
>  		break;
>  	}
>  
> -	asm(XEN_EMULATE_PREFIX "cpuid"
> -		: "=a" (*ax),
> -		  "=b" (*bx),
> -		  "=c" (*cx),
> -		  "=d" (*dx)
> -		: "0" (*ax), "2" (*cx));
> +	if (xen_pvh_domain())
> +		native_cpuid(ax, bx, cx, dx);
> +	else
> +		asm(XEN_EMULATE_PREFIX "cpuid"
> +			: "=a" (*ax),
> +			"=b" (*bx),
> +			"=c" (*cx),
> +			"=d" (*dx)
> +			: "0" (*ax), "2" (*cx));
can''t we just set the cpuid pvop to native_cpuid?

>  	*bx &= maskebx;
>  	*cx &= maskecx;
> @@ -1034,6 +1044,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*/
Which one is the function that is going to get called later to do the
initialization? It cannot be this one because it would still return
immediately.

> +	if (xen_pvh_domain() && xen_initial_domain())
> +		return;
> +
>  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
>  			   xen_start_info->shared_info);
>
> @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
>  		HYPERVISOR_shared_info >  			(struct shared_info
*)__va(xen_start_info->shared_info);
>  
> +	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
> +	if (xen_pvh_domain())
> +		return;
It seems that if xen_initial_domain we always skip the initialization
while if !xen_initial_domain we only initialize HYPERVISOR_shared_info.
I don''t understand why we have this difference.

Also it might be worth refactoring xen_setup_shared_info moving out the
call to xen_setup_vcpu_info_placement and xen_setup_mfn_list_list in
order to avoid all these

if ([!]xen_pvh_domain())
    return;

On Mon, 24 Sep 2012 13:07:19 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > ---
> >  arch/x86/xen/enlighten.c |   99
> > ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 79
> > insertions(+), 20 deletions(-)
> > 
> > +			"with PVH extensions " : "", pv_info.name);
> >  	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)" : ""); @@ -271,12 +278,15 @@
static void
> > xen_cpuid(unsigned int *ax, unsigned int *bx, break;
> >  	}
> >  
> > -	asm(XEN_EMULATE_PREFIX "cpuid"
> > -		: "=a" (*ax),
> > -		  "=b" (*bx),
> > -		  "=c" (*cx),
> > -		  "=d" (*dx)
> > -		: "0" (*ax), "2" (*cx));
> > +	if (xen_pvh_domain())
> > +		native_cpuid(ax, bx, cx, dx);
> > +	else
> > +		asm(XEN_EMULATE_PREFIX "cpuid"
> > +			: "=a" (*ax),
> > +			"=b" (*bx),
> > +			"=c" (*cx),
> > +			"=d" (*dx)
> > +			: "0" (*ax), "2" (*cx));
> 
> can''t we just set the cpuid pvop to native_cpuid?
We had talked about this a bit at code review. There is some massaging
before and after that goes on, and so we thought we should just leave 
it like that.
> 
> >  	*bx &= maskebx;
> >  	*cx &= maskecx;
> > @@ -1034,6 +1044,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*/
> 
> Which one is the function that is going to get called later to do the
> initialization? It cannot be this one because it would still return
> immediately.
xen_pvh_guest_init() just like the comment says :).

> 
> > +	if (xen_pvh_domain() && xen_initial_domain())
> > +		return;
> > +
> >  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> >  			   xen_start_info->shared_info);
> >
> > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
> >  		HYPERVISOR_shared_info > >  			(struct shared_info
> > *)__va(xen_start_info->shared_info); 
> > +	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
> > +	if (xen_pvh_domain())
> > +		return;
> 
> It seems that if xen_initial_domain we always skip the initialization
> while if !xen_initial_domain we only initialize
> HYPERVISOR_shared_info. I don''t understand why we have this
> difference.
The comment in xen_pvh_guest_init() explains it. For domU the library
maps the pfn at shared_info, ie, shared_info is pfn. For dom0, it''s the
mfn. Dom0 then allocates a pfn via extend_brk, and maps the mfn to it. This
happens in the commond hvm code, xen_hvm_init_shared_info().

On Mon, 24 Sep 2012, Mukesh Rathor wrote:
> On Mon, 24 Sep 2012 13:07:19 +0100
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > > ---
> > >  arch/x86/xen/enlighten.c |   99
> > > ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 79
> > > insertions(+), 20 deletions(-)
> > > 
> > > +			"with PVH extensions " : "",
pv_info.name);
> > >  	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)" : ""); @@ -271,12 +278,15 @@
static void
> > > xen_cpuid(unsigned int *ax, unsigned int *bx, break;
> > >  	}
> > >  
> > > -	asm(XEN_EMULATE_PREFIX "cpuid"
> > > -		: "=a" (*ax),
> > > -		  "=b" (*bx),
> > > -		  "=c" (*cx),
> > > -		  "=d" (*dx)
> > > -		: "0" (*ax), "2" (*cx));
> > > +	if (xen_pvh_domain())
> > > +		native_cpuid(ax, bx, cx, dx);
> > > +	else
> > > +		asm(XEN_EMULATE_PREFIX "cpuid"
> > > +			: "=a" (*ax),
> > > +			"=b" (*bx),
> > > +			"=c" (*cx),
> > > +			"=d" (*dx)
> > > +			: "0" (*ax), "2" (*cx));
> > 
> > can''t we just set the cpuid pvop to native_cpuid?
> 
> We had talked about this a bit at code review. There is some massaging
> before and after that goes on, and so we thought we should just leave 
> it like that.
Ah I see, are you talking about all the masks, correct?

 
> > > +	if (xen_pvh_domain() && xen_initial_domain())
> > > +		return;
> > > +
> > >  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> > >  			   xen_start_info->shared_info);
> > >
> > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
> > >  		HYPERVISOR_shared_info > > >  			(struct shared_info
> > > *)__va(xen_start_info->shared_info); 
> > > +	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
> > > +	if (xen_pvh_domain())
> > > +		return;
> > 
> > It seems that if xen_initial_domain we always skip the initialization
> > while if !xen_initial_domain we only initialize
> > HYPERVISOR_shared_info. I don''t understand why we have this
> > difference.
> 
> The comment in xen_pvh_guest_init() explains it. For domU the library
> maps the pfn at shared_info, ie, shared_info is pfn. For dom0,
it''s the
> mfn. Dom0 then allocates a pfn via extend_brk, and maps the mfn to it. This
> happens in the commond hvm code, xen_hvm_init_shared_info().
This difference is really subtle, it would be nice to get rid of it.
Could Xen allocate a pfn for dom0?
Otherwise could we have the tools allocate an mfn instead of a pfn?
In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly having
a different behavior for xc_dom_feature_translated guests and allocates
pfn instead of an mfn. Maybe we could get rid of that special case: less
code in libxc, a common way of allocating the shared_info page for domU
and dom0 => win.

On Tue, 25 Sep 2012 11:03:13 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 24 Sep 2012, Mukesh Rathor wrote:
> > On Mon, 24 Sep 2012 13:07:19 +0100
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > 
> > > On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > > > +		return;
> > > > +
> > > >  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > > >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> > > >  			   xen_start_info->shared_info);
> > > >
> > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
> > > >  		HYPERVISOR_shared_info > > > >  			(struct
shared_info
> > > > *)__va(xen_start_info->shared_info); 
> > > > +	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
> > > > +	if (xen_pvh_domain())
> > > > +		return;
> > > 
> > > It seems that if xen_initial_domain we always skip the
> > > initialization while if !xen_initial_domain we only initialize
> > > HYPERVISOR_shared_info. I don''t understand why we have
this
> > > difference.
> > 
> > The comment in xen_pvh_guest_init() explains it. For domU the
> > library maps the pfn at shared_info, ie, shared_info is pfn. For
> > dom0, it''s the mfn. Dom0 then allocates a pfn via extend_brk,
and
> > maps the mfn to it. This happens in the commond hvm code,
> > xen_hvm_init_shared_info().
> 
> This difference is really subtle, it would be nice to get rid of it.
> Could Xen allocate a pfn for dom0?
Not easily.
> Otherwise could we have the tools allocate an mfn instead of a pfn?
> In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly
> having a different behavior for xc_dom_feature_translated guests and
> allocates pfn instead of an mfn. Maybe we could get rid of that
> special case: less code in libxc, a common way of allocating the
> shared_info page for domU and dom0 => win.
Wish it was simple. But for PV and PVH, domU, it''s already setup the 
shared page. All we need to do is __va(shared_info). But for HVM domUs 
and PVH dom0, we need to hcall with pfn to get it remapped. Changing the
tool to map pfn, would result in unnecessary hcall for all PV and PVH
domUs. It''s only two lines of code, so lets just leave it.
I''ll make the
comment better.


thx,
Mukesh

On Wed, 26 Sep 2012, Mukesh Rathor wrote:
> On Tue, 25 Sep 2012 11:03:13 +0100
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Mon, 24 Sep 2012, Mukesh Rathor wrote:
> > > On Mon, 24 Sep 2012 13:07:19 +0100
> > > Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> > > 
> > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > > > > +		return;
> > > > > +
> > > > >  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> > > > >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> > > > >  			   xen_start_info->shared_info);
> > > > >
> > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void)
> > > > >  		HYPERVISOR_shared_info > > > > >  		
(struct shared_info
> > > > > *)__va(xen_start_info->shared_info); 
> > > > > +	/* PVH TBD/FIXME: vcpu info placement in phase 2 */
> > > > > +	if (xen_pvh_domain())
> > > > > +		return;
> > > > 
> > > > It seems that if xen_initial_domain we always skip the
> > > > initialization while if !xen_initial_domain we only
initialize
> > > > HYPERVISOR_shared_info. I don''t understand why we
have this
> > > > difference.
> > > 
> > > The comment in xen_pvh_guest_init() explains it. For domU the
> > > library maps the pfn at shared_info, ie, shared_info is pfn. For
> > > dom0, it''s the mfn. Dom0 then allocates a pfn via
extend_brk, and
> > > maps the mfn to it. This happens in the commond hvm code,
> > > xen_hvm_init_shared_info().
> > 
> > This difference is really subtle, it would be nice to get rid of it.
> > Could Xen allocate a pfn for dom0?
> 
> Not easily.
> 
> > Otherwise could we have the tools allocate an mfn instead of a pfn?
> > In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly
> > having a different behavior for xc_dom_feature_translated guests and
> > allocates pfn instead of an mfn. Maybe we could get rid of that
> > special case: less code in libxc, a common way of allocating the
> > shared_info page for domU and dom0 => win.
> 
> Wish it was simple. But for PV and PVH, domU, it''s already setup
the
> shared page. All we need to do is __va(shared_info). But for HVM domUs 
> and PVH dom0, we need to hcall with pfn to get it remapped.
For PVH domU is already setup as a pfn only because in
tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code:

    if ( xc_dom_feature_translated(dom) )
        dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared
info");

and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have:

    xen_pfn_t shinfo         xc_dom_feature_translated(dom) ?
dom->shared_info_pfn : dom->
        shared_info_mfn;

if we simply get rid of the two "if xc_dom_feature_translated(dom)"
wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases
would
remain unmodified, but PVH domU would start getting an mfn for shared_info.
> Changing the
> tool to map pfn, would result in unnecessary hcall for all PV and PVH
> domUs. It''s only two lines of code, so lets just leave it.
I''ll make the
> comment better.
Yes, there would be one more unnecessary hypercall but we would get rid
of 4 "if". I think is a good trade off.

On Wed, 2012-09-26 at 12:33 +0100, Stefano Stabellini
wrote:
> On Wed, 26 Sep 2012, Mukesh Rathor wrote:
> > On Tue, 25 Sep 2012 11:03:13 +0100
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > 
> > > On Mon, 24 Sep 2012, Mukesh Rathor wrote:
> > > > On Mon, 24 Sep 2012 13:07:19 +0100
> > > > Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> > > > 
> > > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote:
> > > > > > +		return;
> > > > > > +
> > > > > >  	if
(!xen_feature(XENFEAT_auto_translated_physmap)) {
> > > > > >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> > > > > >  			   xen_start_info->shared_info);
> > > > > >
> > > > > > @@ -1044,6 +1058,10 @@ void
xen_setup_shared_info(void)
> > > > > >  		HYPERVISOR_shared_info > > > > >
>  			(struct shared_info
> > > > > > *)__va(xen_start_info->shared_info); 
> > > > > > +	/* PVH TBD/FIXME: vcpu info placement in phase 2
*/
> > > > > > +	if (xen_pvh_domain())
> > > > > > +		return;
> > > > > 
> > > > > It seems that if xen_initial_domain we always skip the
> > > > > initialization while if !xen_initial_domain we only
initialize
> > > > > HYPERVISOR_shared_info. I don''t understand why
we have this
> > > > > difference.
> > > > 
> > > > The comment in xen_pvh_guest_init() explains it. For domU
the
> > > > library maps the pfn at shared_info, ie, shared_info is pfn.
For
> > > > dom0, it''s the mfn. Dom0 then allocates a pfn via
extend_brk, and
> > > > maps the mfn to it. This happens in the commond hvm code,
> > > > xen_hvm_init_shared_info().
> > > 
> > > This difference is really subtle, it would be nice to get rid of
it.
> > > Could Xen allocate a pfn for dom0?
> > 
> > Not easily.
> > 
> > > Otherwise could we have the tools allocate an mfn instead of a
pfn?
> > > In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly
> > > having a different behavior for xc_dom_feature_translated guests
and
> > > allocates pfn instead of an mfn. Maybe we could get rid of that
> > > special case: less code in libxc, a common way of allocating the
> > > shared_info page for domU and dom0 => win.
> > 
> > Wish it was simple. But for PV and PVH, domU, it''s already
setup the
> > shared page. All we need to do is __va(shared_info). But for HVM domUs
> > and PVH dom0, we need to hcall with pfn to get it remapped.
> 
> For PVH domU is already setup as a pfn only because in
> tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code:
> 
>     if ( xc_dom_feature_translated(dom) )
>         dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared
info");
> 
> and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have:
> 
>     xen_pfn_t shinfo >         xc_dom_feature_translated(dom) ?
dom->shared_info_pfn : dom->
>         shared_info_mfn;
> 
> if we simply get rid of the two "if
xc_dom_feature_translated(dom)"
> wouldn''t we get an mfn for PVH domU too? AFAICT all the other
cases would
> remain unmodified, but PVH domU would start getting an mfn for shared_info.
The other option would be to have the hypervisor side builder for a PVH
dom0 add an entry to the P2M for the shared info as well. Arguably that
would be more PV like and therefore the correct thing for a PVH guest
since it makes the shared info immediately available to the dom0.
> > Changing the
> > tool to map pfn, would result in unnecessary hcall for all PV and PVH
> > domUs. It''s only two lines of code, so lets just leave it.
I''ll make the
> > comment better.
> 
> Yes, there would be one more unnecessary hypercall but we would get rid
> of 4 "if". I think is a good trade off.
Certainly minimising the numbers of hypercalls is not the most important
thing in a code path which is run once at start of day.

On Wed, 26 Sep 2012 12:33:39 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > Wish it was simple. But for PV and PVH, domU, it''s already
setup
> > the shared page. All we need to do is __va(shared_info). But for
> > HVM domUs and PVH dom0, we need to hcall with pfn to get it
> > remapped.
> 
> For PVH domU is already setup as a pfn only because in
> tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code:
> 
>     if ( xc_dom_feature_translated(dom) )
>         dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared
info");
> 
> and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have:
> 
>     xen_pfn_t shinfo >         xc_dom_feature_translated(dom) ?
dom->shared_info_pfn : dom->
>         shared_info_mfn;
> 
> if we simply get rid of the two "if
xc_dom_feature_translated(dom)"
> wouldn''t we get an mfn for PVH domU too? AFAICT all the other
cases
> would remain unmodified, but PVH domU would start getting an mfn for
> shared_info.
> 
> > Changing the
> > tool to map pfn, would result in unnecessary hcall for all PV and
> > PVH domUs. It''s only two lines of code, so lets just leave
it. I''ll
> > make the comment better.
> 
> Yes, there would be one more unnecessary hypercall but we would get
> rid of 4 "if". I think is a good trade off.
Well, not really unfortunately! There are two fields in the library 
shared_info_mfn and shared_info_pfn in struct xc_dom_image. For
xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto 
xlated also, I end up changing/adding  more code in the library where
it tries to map the shared page. Moreover, it''s better to stick with
the
library assumption that for auto xlated, pfn is set, otherwise,
mfn is set.

Hope that makes sense. I tried it, btw.

thanks,
Mukesh

On Tue, 2 Oct 2012 18:36:19 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Wed, 26 Sep 2012 12:33:39 +0100
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > > Wish it was simple. But for PV and PVH, domU, it''s
already setup
> > > the shared page. All we need to do is __va(shared_info). But for
> > > HVM domUs and PVH dom0, we need to hcall with pfn to get it
> > > remapped.
> > 
> > For PVH domU is already setup as a pfn only because in
> > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code:
> > 
> >     if ( xc_dom_feature_translated(dom) )
> >         dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared
> > info");
> > 
> > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have:
> > 
> >     xen_pfn_t shinfo > >         xc_dom_feature_translated(dom)
? dom->shared_info_pfn :
> > dom-> shared_info_mfn;
> > 
> > if we simply get rid of the two "if
xc_dom_feature_translated(dom)"
> > wouldn''t we get an mfn for PVH domU too? AFAICT all the other
cases
> > would remain unmodified, but PVH domU would start getting an mfn for
> > shared_info.
> > 
> > > Changing the
> > > tool to map pfn, would result in unnecessary hcall for all PV and
> > > PVH domUs. It''s only two lines of code, so lets just
leave it.
> > > I''ll make the comment better.
> > 
> > Yes, there would be one more unnecessary hypercall but we would get
> > rid of 4 "if". I think is a good trade off.
> 
> Well, not really unfortunately! There are two fields in the library 
> shared_info_mfn and shared_info_pfn in struct xc_dom_image. For
> xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto 
> xlated also, I end up changing/adding  more code in the library where
> it tries to map the shared page. Moreover, it''s better to stick
with
> the library assumption that for auto xlated, pfn is set, otherwise,
> mfn is set.
> 
> Hope that makes sense. I tried it, btw.
> 
I''m not able to make a quick change to xen to just put pfn for dom0
also. get_gpfn_from_mfn() is crashing. So for now, how about, lets go
with what I''ve got. I''ll make a note, and when I do xen patch,
I''ll
figure it out, and would be a very small linux patch. I want to get
linux patch in soon before the window closes.

thanks
Mukesh

On Wed, 3 Oct 2012, Mukesh Rathor wrote:
> On Tue, 2 Oct 2012 18:36:19 -0700
> Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> 
> > On Wed, 26 Sep 2012 12:33:39 +0100
> > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > 
> > > > Wish it was simple. But for PV and PVH, domU, it''s
already setup
> > > > the shared page. All we need to do is __va(shared_info). But
for
> > > > HVM domUs and PVH dom0, we need to hcall with pfn to get it
> > > > remapped.
> > > 
> > > For PVH domU is already setup as a pfn only because in
> > > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code:
> > > 
> > >     if ( xc_dom_feature_translated(dom) )
> > >         dom->shared_info_pfn = xc_dom_alloc_page(dom,
"shared
> > > info");
> > > 
> > > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have:
> > > 
> > >     xen_pfn_t shinfo > > >        
xc_dom_feature_translated(dom) ? dom->shared_info_pfn :
> > > dom-> shared_info_mfn;
> > > 
> > > if we simply get rid of the two "if
xc_dom_feature_translated(dom)"
> > > wouldn''t we get an mfn for PVH domU too? AFAICT all the
other cases
> > > would remain unmodified, but PVH domU would start getting an mfn
for
> > > shared_info.
> > > 
> > > > Changing the
> > > > tool to map pfn, would result in unnecessary hcall for all
PV and
> > > > PVH domUs. It''s only two lines of code, so lets
just leave it.
> > > > I''ll make the comment better.
> > > 
> > > Yes, there would be one more unnecessary hypercall but we would
get
> > > rid of 4 "if". I think is a good trade off.
> > 
> > Well, not really unfortunately! There are two fields in the library 
> > shared_info_mfn and shared_info_pfn in struct xc_dom_image. For
> > xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto 
> > xlated also, I end up changing/adding  more code in the library where
> > it tries to map the shared page. Moreover, it''s better to
stick with
> > the library assumption that for auto xlated, pfn is set, otherwise,
> > mfn is set.
> > 
> > Hope that makes sense. I tried it, btw.
> > 
> 
> I''m not able to make a quick change to xen to just put pfn for
dom0
> also. get_gpfn_from_mfn() is crashing.
Did you add the newly allocated shared_info page to dom0''s p2m?
If not, the page doesn''t actually belong to the domain (from the p2m
POV), so get_gpfn_from_mfn might not behave correctly.

> So for now, how about, lets go
> with what I''ve got. I''ll make a note, and when I do xen
patch, I''ll
> figure it out, and would be a very small linux patch. I want to get
> linux patch in soon before the window closes.
We can leave page special as it is for now with very little
consequences, because it is not part of the interface with Xen. However
this is part of the interface, so whatever we choose here is going to be
hard to change later on.

I think it would be good if we could either make dom0 use a pfn or domU
use mfn, whatever is easier for you.

On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini
wrote:
> > So for now, how about, lets go
> > with what I''ve got. I''ll make a note, and when I do
xen patch, I''ll
> > figure it out, and would be a very small linux patch. I want to get
> > linux patch in soon before the window closes.
> 
> We can leave page special as it is for now with very little
> consequences, because it is not part of the interface with Xen. However
> this is part of the interface, so whatever we choose here is going to be
> hard to change later on.
I suggested that until we have seen and reviewed the hypervisor side
patches this guest side functionality all needs to be under an option
which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining
that the ABI is not fixed yet, similar to what we did for the ARM port.

Ian.

On Wed, 3 Oct 2012, Ian Campbell wrote:
> On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:
> > > So for now, how about, lets go
> > > with what I''ve got. I''ll make a note, and when
I do xen patch, I''ll
> > > figure it out, and would be a very small linux patch. I want to
get
> > > linux patch in soon before the window closes.
> > 
> > We can leave page special as it is for now with very little
> > consequences, because it is not part of the interface with Xen.
However
> > this is part of the interface, so whatever we choose here is going to
be
> > hard to change later on.
> 
> I suggested that until we have seen and reviewed the hypervisor side
> patches this guest side functionality all needs to be under an option
> which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining
> that the ABI is not fixed yet, similar to what we did for the ARM port.
OK. In that case we can also keep the pfn/mfn issue as it is and change
it later.

On Wed, 3 Oct 2012 12:58:22 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 3 Oct 2012, Mukesh Rathor wrote:
> > On Tue, 2 Oct 2012 18:36:19 -0700
> > Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > 
> > > On Wed, 26 Sep 2012 12:33:39 +0100
> 
> 
> > So for now, how about, lets go
> > with what I''ve got. I''ll make a note, and when I do
xen patch, I''ll
> > figure it out, and would be a very small linux patch. I want to get
> > linux patch in soon before the window closes.
> 
> We can leave page special as it is for now with very little
> consequences, because it is not part of the interface with Xen.
> However this is part of the interface, so whatever we choose here is
> going to be hard to change later on.
> 
> I think it would be good if we could either make dom0 use a pfn or
> domU use mfn, whatever is easier for you.
Ok, finally, focussing on this, the issue with pfn in dom0 is that
I need pfn allocated in construct_dom0() and be mapped so that the
guest can just do :

HYPERVISOR_shared_info=(struct shared_info
*)__va(xen_start_info->shared_info);

How about following I am experimenting with right now:

in construct_dom0():

    vstartinfo_end   = (vstartinfo_start +
                        sizeof(struct start_info) +
                        sizeof(struct dom0_vga_console_info));

    if ( is_hybrid_domain(d) ) {
        start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start;
        vstartinfo_end   += PAGE_SIZE;
    }

I can then put (PFN: start_info_pfn_addr)->(MFN:
virt_to_maddr(d->shared_info))
in the p2m, and dom0 just has to do __va(), like domU does now.  I
wont'' need
to special case dom0 then.

Do you foresee any problems with this approach?

thanks
Mukesh

On Wed, 2012-10-03 at 23:37 +0100, Mukesh Rathor wrote:
> On Wed, 3 Oct 2012 12:58:22 +0100
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > On Wed, 3 Oct 2012, Mukesh Rathor wrote:
> > > On Tue, 2 Oct 2012 18:36:19 -0700
> > > Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > > 
> > > > On Wed, 26 Sep 2012 12:33:39 +0100
> > 
> > 
> > > So for now, how about, lets go
> > > with what I''ve got. I''ll make a note, and when
I do xen patch, I''ll
> > > figure it out, and would be a very small linux patch. I want to
get
> > > linux patch in soon before the window closes.
> > 
> > We can leave page special as it is for now with very little
> > consequences, because it is not part of the interface with Xen.
> > However this is part of the interface, so whatever we choose here is
> > going to be hard to change later on.
> > 
> > I think it would be good if we could either make dom0 use a pfn or
> > domU use mfn, whatever is easier for you.
> 
> Ok, finally, focussing on this, the issue with pfn in dom0 is that
> I need pfn allocated in construct_dom0() and be mapped so that the
> guest can just do :
> 
> HYPERVISOR_shared_info=(struct shared_info
*)__va(xen_start_info->shared_info);
> 
> How about following I am experimenting with right now:
> 
> in construct_dom0():
> 
>     vstartinfo_end   = (vstartinfo_start +
>                         sizeof(struct start_info) +
>                         sizeof(struct dom0_vga_console_info));
> 
>     if ( is_hybrid_domain(d) ) {
>         start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start;
>         vstartinfo_end   += PAGE_SIZE;
>     }
> 
> I can then put (PFN: start_info_pfn_addr)->(MFN:
virt_to_maddr(d->shared_info))
> in the p2m, and dom0 just has to do __va(), like domU does now.  I
wont'' need
> to special case dom0 then.
> 
> Do you foresee any problems with this approach?
Hard to say without all the surrounding context but it seems plausible
to me.

Ian.

On Thu, 4 Oct 2012 09:38:40 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-10-03 at 23:37 +0100, Mukesh Rathor wrote:
> > On Wed, 3 Oct 2012 12:58:22 +0100
> > Ok, finally, focussing on this, the issue with pfn in dom0 is that
> > I need pfn allocated in construct_dom0() and be mapped so that the
> > guest can just do :
> > 
> > HYPERVISOR_shared_info=(struct shared_info
> > *)__va(xen_start_info->shared_info);
> > 
> > How about following I am experimenting with right now:
> > 
> > in construct_dom0():
> > 
> >     vstartinfo_end   = (vstartinfo_start +
> >                         sizeof(struct start_info) +
> >                         sizeof(struct dom0_vga_console_info));
> > 
> >     if ( is_hybrid_domain(d) ) {
> >         start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start;
> >         vstartinfo_end   += PAGE_SIZE;
> >     }
> > 
> > I can then put (PFN: start_info_pfn_addr)->(MFN:
> > virt_to_maddr(d->shared_info)) in the p2m, and dom0 just has to do
> > __va(), like domU does now.  I wont'' need to special case
dom0 then.
> > 
> > Do you foresee any problems with this approach?
> 
> Hard to say without all the surrounding context but it seems plausible
> to me.

Ok, above works. So no dom0 special case in linux now to map shared_page.

thanks
Mukesh

On Wed, 3 Oct 2012 13:05:14 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:
> > > So for now, how about, lets go
> > > with what I''ve got. I''ll make a note, and when
I do xen patch,
> > > I''ll figure it out, and would be a very small linux
patch. I want
> > > to get linux patch in soon before the window closes.
> > 
> > We can leave page special as it is for now with very little
> > consequences, because it is not part of the interface with Xen.
> > However this is part of the interface, so whatever we choose here
> > is going to be hard to change later on.
> 
> I suggested that until we have seen and reviewed the hypervisor side
> patches this guest side functionality all needs to be under an option
> which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining
> that the ABI is not fixed yet, similar to what we did for the ARM
> port.
Konrad is going to keep the changes in his tree and not upstream for
couple months while we go thru the xen patches. This would be a big
help for me as I won''t have to constantly refresh linux tree.

I hope we can get some baseline in linux and xen soon.

thanks
Mukesh

On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote:
> On Wed, 3 Oct 2012 13:05:14 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:
> > > > So for now, how about, lets go
> > > > with what I''ve got. I''ll make a note, and
when I do xen patch,
> > > > I''ll figure it out, and would be a very small linux
patch. I want
> > > > to get linux patch in soon before the window closes.
> > > 
> > > We can leave page special as it is for now with very little
> > > consequences, because it is not part of the interface with Xen.
> > > However this is part of the interface, so whatever we choose here
> > > is going to be hard to change later on.
> > 
> > I suggested that until we have seen and reviewed the hypervisor side
> > patches this guest side functionality all needs to be under an option
> > which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining
> > that the ABI is not fixed yet, similar to what we did for the ARM
> > port.
> 
> Konrad is going to keep the changes in his tree and not upstream for
> couple months while we go thru the xen patches. This would be a big
> help for me as I won''t have to constantly refresh linux tree.
Ah, ok, that sounds like a good plan.

In the case where the ARM stuff I have built on this becomes ripe first
would it be OK with you guys if I were to cherry pick the relevant bits
of the PVH stuff into a series to go in sooner?
> I hope we can get some baseline in linux and xen soon.
Likewise!

Ian.

On Fri, Oct 05, 2012 at 10:23:25AM +0100, Ian Campbell
wrote:
> On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote:
> > On Wed, 3 Oct 2012 13:05:14 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:
> > > > > So for now, how about, lets go
> > > > > with what I''ve got. I''ll make a note,
and when I do xen patch,
> > > > > I''ll figure it out, and would be a very small
linux patch. I want
> > > > > to get linux patch in soon before the window closes.
> > > > 
> > > > We can leave page special as it is for now with very little
> > > > consequences, because it is not part of the interface with
Xen.
> > > > However this is part of the interface, so whatever we choose
here
> > > > is going to be hard to change later on.
> > > 
> > > I suggested that until we have seen and reviewed the hypervisor
side
> > > patches this guest side functionality all needs to be under an
option
> > > which has CONFIG_EXPERIMENTAL as a dependency and a comment
explaining
> > > that the ABI is not fixed yet, similar to what we did for the ARM
> > > port.
> > 
> > Konrad is going to keep the changes in his tree and not upstream for
> > couple months while we go thru the xen patches. This would be a big
> > help for me as I won''t have to constantly refresh linux tree.
> 
> Ah, ok, that sounds like a good plan.
> 
> In the case where the ARM stuff I have built on this becomes ripe first
> would it be OK with you guys if I were to cherry pick the relevant bits
> of the PVH stuff into a series to go in sooner?
My plan right now was to wait for Mukesh to repost his series (which
I hope will address all our review comments) and then I can shovel
any other patches on top of that.

But if it makes sense to intermix Mukeshes''s patchs, and yours.
(or squash some together) I can do that too.

On Mon, 8 Oct 2012 08:41:02 -0400
Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> On Fri, Oct 05, 2012 at 10:23:25AM +0100, Ian Campbell wrote:
> > On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote:
> > > On Wed, 3 Oct 2012 13:05:14 +0100
> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > 
> > > > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:
> > In the case where the ARM stuff I have built on this becomes ripe
> > first would it be OK with you guys if I were to cherry pick the
> > relevant bits of the PVH stuff into a series to go in sooner?
> 
> My plan right now was to wait for Mukesh to repost his series (which
> I hope will address all our review comments) and then I can shovel
> any other patches on top of that.
> 
> But if it makes sense to intermix Mukeshes''s patchs, and yours.
> (or squash some together) I can do that too.
Ok, I''m all done with changes. Doing final testing now, and will have 
patches sent by Tues afternoon.

thanks,
Mukesh