Re: [Xen-devel] [V10 PATCH 10/23] PVH xen: domain create, context switch related code changes

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> This patch mostly contains changes to arch/x86/domain.c to allow for a PVH
> domain creation. The new function pvh_set_vcpu_info(), introduced in the
> previous patch, is called here to set some guest context in the VMCS.
> This patch also changes the context_switch code in the same file to follow
> HVM behaviour for PVH.
>
> Changes in V2:
>   - changes to read_segment_register() moved to this patch.
>   - The other comment was to create NULL functions for pvh_set_vcpu_info
>     and pvh_read_descriptor which are implemented in later patch, but since
>     I disable PVH creation until all patches are checked in, it is not needed.
>     But it helps breaking down of patches.
>
> Changes in V3:
>   - Fix read_segment_register() macro to make sure args are evaluated once,
>     and use # instead of STR for name in the macro.
>
> Changes in V4:
>   - Remove pvh substruct in the hvm substruct, as the vcpu_info_mfn has been
>     moved out of pv_vcpu struct.
>   - rename hvm_pvh_* functions to hvm_*.
>
> Changes in V5:
>   - remove pvh_read_descriptor().
>
> Changes in V7:
>   - remove hap_update_cr3() and read_segment_register changes from here.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/arch/x86/domain.c |   56 ++++++++++++++++++++++++++++++++----------------
>  xen/arch/x86/mm.c     |    3 ++
>  2 files changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index c361abf..fccb4ee 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -385,7 +385,7 @@ int vcpu_initialise(struct vcpu *v)
>
>      vmce_init_vcpu(v);
>
> -    if ( is_hvm_domain(d) )
> +    if ( !is_pv_domain(d) )
>      {
>          rc = hvm_vcpu_initialise(v);
>          goto done;
> @@ -452,7 +452,7 @@ void vcpu_destroy(struct vcpu *v)
>
>      vcpu_destroy_fpu(v);
>
> -    if ( is_hvm_vcpu(v) )
> +    if ( !is_pv_vcpu(v) )
>          hvm_vcpu_destroy(v);
>      else
>          xfree(v->arch.pv_vcpu.trap_ctxt);
> @@ -464,7 +464,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>      int rc = -ENOMEM;
>
>      d->arch.hvm_domain.hap_enabled =
> -        is_hvm_domain(d) &&
> +        !is_pv_domain(d) &&
>          hvm_funcs.hap_supported &&
>          (domcr_flags & DOMCRF_hap);
>      d->arch.hvm_domain.mem_sharing_enabled = 0;
> @@ -512,7 +512,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>      mapcache_domain_init(d);
>
>      HYPERVISOR_COMPAT_VIRT_START(d) =
> -        is_hvm_domain(d) ? ~0u : __HYPERVISOR_COMPAT_VIRT_START;
> +        is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
>
>      if ( (rc = paging_domain_init(d, domcr_flags)) != 0 )
>          goto fail;
> @@ -555,7 +555,7 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>      }
>      spin_lock_init(&d->arch.e820_lock);
>
> -    if ( is_hvm_domain(d) )
> +    if ( !is_pv_domain(d) )
>      {
>          if ( (rc = hvm_domain_initialise(d)) != 0 )
>          {
> @@ -651,7 +651,7 @@ int arch_set_info_guest(
>  #define c(fld) (compat ? (c.cmp->fld) : (c.nat->fld))
>      flags = c(flags);
>
> -    if ( !is_hvm_vcpu(v) )
> +    if ( is_pv_vcpu(v) )
>      {
>          if ( !compat )
>          {
> @@ -704,7 +704,7 @@ int arch_set_info_guest(
>      v->fpu_initialised = !!(flags & VGCF_I387_VALID);
>
>      v->arch.flags &= ~TF_kernel_mode;
> -    if ( (flags & VGCF_in_kernel) || is_hvm_vcpu(v)/*???*/ )
> +    if ( (flags & VGCF_in_kernel) || !is_pv_vcpu(v)/*???*/ )
>          v->arch.flags |= TF_kernel_mode;
>
>      v->arch.vgc_flags = flags;
> @@ -719,7 +719,7 @@ int arch_set_info_guest(
>      if ( !compat )
>      {
>          memcpy(&v->arch.user_regs, &c.nat->user_regs, 
> sizeof(c.nat->user_regs));
> -        if ( !is_hvm_vcpu(v) )
> +        if ( is_pv_vcpu(v) )
>              memcpy(v->arch.pv_vcpu.trap_ctxt, c.nat->trap_ctxt,
>                     sizeof(c.nat->trap_ctxt));
>      }
> @@ -735,10 +735,13 @@ int arch_set_info_guest(
>
>      v->arch.user_regs.eflags |= 2;
>
> -    if ( is_hvm_vcpu(v) )
> +    if ( !is_pv_vcpu(v) )
>      {
>          hvm_set_info_guest(v);
> -        goto out;
> +        if ( is_hvm_vcpu(v) || v->is_initialised )
> +            goto out;
> +        else
> +            goto pvh_skip_pv_stuff;
>      }
>
>      init_int80_direct_trap(v);
> @@ -853,6 +856,7 @@ int arch_set_info_guest(
>
>      set_bit(_VPF_in_reset, &v->pause_flags);
>
> + pvh_skip_pv_stuff:

Any idea what this set_bit(_VPF_in_reset) stuff is?  It looks like
it's set above, and cleared down near the bottom of the function if
nothing gets screwed up.

It seems like if that set/clear pair is important, then PVH should do
them both as well, shouldn't it?

 -George

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