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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Wed, 05 Jun 2013 08:03:12 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 04.06.13 at 23:53, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> 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


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