Re: [Xen-devel] RFC: PVH set vcpu info context in vmcs....

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Fri, 16 Aug 2013 08:28:12 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 16.08.13 at 04:26, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >>> wrote:
> > On Thu, 15 Aug 2013 07:31:32 +0100
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > 
> >> >>> On 15.08.13 at 02:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >> >>> wrote:
> >> > On Wed, 14 Aug 2013 10:12:18 +0100
...
> > is_initialised is marked for *each* vcpu upon it being updated, so 
> > second pass around, things are skipped in arch_set_info_guest().
> 
> So what's the point of the of != 0 check then? The intention is for
> this to be a one time thing, i.e. to not be used on already
> initialized CPUs.
> 
> > What we want here is something just for non-boot vcpu. The boot vcpu
> > should fields mostly null in this function since its coming up flat 
> > with default descriptors we loaded. Hence, it will return -EINVAL as
> > the function is.
> 
> But you still didn't explain how you would _incorrectly_ get here for
> the boot CPU _at all_.

Like I said, arch_set_info_guest() is called for all vcpus, boot or
non-boot. arch_set_info_guest() calls pvh_set_vcpu_info() for all vcpus,
boot or nonboot, hence the check for vcpuid. Anyways, with
latest change below, i think it doesn't matter now anyways.

> > The boot vcpu sets up bunch of things for other vcpus, 
> > like gdt, per cpu data areas, etc.. and jump starts them. To jump
> > start secondary vcpus, its sets few things in this function.
> 
> Nor did you explain why a second call, with is_initialized already
> set, would be valid.

pvh_set_vcpu_info() gets called once for each vcpu. is_initialized doesn't
affect it, neither we can use is_initialized to find whether a vcpu
is boot vcpu or not.

> > If I had my way, I'd have this function as follows:
> > 
> > int vmx_pvh_set_vcpu_info(struct vcpu *v, struct vcpu_guest_context
> > *ctxtp) {
> >         vmx_vmcs_enter(v);
> >         __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
> >         vmx_vmcs_exit(v);
> > 
> >         return 0;
> > }
> > 
> > I realize there are other fields in vcpu_guest_context,  but at the 
> > risk of repeating myself, they are all needed for PV, but not for
> > PVH. Yes, they all will need to be used for pvh save/restore, but
> > not here where we are just powering on a secondary vcpu. Perhaps,
> > rename function to something like:
> > vmx_pvh_set_boot_secondary_vcpu_fields()? 
> 
> I think you're heading the completely wrong direction here. The
> function, just like its PV and HVM counterparts, should be
> generically handling both booting and restoring of a guest.

I don't think so :). Like I said in another thread, PVH save/restore
is intended to be done the HVM way. If you are not familiar with HVM
way:
   Boot:  do_vcpu_op->VCPUOP_initialise->arch_set_info_guest().
   Save/Restore:  do_domctl->XEN_DOMCTL_sethvmcontext -> hvm_load/save().

Same for PVH, thus, the function above is called for pvh boot path only,
like it says in the prolog.

> And any fields documented as unused for PVH in _both_ of these
> scenarios should be validated to be zero (to allow eventual later
> use for whatever purpose).

Ok, I'll have to change tools to make sure they are zeroed in the
create path. I can do that. I'll also take that to mean you are ok with 
the above short function with checks for zero...  I'll have it that way in 
the next version. Thank you very much :), I'm glad this is finally resolved.

Mukesh

vmx_pvh_vcpu_boot_set_info(...) (renamed from vmx_pvh_set_vcpu_info())
{
    check all other fields are zero

    vmx_vmcs_enter(v);
    __vmwrite(GUEST_GS_BASE, ctxtp->gs_base_kernel);
    vmx_vmcs_exit(v);
}

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