Xen devel - Jun 2013 - [PATCH] PVH: vcpu info placement, load selectors, and remove debug printk.

This patch addresses 3 things:
   - Resolve vcpu info placement fixme.
   - Load DS/SS/CS selectors for PVH after switching to new gdt.
   - Remove printk in case of failure to map pnfs in p2m. This because qemu
     has lot of benign failures when mapping HVM pages.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 arch/x86/xen/enlighten.c |   22 ++++++++++++++++++----
 arch/x86/xen/mmu.c       |    3 ---
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index a7ee39f..6ff30d8 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1083,14 +1083,12 @@ 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();
 #endif
+	if (xen_pvh_domain())
+		return;
 
 	xen_setup_mfn_list_list();
 }
@@ -1103,6 +1101,10 @@ void xen_setup_vcpu_info_placement(void)
 	for_each_possible_cpu(cpu)
 		xen_vcpu_setup(cpu);
 
+	/* PVH always uses native IRQ ops */
+	if (xen_pvh_domain())
+		return;
+
 	/* xen_vcpu_setup managed to place the vcpu_info within the
 	   percpu area for all cpus, so make use of it */
 	if (have_vcpu_info_placement) {
@@ -1327,6 +1329,18 @@ 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);
+
+		/* xen started us with null selectors. load them now */
+		__asm__ __volatile__ (
+			"movl %0,%%ds\n"
+			"movl %0,%%ss\n"
+			"pushq %%rax\n"
+			"leaq 1f(%%rip),%%rax\n"
+			"pushq %%rax\n"
+			"retfq\n"
+			"1:\n"
+			: : "r" (__KERNEL_DS), "a" (__KERNEL_CS) :
"memory");
+
 		return;
 	}
 	pv_cpu_ops.write_gdt_entry = xen_write_gdt_entry_boot;
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 31cc1ef..c104895 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -2527,9 +2527,6 @@ static int pvh_add_to_xen_p2m(unsigned long lpfn, unsigned
long fgmfn,
 	set_xen_guest_handle(xatp.errs, &err);
 
 	rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap_range, &xatp);
-	if (rc || err)
-		pr_warn("d0: Failed to map pfn (0x%lx) to mfn (0x%lx) rc:%d:%d\n",
-			lpfn, fgmfn, rc, err);
 	return rc;
 }
 
-- 
1.7.2.3

