Xen devel - Mar 2013 - [PATCH 12/18 V2]:PVH xen: support invalid op, return PVH features etc...

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;
+
     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);
+
     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))
+        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. */
-    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");
+            goto fail;
+        } else
+            printk("PVH guest. Please note it is experimental.
domid:%d\n",
+                   domid);
+    }
 
     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 */
         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

>>> On 16.03.13 at 01:48, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> --- 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;
So how would breakpoint matching on emulated instructions work?
> +
>      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;
Returning "addr" here as well as EXCRET_* values, even if you got
this properly separated right now, looks like setting us up for
future problems.

Jan

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
>

At 17:48 -0700 on 15 Mar (1363369724), Mukesh Rathor
wrote:
> @@ -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;
Ah, I see Jan already commented on this.  Since the function can already
return 0 or 1 there''s no consistent way to tell what the return value
means (and if there were, it would merit a great big comment at the
declaration explaining it).

Tim.

On Mon, 18 Mar 2013 13:48:43 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 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.
> > 
> > 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'' ?
No it can''t.
> > @@ -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?
> 
No. hap doesn''t automatically result in setting of DOMCRF_hvm.

On Mon, 18 Mar 2013 12:27:47 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 16.03.13 at 01:48, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > --- 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;
> 
> So how would breakpoint matching on emulated instructions work?
Should be same as HVM guest, but I''ll add action item to investigate
more.

thanks,
Mukesh