Re: [PATCH 09/18] PVH xen: Support privileged op emulation for PVH

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Tue, 25 Jun 2013 10:36:41 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -1524,6 +1528,49 @@ static int read_descriptor(unsigned int sel,
> > --- a/xen/include/asm-x86/system.h
> > +++ b/xen/include/asm-x86/system.h
> > @@ -4,10 +4,20 @@
> >  #include <xen/lib.h>
> >  #include <xen/bitops.h>
> >  
> > -#define read_segment_register(vcpu, regs, name)                 \
> > -({  u16 __sel;                                                  \
> > -    asm volatile ( "movw %%" STR(name) ",%0" : "=r" (__sel) );  \
> > -    __sel;                                                      \
> > +/*
> > + * We need vcpu because during context switch, going from PVH to
> > PV,
> > + * in save_segments(), current has been updated to next, and no
> > longer pointing
> > + * to the PVH.
> > + */
> 
> This is bogus - you shouldn't need any of the {save,load}_segment()
> machinery for PVH, and hence this is not a valid reason for adding a
> vcpu parameter here.

Ok, lets revisit this again since it's been few months already: 

read_segment_register() is called from few places for PVH, and for PVH
it needs to read the value from regs. So it needs to be modified to check
for PVH. Originally, I had started with checking for is_pvh_vcpu(current), 
but that failed quickly because of the context switch call chain:

__context_switch -> ctxt_switch_from --> save_segments -> read_segment_register

In this path, going from PV to PVH, the intention is to save segments for
PV, and since current has already been updated to point to PVH, the check
for current is not correct. Hence, the need for vcpu parameter. I will
enhance my comments in the macro prolog in the next patch version.

Hope that resolves it.

thanks,
Mukesh