>>> On 04.06.13 at 02:43, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> @@ -1327,6 +1329,18 @@ 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);
> +
> +		/* xen started us with null selectors. load them now */
> +		__asm__ __volatile__ (
> +			"movl %0,%%ds\n"
> +			"movl %0,%%ss\n"
> +			"pushq %%rax\n"
> +			"leaq 1f(%%rip),%%rax\n"
> +			"pushq %%rax\n"
> +			"retfq\n"
> +			"1:\n"
> +			: : "r" (__KERNEL_DS), "a" (__KERNEL_CS) :
"memory");
> +
I can see why you want CS to be reloaded (and CS, other than the
comment says, clearly hasn''t been holding a null selector up to here.

I can''t immediately see why you''d need SS to be other than
null, and
it completely escapes me why you''d need to DS (but not ES) to be
non-null.

Furthermore, is there any reason why you use "retfq" (Intel syntax)
when all assembly code otherwise uses AT&T syntax (the proper
equivalent here would be "lretq")?

And finally, please consistently use %<number> (which, once
fixed, will make clear that the second constraint really can be "r"),
and avoid using suffixes on moves to/from selector registers
(which, once fixed, will make clear that at least the first constraint
really can be relaxed to "rm").

Jan

On Tue, 04 Jun 2013 09:27:03 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 04.06.13 at 02:43, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -1327,6 +1329,18 @@ 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); +
> > +		/* xen started us with null selectors. load them
> > now */
> > +		__asm__ __volatile__ (
> > +			"movl %0,%%ds\n"
> > +			"movl %0,%%ss\n"
> > +			"pushq %%rax\n"
> > +			"leaq 1f(%%rip),%%rax\n"
> > +			"pushq %%rax\n"
> > +			"retfq\n"
> > +			"1:\n"
> > +			: : "r" (__KERNEL_DS), "a" (__KERNEL_CS) :
> > "memory"); +
> 
> I can see why you want CS to be reloaded (and CS, other than the
> comment says, clearly hasn''t been holding a null selector up to
here.
> 
> I can''t immediately see why you''d need SS to be other
than null, and
> it completely escapes me why you''d need to DS (but not ES) to be
> non-null.
> 
> Furthermore, is there any reason why you use "retfq" (Intel
syntax)
> when all assembly code otherwise uses AT&T syntax (the proper
> equivalent here would be "lretq")?
> 
> And finally, please consistently use %<number> (which, once
> fixed, will make clear that the second constraint really can be
"r"),
> and avoid using suffixes on moves to/from selector registers
> (which, once fixed, will make clear that at least the first constraint
> really can be relaxed to "rm").
Following OK? :

        if (xen_feature(XENFEAT_auto_translated_physmap)) {
                switch_to_new_gdt(0);

                asm volatile (
                        "pushq %%rax\n"
                        "leaq 1f(%%rip),%%rax\n"
                        "pushq %%rax\n"
                        "lretq\n"
                        "1:\n"
                        : : "a" (__KERNEL_CS) : "memory");

                return;
        }

thanks,
Mukesh

>>> On 04.06.13 at 23:53, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> Following OK? :
> 
>         if (xen_feature(XENFEAT_auto_translated_physmap)) {
>                 switch_to_new_gdt(0);
> 
>                 asm volatile (
>                         "pushq %%rax\n"
>                         "leaq 1f(%%rip),%%rax\n"
>                         "pushq %%rax\n"
>                         "lretq\n"
>                         "1:\n"
>                         : : "a" (__KERNEL_CS) :
"memory");
> 
>                 return;
>         }
While generally the choice of using %%rax instead of %0 here is
a matter of taste to some degree, I still don''t see why you
can''t
use "r" as the constraint here in the first place.

Furthermore, assuming this sits in a function guaranteed to not be
inlined, this has a latent bug (and if the assumption isn''t right, the
bug is real) in that the asm() modifies %rax without telling the
compiler.

This is how I would have done it:

		unsigned long dummy;

		asm volatile ("pushq %0\n"
			      "leaq 1f(%%rip),%0\n"
			      "pushq %0\n"
			      "lretq\n"
			      "1:\n"
			      : "=&r" (dummy) : "0" (__KERNEL_CS));

(also dropping the memory clobber, as I don''t see what you
need this for).

Jan

On Wed, 05 Jun 2013 08:03:12 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 04.06.13 at 23:53, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > Following OK? :
> > 
> >         if (xen_feature(XENFEAT_auto_translated_physmap)) {
> >                 switch_to_new_gdt(0);
> > 
> >                 asm volatile (
> >                         "pushq %%rax\n"
> >                         "leaq 1f(%%rip),%%rax\n"
> >                         "pushq %%rax\n"
> >                         "lretq\n"
> >                         "1:\n"
> >                         : : "a" (__KERNEL_CS) :
"memory");
> > 
> >                 return;
> >         }
> 
> While generally the choice of using %%rax instead of %0 here is
> a matter of taste to some degree, I still don''t see why you
can''t
> use "r" as the constraint here in the first place.
The compiler mostly picks eax anyways, but good suggestion.
> Furthermore, assuming this sits in a function guaranteed to not be
> inlined, this has a latent bug (and if the assumption isn''t right,
the
> bug is real) in that the asm() modifies %rax without telling the
> compiler.
According to one of the unofficial asm tutorials i''ve here, the
compiler
knows since it''s input and doesn''t need to be told. In fact
it''ll barf if added to clobber list.
> This is how I would have done it:
> 
> 		unsigned long dummy;
> 
> 		asm volatile ("pushq %0\n"
> 			      "leaq 1f(%%rip),%0\n"
> 			      "pushq %0\n"
> 			      "lretq\n"
> 			      "1:\n"
> 			      : "=&r" (dummy) : "0" (__KERNEL_CS));
> 
Looks good. Thanks,
Mukesh

>>> On 05.06.13 at 21:17, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Wed, 05 Jun 2013 08:03:12 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 04.06.13 at 23:53, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > Following OK? :
>> > 
>> >         if (xen_feature(XENFEAT_auto_translated_physmap)) {
>> >                 switch_to_new_gdt(0);
>> > 
>> >                 asm volatile (
>> >                         "pushq %%rax\n"
>> >                         "leaq 1f(%%rip),%%rax\n"
>> >                         "pushq %%rax\n"
>> >                         "lretq\n"
>> >                         "1:\n"
>> >                         : : "a" (__KERNEL_CS) :
"memory");
>> > 
>> >                 return;
>> >         }
>> 
>> While generally the choice of using %%rax instead of %0 here is
>> a matter of taste to some degree, I still don''t see why you
can''t
>> use "r" as the constraint here in the first place.
> 
> The compiler mostly picks eax anyways, but good suggestion.
> 
>> Furthermore, assuming this sits in a function guaranteed to not be
>> inlined, this has a latent bug (and if the assumption isn''t
right, the
>> bug is real) in that the asm() modifies %rax without telling the
>> compiler.
> 
> According to one of the unofficial asm tutorials i''ve here, the
compiler
> knows since it''s input and doesn''t need to be told. In
fact
> it''ll barf if added to clobber list.
No, if a tutorial says something like this, it''s plain wrong. The
compiler does, as we know, occasionally re-use an unaltered
input in subsequent code (see the hypervisor change
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=84628ee52a427b0f0fe50502eb8ffd0eedad0f03
for a case where this actually caused very bad problems in
practice).

Adding a register to the clobber list that''s among the inputs and/or
outputs of course won''t work, because it being on the clobber list
means the compiler must not use it _at all_ for any of its own
purposes. That''s why you need - in the case here - a dummy output.

Jan