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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Thu, 27 Jun 2013 08:22:42 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> 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>
> >> > + * 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.

They are *not* being called for PVH, where do you see that? Are you 
looking at the right patches? They are called for PV. Again, going from 
PV to PVH in context switch, current will be pointing to PVH and not 
PV when save_segments calls the macro to save segments for PV (not PVH).
Hence, the VCPU is passed to save_segments, and we need to pass to our
famed macro above!

> 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. 

Right,  because you made me move it to the path that calls the macro.
So, for the path that the macro is called, the selectors would have
been read. So, whats the latent bug?

>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.

Well, that would require v == current always, that is not guaranteed
in the macro path. What exactly is the problem the way it is?

-Mukesh

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