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

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

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
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > 
> >> >>> On 14.08.13 at 04:12, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> >> >>> wrote:
> >> > On Tue, 13 Aug 2013 11:56:36 +0100
> >> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> > 
> > ...
> >> >> >     if ( v->vcpu_id == 0 )
> >> >> >         return 0;
> >> >> 
> >> >> Bogus/pointless.
> >> > 
> >> > No, we don't have anything for the boot vcpu. It's totally
> >> > coming up on the flat address space. For non boot, the vcpu is
> >> > coming up on the kernel GDT. Recall it's a PV guest (coming up
> >> > in an HVM container).
> >> 
> >> No, that's the wrong perspective. You either should never get here
> >> for vCPU 0, or you should refuse this for all already initialized
> >> vCPU-s.
> > 
> > Ok, i'll move the check to caller. FWIW, the vcpu->is_initialised is
> > inapplicable here because this is relevant for non-boot vcpus only.
> 
> That won't make the check any better. Can you please explain in
> detail why this check is necessary, and why the ->is_initialised one
> would be wrong?

is_initialised is marked for *each* vcpu upon it being updated, so 
second pass around, things are skipped in arch_set_info_guest().
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. 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. For PV guest, all 
things need to be sent to the hypervisor, but for PVH the initialization 
can be done in the guest with very minor change, hence I had proposed 
in the past to make this function even more minimal. Actually, all the 
secondary vcpu needs is gs_base_kernel.  Again, we are not really
creating a new guest type, but booting a PV guest in HVM container.
Hence, we look at it from the perspective of what a PV guest and
accomodate PVH extension.

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()? 

thanks much,
Mukesh

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