Re: [PATCH 9/18 V2]: PVH xen: create PVH vmcs, and initialization

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

On Tue, 19 Mar 2013 09:23:30 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Mon, Mar 18, 2013 at 06:00:36PM -0700, Mukesh Rathor wrote:
> > On Mon, 18 Mar 2013 11:28:43 -0400
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > 
> > > So that sets the Reserved flag. Could you include a comment
> > > explaining why.. Ah, is it b/c we later on bit-shift it and use
> > > it to figure out whether IOPL needs to be virtualized in
> > > arch_set_info_guest? Or is it just b/c this function is based off
> > > hvm_vcpu_initialise? If so, since you are being called by it, can
> > > you skip it?
> > 
> > That resvd bit is required to be set for bootstrap. Set in other
> > places also, like arch_set_info_guest():
> > 
> > v->arch.user_regs.eflags |= 2;
> 
> OK, but why? I am not seeing that bit defined? Or was it needed to get
> the machine to boot up.

It's needed to get the machine to boot up. Thats the EFLAGS default in
the CPU upon reset or power up. It's a resvd bit.

> > > Not sure I follow, why the move of it further down?
> > 
> > params is not defined/allocated for PVH.
> 
> But you are still using it in the code. As in, you are still
> fetching it (just later).

No, not really. The function returns early for PVH before params is
used. params is NULL for PVH:

 int hvm_vcpu_initialise(struct vcpu *v)
 {
     int rc;
     struct domain *d = v->domain;
-    domid_t dm_domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
+    domid_t dm_domid; 
 
     hvm_asid_flush_vcpu(v);
 
+    if ( is_pvh_vcpu(v) )
+        return hvm_pvh_vcpu_initialise(v);    <===============
+
  ...........

> > > > +    /* VMCS controls. */
> > > > +    vmx_pin_based_exec_control &= ~PIN_BASED_VIRTUAL_NMIS;
> > > > +    __vmwrite(PIN_BASED_VM_EXEC_CONTROL,
> > > > vmx_pin_based_exec_control); +
> > > > +    v->arch.hvm_vmx.exec_control = vmx_cpu_based_exec_control;
> > > > +
> > > > +    /* if rdtsc exiting is turned on and it goes thru
> > > > emulate_privileged_op,
> > > > +     * then pv_vcpu.ctrlreg must be added to pvh struct */
> > > 
> > > That would be the 'timer_mode' syntax in the guest config right?
> > > Perhaps then a check at the top of the function to see which
> > > timer_mode is used and exit out with -ENOSYS?
> > 
> > The vtsc setting. We set it to 0 for PVH guests.
> > 
> 
> OK, so that is the the 'timer_mode' always set to '2' or rather
> timer_mode="native" (in the guest config). Which then does
> the xc_domain_set_tsc_info hypercall to set the vtsc.
> You need to document that please.

Right. I set vtsc to 0 in tsc_set_info(), like suggested in prior review,
but may be I should print a warning there that its defaulted to that.
I also had put in cover letter to expand this in Phase II or III or IV... btw.
I'll document this in tsc_set_info() also.

thanks,
Mukesh