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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
>> > "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().

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_.

> 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.

> 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.

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).

Jan


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