Re: [Xen-devel] [PATCH RFC v12 15/21] pvh: Set up more PV stuff in set_info_guest

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> @@ -737,7 +737,31 @@ int arch_set_info_guest(
>      if ( has_hvm_container_vcpu(v) )
>      {
>          hvm_set_info_guest(v);
> -        goto out;
> +
> +        if ( is_hvm_vcpu(v) || v->is_initialised )
> +            goto out;
> +
> +        /* PVH */
> +        if ( c.nat->ctrlreg[1] )
> +            return -EINVAL;
> +
> +        ASSERT(!compat);

This lacks a pvh-32bit-fixme annotation.

> +
> +        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
> +        cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
> +
> +        v->arch.cr3 = page_to_maddr(cr3_page);
> +        v->arch.hvm_vcpu.guest_cr[3] = c.nat->ctrlreg[3];
> +        
> +        if ( paging_mode_enabled(d) )

Is the opposite really possible? I think this ought to be an
assertion.

> +int vmx_pvh_vcpu_boot_set_info(struct vcpu *v,
> +                               struct vcpu_guest_context *ctxtp)
> +{
> +    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
> +         ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
> +         ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
> +         *ctxtp->gdt_frames || ctxtp->gdt_ents ||

Don't know why I didn't spot this earlier, but the gdt_frames check
is pointless when gdt_ents is verified to be zero.

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -150,6 +150,10 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
>  /*
>   * The following is all CPU context. Note that the fpu_ctxt block is filled 
> 
>   * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
> + *
> + * PVH 64bit: In the vcpu boot path, for vmcs context, only gs_base_kernel
> + *            is honored. Other fields like gdt, ldt, and selectors must be
> + *            zeroed. See vmx_pvh_vcpu_boot_set_info.

In the current form I hope the comment is misleading: Surely
general purpose and FPU registers get honored too?

And referring to an internal function from a public header is sort
of bogus too.

>   */
>  struct vcpu_guest_context {
>      /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */

Jan


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