Re: [Xen-devel] [V10 PATCH 08/23] PVH xen: Introduce PVH guest type and some basic changes.

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> This patch introduces the concept of a pvh guest. There are other basic
> changes like creating macros to check for pv/pvh vcpu/domain, and also
> modifying copy-macros to account for pvh. Finally, guest_kernel_mode is
> changed to boast that a PVH doesn't need to check for TF_kernel_mode
> flag since the kernel runs in ring 0.
>
> Chagnes in V2:
>   - make is_pvh/is_hvm enum instead of adding is_pvh as a new flag.
>   - fix indentation and spacing in guest_kernel_mode macro.
>   - add debug only BUG() in GUEST_KERNEL_RPL macro as it should no longer
>     be called in any PVH paths.
>
> Chagnes in V3:
>   - Rename enum fields, and add is_pv to it.
>   - Get rid if is_hvm_or_pvh_* macros.
>
> Chagnes in V4:
>   - Move e820 fields out of pv_domain struct.
>
> Chagnes in V5:
>   - Move e820 changes above in V4, to a separate patch.
>
> Chagnes in V5:
>   - Rename enum guest_type from is_pv, ... to guest_type_pv, ....
>
> Chagnes in V8:
>   - Got to VMCS for DPL check instead of checking the rpl in
>     guest_kernel_mode.  Note, we drop the const qualifier from
>     vcpu_show_registers() to accomodate the hvm function call in
>     guest_kernel_mode().
>   - Also, hvm_kernel_mode is put in hvm.c because it's called from
>     guest_kernel_mode in regs.h which is a pretty early header include.
>     Hence, we can't place it in hvm.h like other similar functions.
>     The other alternative, to put hvm_kernel_mode in regs.h itself,
>     but then it calls hvm_get_segment_register() for which either we
>     need to include hvm.h in regs.h, not possible, or add
>     proto for hvm_get_segment_register(). But then the args to
>     hvm_get_segment_register() also need their headers. So, in the
>     end this seems to be the best/only way.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
> ---
>  xen/arch/x86/debug.c               |    2 +-
>  xen/arch/x86/hvm/hvm.c             |    8 ++++++++
>  xen/arch/x86/x86_64/traps.c        |    2 +-
>  xen/common/domain.c                |    2 +-
>  xen/include/asm-x86/desc.h         |    4 +++-
>  xen/include/asm-x86/domain.h       |    2 +-
>  xen/include/asm-x86/guest_access.h |   12 ++++++------
>  xen/include/asm-x86/x86_64/regs.h  |   11 +++++++----
>  xen/include/public/domctl.h        |    3 +++
>  xen/include/xen/sched.h            |   21 ++++++++++++++++++---
>  10 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index e67473e..167421d 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -158,7 +158,7 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, 
> struct domain *dp,
>
>          pagecnt = min_t(long, PAGE_SIZE - (addr & ~PAGE_MASK), len);
>
> -        mfn = (dp->is_hvm
> +        mfn = (!is_pv_domain(dp)
>                 ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn)
>                 : dbg_pv_va2mfn(addr, dp, pgd3));
>          if ( mfn == INVALID_MFN )
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8284b3b..bac4708 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4642,6 +4642,14 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>      return hvm_funcs.nhvm_intr_blocked(v);
>  }
>
> +bool_t hvm_kernel_mode(struct vcpu *v)
> +{
> +    struct segment_register seg;
> +
> +    hvm_get_segment_register(v, x86_seg_ss, &seg);
> +    return (seg.attr.fields.dpl == 0);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 9e0571d..feb50ff 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -141,7 +141,7 @@ void show_registers(struct cpu_user_regs *regs)
>      }
>  }
>
> -void vcpu_show_registers(const struct vcpu *v)
> +void vcpu_show_registers(struct vcpu *v)

Rather than doing this (which could potentially mask a bug in which
something actually *does* get changed),  wouldn't it make more sense
to make hvm_kernel_mode (and hvm_get_segment_register) be const?

 -George

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