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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.13 at 00:41, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 25 Jun 2013 10:36:41 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 25.06.13 at 02:01, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
>> >>> 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.

No. I already said that {save,load}_segments() ought to be
skipped for PVH, as what it does is already done by VMRESUME/
#VMEXIT. And the function is being passed a vCPU pointer, so
simply making the single call to save_segments() conditional on
is_pv_vcpu(), and converting the !is_hvm_vcpu() around the
call to load_LDT() and load_segments() to is_pv_vcpu() (provided
the LDT handling isn't needed on the same basis) should eliminate
that need.

Furthermore - the reading from struct cpu_user_regs continues
to be bogus (read: at least a latent bug) as long as you don't
always save the selector registers, which you now validly don't
do anymore. You should be consulting the VMCS instead, i.e. go
through hvm_get_segment_register().

Whether a more lightweight variant reading just the selector is
on order I can't immediately tell.

Jan


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