Re: [PATCH v14 11/17] pvh: Set up more PV stuff in set_info_guest

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/11/13 12:15, George Dunlap wrote:
> Allow the guest to set up a few more things when bringing up a vcpu.
>
> This includes cr3 and gs_base.
>
> Also set up wallclock, and only initialize a vcpu once.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> v14:
>   - Share more of the codepath, removing a potential bug that might happen
>     if paging functions are called with "is_initialised" set to zero.
>   - Put cr3 in v->arch.guest_table, so the ref counting happens properly.
>     This should fix the "zombie domains" problem.
> v13:
>   - Get rid of separate pvh call, and fold gs_base write into hvm_set_info_guest
>   - Check pvh parameters for validity at the top of arch_set_info_guest
>   - Fix comment about PVH and set_info_guest
>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Keir Fraser <keir@xen.org>
> ---
>   xen/arch/x86/domain.c             |   30 ++++++++++++++++++++++++++++--
>   xen/arch/x86/hvm/vmx/vmx.c        |    7 ++++++-
>   xen/include/asm-x86/hvm/hvm.h     |    6 +++---
>   xen/include/public/arch-x86/xen.h |   11 +++++++++++
>   4 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 8c2a57f..c80ef4c 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -691,6 +691,18 @@ int arch_set_info_guest(
>                (c(ldt_ents) > 8192) )
>               return -EINVAL;
>       }
> +    else if ( is_pvh_vcpu(v) )
> +    {
> +        /* PVH 32bitfixme */
> +        ASSERT(!compat);
> +
> +        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
> +             c(user_regs.cs) || c(user_regs.ss) || c(user_regs.es) ||
> +             c(user_regs.ds) || c(user_regs.fs) || c(user_regs.gs) ||
> +             c.nat->gdt_ents || c.nat->fs_base || c.nat->gs_base_user )
> +            return -EINVAL;
> +
> +    }
>   
>       v->fpu_initialised = !!(flags & VGCF_I387_VALID);
>   
> @@ -728,8 +740,21 @@ int arch_set_info_guest(
>   
>       if ( has_hvm_container_vcpu(v) )
>       {
> -        hvm_set_info_guest(v);
> -        goto out;
> +        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
> +
> +        if ( is_hvm_vcpu(v) || v->is_initialised )
> +            goto out;
> +
> +        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];
> +        v->arch.guest_table = pagetable_from_page(cr3_page);
> +
> +        ASSERT(paging_mode_enabled(d));
> +
> +        goto pvh_skip_pv_stuff;
>       }
>   
>       init_int80_direct_trap(v);
> @@ -934,6 +959,7 @@ int arch_set_info_guest(
>   
>       clear_bit(_VPF_in_reset, &v->pause_flags);
>   
> + pvh_skip_pv_stuff:
>       if ( v->vcpu_id == 0 )
>           update_domain_wallclock_time(d);
>   
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index fdb560e..94e9e21 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1401,7 +1401,7 @@ static void vmx_set_uc_mode(struct vcpu *v)
>       hvm_asid_flush_vcpu(v);
>   }
>   
> -static void vmx_set_info_guest(struct vcpu *v)
> +static void vmx_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
>   {
>       unsigned long intr_shadow;
>   
> @@ -1426,6 +1426,11 @@ static void vmx_set_info_guest(struct vcpu *v)
>           __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
>       }
>   
> +    /* PVH 32bitfixme */
> +    if ( is_pvh_vcpu(v) )
> +        __vmwrite(GUEST_GS_BASE, gs_base_kernel);
> +
> +
>       vmx_vmcs_exit(v);
>   }
>   
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 3376418..d6bfcf2 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -157,7 +157,7 @@ struct hvm_function_table {
>       int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
>       void (*invlpg_intercept)(unsigned long vaddr);
>       void (*set_uc_mode)(struct vcpu *v);
> -    void (*set_info_guest)(struct vcpu *v);
> +    void (*set_info_guest)(struct vcpu *v, uint64_t gs_base_kernel);
>       void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
>   
>       /* Nested HVM */
> @@ -431,10 +431,10 @@ void *hvm_map_guest_frame_rw(unsigned long gfn, bool_t permanent);
>   void *hvm_map_guest_frame_ro(unsigned long gfn, bool_t permanent);
>   void hvm_unmap_guest_frame(void *p, bool_t permanent);
>   
> -static inline void hvm_set_info_guest(struct vcpu *v)
> +static inline void hvm_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
>   {
>       if ( hvm_funcs.set_info_guest )
> -        return hvm_funcs.set_info_guest(v);
> +        return hvm_funcs.set_info_guest(v, gs_base_kernel);
>   }
>   
>   int hvm_debug_op(struct vcpu *v, int32_t op);
> diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
> index 908ef87..42b818e 100644
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -154,6 +154,17 @@ 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.
> + *
> + * Also note that when calling DOMCTL_setvcpucontext and VCPU_initialise
> + * for HVM and PVH guests, not all information in this structure is updated:
> + *
> + * - For HVM guests, the structures read include: fpu_ctxt (if
> + * VGCT_I387_VALID is set), flags, user_regs, debugreg[*]
> + *
> + * - PVH guests are the same as HVM guests, but additionally set cr3,
> + * and for 64-bit guests, gs_base_kernel.  Additionally, the following
> + * entries must be 0: ctrlreg[1], ldt_base, ldg_ents, user_regs.{cs,
> + * ss, es, ds, fs, gs), gdt_ents, fs_base, and gs_base_user.
>    */
>   struct vcpu_guest_context {
>       /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */