Re: [V11 PATCH 17/21] PVH xen: vmcs related changes

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

On Tue, 27 Aug 2013 18:00:04 +0100
George Dunlap <George.Dunlap@eu.citrix.com> wrote:

> On Sat, Aug 24, 2013 at 1:26 AM, Mukesh Rathor
> <mukesh.rathor@oracle.com> wrote:
> > On Fri, 23 Aug 2013 09:41:55 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >
> >> >>> On 23.08.13 at 03:19, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > This patch contains vmcs changes related for PVH, mainly
> >> > creating a VMCS for PVH guest.
> >> >
> >> > Changes in V11:
> >> >    - Remove pvh_construct_vmcs and make it part of construct_vmcs
> >> >
...
> > Ok. They, the adjustments, are necessary in Phase I.
> >
> >> This is misplaced too - we're past the point of determining the
> >> set of flags, and already in the process of committing them. The
> >> code again should go alongside where the corresponding HVM code
> >> sits.
> >>
> >> > -    if ( cpu_has_vmx_ple )
> >> > +    if ( cpu_has_vmx_ple && !is_pvh_vcpu(v) )
> >> >      {
> >> >          __vmwrite(PLE_GAP, ple_gap);
> >> >          __vmwrite(PLE_WINDOW, ple_window);
> >>
> >> Why would this be conditional upon !PVH?
> >
> > We don't have intercept for PLE right now for PVH in phase I.
> > Easy to add tho.
> 
> Or, you could just use the pre-existing VMX exits.  That's what I've
> done in my port.

My intention was to have this in with some baseline working, then add
each of bells whistles one at a time by testing them. If you are able to 
test it all, or are comfortable checking untested code, go for it.

> >> > @@ -1032,16 +1150,36 @@ static int construct_vmcs(struct vcpu *v)
> >> >
> >> >      v->arch.hvm_vmx.exception_bitmap = HVM_TRAP_MASK
> >> >                | (paging_mode_hap(d) ? 0 : (1U <<
> >> > TRAP_page_fault))
> >> > +              | (is_pvh_vcpu(v) ? (1U << TRAP_int3) | (1U <<
> >> > TRAP_debug) : 0) | (1U << TRAP_no_device);
> >>
> >> What's so special about PVH that it requires these extra
> >> intercepts?
> >
> > HVM does the same in vmx_update_debug_state() called from
> > vmx_update_guest_cr() which we don't call for cr0. We need the
> > two for gdbsx and kdb, and they are harmless too as we inject back
> > into the guest if we don't own the exception, jfyi.
> 
> What you mean is, when an HVM guest sets the cr0 to come out of real
> mode, these will be set from vmx_update_debug_state().  Since PVH
> never goes through that transition, we need to set them at
> start-of-day.  Is that right?
> 
> It seems like it would be better to just call vmx_update_debug_state()
> directly, with a comment about the lack of real-mode transition.

That would work too.

thx,
m-