Re: [PATCH 12/18 V2]:PVH xen: support invalid op, return PVH features etc...

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Mar 15, 2013 at 05:48:44PM -0700, Mukesh Rathor wrote:
>  The biggest change in this patch is in traps.c to allow forced invalid
>  op for PVH guest. Also, enable hypercall page init for PVH guest also.
>  Finally, set guest type to PVH if PV with HAP is created.
> 
> Changes in V2:
>   - Fix emulate_forced_invalid_op() to use proper copy function, and inject PF
>     in case it fails.
>   - remove extraneous PVH check in STI/CLI ops en emulate_privileged_op().
>   - Make assert a debug ASSERT in show_registers().
>   - debug.c: keep get_gfn() locked and move put_gfn closer to it.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/debug.c        |    9 ++++-----
>  xen/arch/x86/traps.c        |   43 +++++++++++++++++++++++++++++++++++++------
>  xen/arch/x86/x86_64/traps.c |    5 +++--
>  xen/common/domain.c         |    9 +++++++++
>  xen/common/domctl.c         |    4 ++++
>  xen/common/kernel.c         |    6 +++++-
>  6 files changed, 62 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
> index 502edbc..abe538f 100644
> --- a/xen/arch/x86/debug.c
> +++ b/xen/arch/x86/debug.c
> @@ -59,7 +59,9 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct domain *dp, int toaddr,
>          return INVALID_MFN;
>      }
>  
> -    mfn = mfn_x(get_gfn(dp, *gfn, &gfntype)); 
> +    mfn = mfn_x(get_gfn_query(dp, *gfn, &gfntype)); 
> +    put_gfn(dp, *gfn);
> +
>      if ( p2m_is_readonly(gfntype) && toaddr )
>      {
>          DBGP2("kdb:p2m_is_readonly: gfntype:%x\n", gfntype);
> @@ -158,7 +160,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 = (is_hvm_domain(dp)
> +        mfn = (is_hvm_or_pvh_domain(dp)
>                 ? dbg_hvm_va2mfn(addr, dp, toaddr, &gfn)
>                 : dbg_pv_va2mfn(addr, dp, pgd3));
>          if ( mfn == INVALID_MFN ) 
> @@ -178,9 +180,6 @@ dbg_rw_guest_mem(dbgva_t addr, dbgbyte_t *buf, int len, struct domain *dp,
>          }
>  
>          unmap_domain_page(va);
> -        if ( gfn != INVALID_GFN )
> -            put_gfn(dp, gfn);
> -
>          addr += pagecnt;
>          buf += pagecnt;
>          len -= pagecnt;
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index ab54f82..14656c1 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -459,6 +459,10 @@ static void instruction_done(
>      struct cpu_user_regs *regs, unsigned long eip, unsigned int bpmatch)
>  {
>      regs->eip = eip;
> +
> +    if ( is_pvh_vcpu(current) )
> +        return;

Can it be above the 'regs->eip = eip' ?

> +
>      regs->eflags &= ~X86_EFLAGS_RF;
>      if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
>      {
> @@ -475,6 +479,10 @@ static unsigned int check_guest_io_breakpoint(struct vcpu *v,
>      unsigned int width, i, match = 0;
>      unsigned long start;
>  
> +    if ( is_pvh_vcpu(v) ) {
> +        /* for pvh, ctrlreg field is not implemented/used unless we need to */
> +        return 0;
> +    }
>      if ( !(v->arch.debugreg[5]) ||
>           !(v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>          return 0;
> @@ -908,14 +916,18 @@ static int emulate_invalid_rdtscp(struct cpu_user_regs *regs)
>  unsigned long emulate_forced_invalid_op(struct cpu_user_regs *regs)
>  {
>      char sig[5], instr[2];
> -    unsigned long eip, rc;
> +    unsigned long eip, rc, addr;
>  
>      eip = regs->eip;
>  
>      /* Check for forced emulation signature: ud2 ; .ascii "xen". */
> -    if ( (rc = copy_from_user(sig, (char *)eip, sizeof(sig))) != 0 )
> +    if ( (rc = raw_copy_from_guest(sig, (char *)eip, sizeof(sig))) != 0 )
>      {
> -        propagate_page_fault(eip + sizeof(sig) - rc, 0);
> +        addr = eip + sizeof(sig) - rc;
> +        if ( is_pvh_vcpu(current) )
> +            return addr;
> +
> +        propagate_page_fault(addr, 0);
>          return EXCRET_fault_fixed;
>      }
>      if ( memcmp(sig, "\xf\xbxen", sizeof(sig)) )
> @@ -923,9 +935,13 @@ unsigned long emulate_forced_invalid_op(struct cpu_user_regs *regs)
>      eip += sizeof(sig);
>  
>      /* We only emulate CPUID. */
> -    if ( ( rc = copy_from_user(instr, (char *)eip, sizeof(instr))) != 0 )
> +    if ( ( rc = raw_copy_from_guest(instr, (char *)eip, sizeof(instr))) != 0 )
>      {
> -        propagate_page_fault(eip + sizeof(instr) - rc, 0);
> +        addr = eip + sizeof(instr) - rc;
> +        if ( is_pvh_vcpu(current) )
> +            return addr;
> +
> +        propagate_page_fault(addr, 0);
>          return EXCRET_fault_fixed;
>      }
>      if ( memcmp(instr, "\xf\xa2", sizeof(instr)) )
> @@ -1068,6 +1084,10 @@ void propagate_page_fault(unsigned long addr, u16 error_code)
>      struct vcpu *v = current;
>      struct trap_bounce *tb = &v->arch.pv_vcpu.trap_bounce;
>  
> +    /* PVH should not get here. ctrlreg is not implemented amongst other 
> +     * things */
> +    ASSERT( !is_pvh_vcpu(v) );
> +
>      v->arch.pv_vcpu.ctrlreg[2] = addr;
>      arch_set_cr2(v, addr);
>  
> @@ -1453,6 +1473,9 @@ static int read_descriptor(unsigned int sel,
>  {
>      struct desc_struct desc;
>  
> +    if ( is_pvh_vcpu(v) )
> +        return hvm_pvh_read_descriptor(sel, v, regs, base, limit, ar);

Ah, that is why you are using such weird arguments. And it looks like
emulate_privileged_op really needs it as single variables.

Perhaps then you can just add a comment in hvm_pvh_read_descriptor saying
that the reason you are shifting is b/c the caller (emulate_privileged_op)
expects it to be in this format.

> +
>      if ( !vm86_mode(regs) )
>      {
>          if ( sel < 4)
> @@ -1571,6 +1594,11 @@ static int guest_io_okay(
>      int user_mode = !(v->arch.flags & TF_kernel_mode);
>  #define TOGGLE_MODE() if ( user_mode ) toggle_guest_mode(v)
>  
> +    /* for PVH we check this in vmexit for EXIT_REASON_IO_INSTRUCTION 
> +     * and so don't need to check again here. */
> +    if (is_pvh_vcpu(v))

Odd syntax.

> +        return 1;
> +
>      if ( !vm86_mode(regs) &&
>           (v->arch.pv_vcpu.iopl >= (guest_kernel_mode(v, regs) ? 1 : 3)) )
>          return 1;
> @@ -1816,7 +1844,7 @@ static inline uint64_t guest_misc_enable(uint64_t val)
>          _ptr = (unsigned int)_ptr;                                          \
>      if ( (limit) < sizeof(_x) - 1 || (eip) > (limit) - (sizeof(_x) - 1) )   \
>          goto fail;                                                          \
> -    if ( (_rc = copy_from_user(&_x, (type *)_ptr, sizeof(_x))) != 0 )       \
> +    if ( (_rc = raw_copy_from_guest(&_x, (type *)_ptr, sizeof(_x))) != 0 )  \
>      {                                                                       \
>          propagate_page_fault(_ptr + sizeof(_x) - _rc, 0);                   \
>          goto skip;                                                          \
> @@ -3245,6 +3273,9 @@ void do_device_not_available(struct cpu_user_regs *regs)
>  
>      BUG_ON(!guest_mode(regs));
>  
> +    /* PVH should not get here. ctrlreg is not implemented */
> +    ASSERT( !is_pvh_vcpu(curr) );
> +
>      vcpu_restore_fpu_lazy(curr);
>  
>      if ( curr->arch.pv_vcpu.ctrlreg[0] & X86_CR0_TS )
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index d2f7209..47ec2ff 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -147,7 +147,7 @@ void vcpu_show_registers(const struct vcpu *v)
>      unsigned long crs[8];
>  
>      /* No need to handle HVM for now. */

.. and PVH ..
> -    if ( is_hvm_vcpu(v) )
> +    if ( is_hvm_or_pvh_vcpu(v) )

>          return;
>  
>      crs[0] = v->arch.pv_vcpu.ctrlreg[0];
> @@ -440,6 +440,7 @@ static long register_guest_callback(struct callback_register *reg)
>      long ret = 0;
>      struct vcpu *v = current;
>  
> +    ASSERT( !is_pvh_vcpu(v) );
>      if ( !is_canonical_address(reg->address) )
>          return -EINVAL;
>  
> @@ -620,7 +621,7 @@ static void hypercall_page_initialise_ring3_kernel(void *hypercall_page)
>  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
>  {
>      memset(hypercall_page, 0xCC, PAGE_SIZE);
> -    if ( is_hvm_domain(d) )
> +    if ( is_hvm_or_pvh_domain(d) )
>          hvm_hypercall_page_initialise(d, hypercall_page);
>      else if ( !is_pv_32bit_domain(d) )
>          hypercall_page_initialise_ring3_kernel(hypercall_page);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b6f10b7..aac6699 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -232,6 +232,15 @@ struct domain *domain_create(
>  
>      if ( domcr_flags & DOMCRF_hvm )
>          d->guest_type = hvm_guest;
> +    else if ( domcr_flags & DOMCRF_pvh ) {
> +        d->guest_type = pvh_guest;
> +        if ( !(domcr_flags & DOMCRF_hap) ) {
> +            printk("PVH guest must have HAP on\n");

Ahem. XENLOG_ERR
> +            goto fail;
> +        } else
> +            printk("PVH guest. Please note it is experimental. domid:%d\n",
> +                   domid);

XENLOG_INFO or XENLOG_DEBUG

> +    }
>  
>      if ( domid == 0 )
>      {
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index c98e99c..ab615f1 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -149,6 +149,8 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info)
>  
>      if ( is_hvm_domain(d) )
>          info->flags |= XEN_DOMINF_hvm_guest;
> +    else if ( is_pvh_domain(d) )
> +        info->flags |= XEN_DOMINF_pvh_guest;
>  
>      xsm_security_domaininfo(d, info);
>  
> @@ -400,6 +402,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          domcr_flags = 0;
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hvm_guest )
>              domcr_flags |= DOMCRF_hvm;
> +        else if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
> +            domcr_flags |= DOMCRF_pvh;     /* PV with HAP is a PVH guest */

<scratches his head>

So if the user sets: 'hap' in their guest config we automatically
set domcr_flags to DOMCRF_hvm | DOMCRF_pvh right?

Then in domain_create we do this check:

	if (domcr_flags & DOMCRF_pvh ) {
              d->guest_type = pvh_guest;

and we force an HVM guest with 'hap=1' in the guest config to be
a pvh_guest ?




>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_hap )
>              domcr_flags |= DOMCRF_hap;
>          if ( op->u.createdomain.flags & XEN_DOMCTL_CDF_s3_integrity )
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 72fb905..3bba758 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -289,7 +289,11 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              if ( current->domain == dom0 )
>                  fi.submap |= 1U << XENFEAT_dom0;
>  #ifdef CONFIG_X86
> -            if ( !is_hvm_vcpu(current) )
> +            if ( is_pvh_vcpu(current) )
> +                fi.submap |= (1U << XENFEAT_hvm_safe_pvclock) |
> +                             (1U << XENFEAT_supervisor_mode_kernel) |
> +                             (1U << XENFEAT_hvm_callback_vector);
> +            else if ( !is_hvm_vcpu(current) )
>                  fi.submap |= (1U << XENFEAT_mmu_pt_update_preserve_ad) |
>                               (1U << XENFEAT_highmem_assist) |
>                               (1U << XENFEAT_gnttab_map_avail_bits);
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>