Re: [PATCH 18/18] PVH xen: introduce vmx_pvh.c

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

On Tue, 16 Jul 2013 07:50:49 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 16.07.13 at 04:00, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > On Fri, 28 Jun 2013 10:44:08 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 28.06.13 at 04:28, Mukesh Rathor <mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> > On Tue, 25 Jun 2013 11:49:57 +0100 "Jan Beulich"
> >> > <JBeulich@suse.com> wrote:
> >> >> > +int vmx_pvh_set_vcpu_info(struct vcpu *v, struct
....
> >> >> How does this work without also writing the "hidden" register
> >> >> fields?
> >> > 
> >> > This is for bringing up SMP CPUs by the guest, which already has
> >> > set GDT up so it just needs selectors to be loaded to start the
> >> > target vcpu.
> >> 
> >> That makes no sense to me: Once you VMLAUNCH that vCPU, it'll
> >> get the hidden register fields loaded from the VMCS, without
> >> accessing the GDT. If that understanding of mine is wrong, please
> >> explain how you see things working in more detail.
> > 
> > Re-reading this I realize I misunderstood your question. Sorry. The
> > hidden fields are set to the default values in vmcs create. That
> > call comes from the tool stack during domain creation via the
> > do_domctl ---> vcpu_initialise hcall. That happens during guest
> > creation. Here, the guest is booting and bringing up secondary CPU,
> > and wants to set certain fields via the do_vcpu_op() ->
> > VCPUOP_initialise hcall.
> > 
> > Hope that clarifies.
> 
> Yes, that clarifies that my complaint was right. Either you add a
> comment to the code stating why you don't load these fields with
> the guest specified values (from the GDT/LDT), or you set them
> to the specified values rather than the defaults (or, of course, a
> mixture of both - it may be reasonable to require the descriptors
> referenced by CS, SS, DS, and ES to use defaults, but extending
> this to FS, GS, and LDT would likely be too much of a requirement).
> And even if implying defaults, verifying the descriptors to match
> those requirements at least in debug builds may be a good idea.

I see your point. I had it in the back of my mind that loading a selector
will cause the hidden fields to be loaded, SDM Vol 3A 3.4.3, but I see 
that it doesn't apply to VMCS. I didn't realize this because the default
values are the same as the initial guest GDT. So, options: 

 o Add comment indicating that we don't need to load hidden parts because
   they are same as default values.

      - This makes it linux dependent.

 o Load hidden parts also for any non null CS/SS/DS/ES selector being
   loaded.

      - This is better if the guest GDT doesn't match defaults.
 
 o Don't worry about the selectors in xen, just let the guest load it itself
   first thing. I tested this and works fine. The ABI is flexible at this
   point :).


Please lmk what you prefer. I prefer the last option as I believe the
hypervisor should do the least, but am OK with any options you prefer.

I think this is the last issue from prev version, and I'd be ready to submit
next version of patches as soon as I fix this.

thanks
Mukesh