[RFC PATCH 5/16]: PVH xen: supporting changes.

[RFC PATCH 5/16]: PVH xen: supporting changes.

[Mukesh Rathor]

In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
public to be used by PVH. Also put vmx functions like vmr(),
get_instruction_length(), inlined in header file to be used by PVH. No
real code change.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>


diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/hvm/vmx/vmcs.c
--- a/xen/arch/x86/hvm/vmx/vmcs.c	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/arch/x86/hvm/vmx/vmcs.c	Fri Jan 11 16:25:27 2013 -0800
@@ -1170,14 +1170,6 @@ void vmx_do_resume(struct vcpu *v)
     reset_stack_and_jump(vmx_asm_do_vmentry);
 }
 
-static unsigned long vmr(unsigned long field)
-{
-    int rc;
-    unsigned long val;
-    val = __vmread_safe(field, &rc);
-    return rc ? 0 : val;
-}
-
 static void vmx_dump_sel(char *name, uint32_t selector)
 {
     uint32_t sel, attr, limit;
diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Jan 11 16:25:27 2013 -0800
@@ -593,7 +593,7 @@ static int vmx_load_vmcs_ctxt(struct vcp
     return 0;
 }
 
-static void vmx_fpu_enter(struct vcpu *v)
+void vmx_fpu_enter(struct vcpu *v)
 {
     vcpu_restore_fpu_lazy(v);
     v->arch.hvm_vmx.exception_bitmap &= ~(1u << TRAP_no_device);
@@ -1543,19 +1543,7 @@ struct hvm_function_table * __init start
     return &vmx_function_table;
 }
 
-/*
- * Not all cases receive valid value in the VM-exit instruction length field.
- * Callers must know what they're doing!
- */
-static int get_instruction_length(void)
-{
-    int len;
-    len = __vmread(VM_EXIT_INSTRUCTION_LEN); /* Safe: callers audited */
-    BUG_ON((len < 1) || (len > 15));
-    return len;
-}
-
-static void update_guest_eip(void)
+void update_guest_eip(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     unsigned long x;
@@ -1633,8 +1621,8 @@ static void vmx_do_cpuid(struct cpu_user
     regs->edx = edx;
 }
 
-static void vmx_dr_access(unsigned long exit_qualification,
-                          struct cpu_user_regs *regs)
+void vmx_dr_access(unsigned long exit_qualification,
+                   struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
 
@@ -2009,7 +1997,7 @@ gp_fault:
     return X86EMUL_EXCEPTION;
 }
 
-static void vmx_do_extint(struct cpu_user_regs *regs)
+void vmx_do_extint(struct cpu_user_regs *regs)
 {
     unsigned int vector;
 
diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/traps.c
--- a/xen/arch/x86/traps.c	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/arch/x86/traps.c	Fri Jan 11 16:25:27 2013 -0800
@@ -723,7 +723,7 @@ int cpuid_hypervisor_leaves( uint32_t id
     return 1;
 }
 
-static void pv_cpuid(struct cpu_user_regs *regs)
+void pv_cpuid(struct cpu_user_regs *regs)
 {
     uint32_t a, b, c, d;
 
@@ -900,7 +900,7 @@ static int emulate_invalid_rdtscp(struct
     return EXCRET_fault_fixed;
 }
 
-static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
+int emulate_forced_invalid_op(struct cpu_user_regs *regs)
 {
     char sig[5], instr[2];
     unsigned long eip, rc;
diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Fri Jan 11 16:25:27 2013 -0800
@@ -415,6 +415,7 @@ int vmx_add_host_load_msr(u32 msr);
 void vmx_vmcs_switch(struct vmcs_struct *from, struct vmcs_struct *to);
 void vmx_set_eoi_exit_bitmap(struct vcpu *v, u8 vector);
 void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector);
+void vmx_fpu_enter(struct vcpu *v);
 
 #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
 
diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/hvm/vmx/vmx.h
--- a/xen/include/asm-x86/hvm/vmx/vmx.h	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h	Fri Jan 11 16:25:27 2013 -0800
@@ -389,6 +389,26 @@ static inline int __vmxon(u64 addr)
     return rc;
 }
 
+static inline unsigned long vmr(unsigned long field)
+{
+    int rc;
+    unsigned long val;
+    val = __vmread_safe(field, &rc);
+    return rc ? 0 : val;
+}
+
+/*
+ * Not all cases receive valid value in the VM-exit instruction length field.
+ * Callers must know what they're doing!
+ */
+static inline int get_instruction_length(void)
+{
+    int len;
+    len = __vmread(VM_EXIT_INSTRUCTION_LEN); /* Safe: callers audited */
+    BUG_ON((len < 1) || (len > 15));
+    return len;
+}
+
 void vmx_get_segment_register(struct vcpu *, enum x86_segment,
                               struct segment_register *);
 void vmx_inject_extint(int trap);
@@ -397,6 +417,9 @@ void vmx_inject_nmi(void);
 void ept_p2m_init(struct p2m_domain *p2m);
 void ept_walk_table(struct domain *d, unsigned long gfn);
 void setup_ept_dump(void);
+void update_guest_eip(void);
+void vmx_dr_access(unsigned long exit_qualification,struct cpu_user_regs *regs);
+void vmx_do_extint(struct cpu_user_regs *regs);
 
 /* EPT violation qualifications definitions */
 #define _EPT_READ_VIOLATION         0
diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/processor.h
--- a/xen/include/asm-x86/processor.h	Fri Jan 11 16:24:00 2013 -0800
+++ b/xen/include/asm-x86/processor.h	Fri Jan 11 16:25:27 2013 -0800
@@ -551,6 +551,8 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
 void microcode_set_module(unsigned int);
 int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
 int microcode_resume_cpu(int cpu);
+void pv_cpuid(struct cpu_user_regs *regs);
+int emulate_forced_invalid_op(struct cpu_user_regs *regs);
 
 #endif /* !__ASSEMBLY__ */

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Jan Beulich]

>>> On 12.01.13 at 02:48, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
> public to be used by PVH. Also put vmx functions like vmr(),
> get_instruction_length(), inlined in header file to be used by PVH. No
> real code change.

As these functions are VMX-specific, I think they ought to get
renamed to express that. However, the mere fact that you
intend to use VMX functions outside of VMX code looks wrong.

Jan

> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> 
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/hvm/vmx/vmcs.c
> --- a/xen/arch/x86/hvm/vmx/vmcs.c	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c	Fri Jan 11 16:25:27 2013 -0800
> @@ -1170,14 +1170,6 @@ void vmx_do_resume(struct vcpu *v)
>      reset_stack_and_jump(vmx_asm_do_vmentry);
>  }
>  
> -static unsigned long vmr(unsigned long field)
> -{
> -    int rc;
> -    unsigned long val;
> -    val = __vmread_safe(field, &rc);
> -    return rc ? 0 : val;
> -}
> -
>  static void vmx_dump_sel(char *name, uint32_t selector)
>  {
>      uint32_t sel, attr, limit;
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Jan 11 16:25:27 2013 -0800
> @@ -593,7 +593,7 @@ static int vmx_load_vmcs_ctxt(struct vcp
>      return 0;
>  }
>  
> -static void vmx_fpu_enter(struct vcpu *v)
> +void vmx_fpu_enter(struct vcpu *v)
>  {
>      vcpu_restore_fpu_lazy(v);
>      v->arch.hvm_vmx.exception_bitmap &= ~(1u << TRAP_no_device);
> @@ -1543,19 +1543,7 @@ struct hvm_function_table * __init start
>      return &vmx_function_table;
>  }
>  
> -/*
> - * Not all cases receive valid value in the VM-exit instruction length field.
> - * Callers must know what they're doing!
> - */
> -static int get_instruction_length(void)
> -{
> -    int len;
> -    len = __vmread(VM_EXIT_INSTRUCTION_LEN); /* Safe: callers audited */
> -    BUG_ON((len < 1) || (len > 15));
> -    return len;
> -}
> -
> -static void update_guest_eip(void)
> +void update_guest_eip(void)
>  {
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
>      unsigned long x;
> @@ -1633,8 +1621,8 @@ static void vmx_do_cpuid(struct cpu_user
>      regs->edx = edx;
>  }
>  
> -static void vmx_dr_access(unsigned long exit_qualification,
> -                          struct cpu_user_regs *regs)
> +void vmx_dr_access(unsigned long exit_qualification,
> +                   struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
>  
> @@ -2009,7 +1997,7 @@ gp_fault:
>      return X86EMUL_EXCEPTION;
>  }
>  
> -static void vmx_do_extint(struct cpu_user_regs *regs)
> +void vmx_do_extint(struct cpu_user_regs *regs)
>  {
>      unsigned int vector;
>  
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/arch/x86/traps.c
> --- a/xen/arch/x86/traps.c	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/arch/x86/traps.c	Fri Jan 11 16:25:27 2013 -0800
> @@ -723,7 +723,7 @@ int cpuid_hypervisor_leaves( uint32_t id
>      return 1;
>  }
>  
> -static void pv_cpuid(struct cpu_user_regs *regs)
> +void pv_cpuid(struct cpu_user_regs *regs)
>  {
>      uint32_t a, b, c, d;
>  
> @@ -900,7 +900,7 @@ static int emulate_invalid_rdtscp(struct
>      return EXCRET_fault_fixed;
>  }
>  
> -static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
> +int emulate_forced_invalid_op(struct cpu_user_regs *regs)
>  {
>      char sig[5], instr[2];
>      unsigned long eip, rc;
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/hvm/vmx/vmcs.h
> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Fri Jan 11 16:25:27 2013 -0800
> @@ -415,6 +415,7 @@ int vmx_add_host_load_msr(u32 msr);
>  void vmx_vmcs_switch(struct vmcs_struct *from, struct vmcs_struct *to);
>  void vmx_set_eoi_exit_bitmap(struct vcpu *v, u8 vector);
>  void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector);
> +void vmx_fpu_enter(struct vcpu *v);
>  
>  #endif /* ASM_X86_HVM_VMX_VMCS_H__ */
>  
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/hvm/vmx/vmx.h
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h	Fri Jan 11 16:25:27 2013 -0800
> @@ -389,6 +389,26 @@ static inline int __vmxon(u64 addr)
>      return rc;
>  }
>  
> +static inline unsigned long vmr(unsigned long field)
> +{
> +    int rc;
> +    unsigned long val;
> +    val = __vmread_safe(field, &rc);
> +    return rc ? 0 : val;
> +}
> +
> +/*
> + * Not all cases receive valid value in the VM-exit instruction length 
> field.
> + * Callers must know what they're doing!
> + */
> +static inline int get_instruction_length(void)
> +{
> +    int len;
> +    len = __vmread(VM_EXIT_INSTRUCTION_LEN); /* Safe: callers audited */
> +    BUG_ON((len < 1) || (len > 15));
> +    return len;
> +}
> +
>  void vmx_get_segment_register(struct vcpu *, enum x86_segment,
>                                struct segment_register *);
>  void vmx_inject_extint(int trap);
> @@ -397,6 +417,9 @@ void vmx_inject_nmi(void);
>  void ept_p2m_init(struct p2m_domain *p2m);
>  void ept_walk_table(struct domain *d, unsigned long gfn);
>  void setup_ept_dump(void);
> +void update_guest_eip(void);
> +void vmx_dr_access(unsigned long exit_qualification,struct cpu_user_regs 
> *regs);
> +void vmx_do_extint(struct cpu_user_regs *regs);
>  
>  /* EPT violation qualifications definitions */
>  #define _EPT_READ_VIOLATION         0
> diff -r 0339f85f6068 -r fadb3f2f5d97 xen/include/asm-x86/processor.h
> --- a/xen/include/asm-x86/processor.h	Fri Jan 11 16:24:00 2013 -0800
> +++ b/xen/include/asm-x86/processor.h	Fri Jan 11 16:25:27 2013 -0800
> @@ -551,6 +551,8 @@ int wrmsr_hypervisor_regs(uint32_t idx, 
>  void microcode_set_module(unsigned int);
>  int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long 
> len);
>  int microcode_resume_cpu(int cpu);
> +void pv_cpuid(struct cpu_user_regs *regs);
> +int emulate_forced_invalid_op(struct cpu_user_regs *regs);
>  
>  #endif /* !__ASSEMBLY__ */
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Mukesh Rathor]

On Mon, 14 Jan 2013 11:27:15 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 12.01.13 at 02:48, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
> > public to be used by PVH. Also put vmx functions like vmr(),
> > get_instruction_length(), inlined in header file to be used by PVH.
> > No real code change.
> 
> As these functions are VMX-specific, I think they ought to get
> renamed to express that. However, the mere fact that you
> intend to use VMX functions outside of VMX code looks wrong.

Not sure what you are referring to. The functions vmr,
get_instruction_length(), etc..  are called from VMX code, ie,
vmx_pvh.c. I'll rename them with vmx_ prefix.

The two functions, pv_cpuid and emulate_forced_invalid_op are not vmx
specific, they are called from non vmx code, ie, PV also. So renaming
them doesn't make sense IMO. Recall, PVH is a PV guest.

thanks,
Mukesh

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Jan Beulich]

>>> On 16.01.13 at 00:41, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Mon, 14 Jan 2013 11:27:15 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 12.01.13 at 02:48, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>> > In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
>> > public to be used by PVH. Also put vmx functions like vmr(),
>> > get_instruction_length(), inlined in header file to be used by PVH.
>> > No real code change.
>> 
>> As these functions are VMX-specific, I think they ought to get
>> renamed to express that. However, the mere fact that you
>> intend to use VMX functions outside of VMX code looks wrong.
> 
> Not sure what you are referring to. The functions vmr,
> get_instruction_length(), etc..  are called from VMX code, ie,
> vmx_pvh.c. I'll rename them with vmx_ prefix.

That's fine then (perhaps even without the rename if the
declarations sit in a header _only_ included by VMX code).

> The two functions, pv_cpuid and emulate_forced_invalid_op are not vmx
> specific, they are called from non vmx code, ie, PV also. So renaming
> them doesn't make sense IMO. Recall, PVH is a PV guest.

Sure - these ones I didn't have in mind.

Jan

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Tim Deegan]

At 17:48 -0800 on 11 Jan (1357926531), Mukesh Rathor wrote:
> In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
> public to be used by PVH. Also put vmx functions like vmr(),
> get_instruction_length(), inlined in header file to be used by PVH. No
> real code change.

Please don't export vmr().  It silently suppresses errors, which is OK
for its current use (as a helper function in the state dump) but not for
actual guest state manipulations.

Tim.

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Mukesh Rathor]

On Thu, 24 Jan 2013 15:22:21 +0000
Tim Deegan <tim@xen.org> wrote:

> At 17:48 -0800 on 11 Jan (1357926531), Mukesh Rathor wrote:
> > In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
> > public to be used by PVH. Also put vmx functions like vmr(),
> > get_instruction_length(), inlined in header file to be used by PVH.
> > No real code change.
> 
> Please don't export vmr().  It silently suppresses errors, which is OK
> for its current use (as a helper function in the state dump) but not
> for actual guest state manipulations.

Ok, done. I'll assueme it's ok to export get_instruction_length().

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Jan Beulich]

>>> On 25.01.13 at 02:44, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Thu, 24 Jan 2013 15:22:21 +0000
> Tim Deegan <tim@xen.org> wrote:
> 
>> At 17:48 -0800 on 11 Jan (1357926531), Mukesh Rathor wrote:
>> > In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
>> > public to be used by PVH. Also put vmx functions like vmr(),
>> > get_instruction_length(), inlined in header file to be used by PVH.
>> > No real code change.
>> 
>> Please don't export vmr().  It silently suppresses errors, which is OK
>> for its current use (as a helper function in the state dump) but not
>> for actual guest state manipulations.
> 
> Ok, done. I'll assueme it's ok to export get_instruction_length().

With a properly qualified name that's fine I think.

Jan

Re: [RFC PATCH 5/16]: PVH xen: supporting changes.

[Tim Deegan]

At 17:44 -0800 on 24 Jan (1359049481), Mukesh Rathor wrote:
> On Thu, 24 Jan 2013 15:22:21 +0000
> Tim Deegan <tim@xen.org> wrote:
> 
> > At 17:48 -0800 on 11 Jan (1357926531), Mukesh Rathor wrote:
> > > In this patch, we make pv_cpuid() and emulate_forced_invalid_op()
> > > public to be used by PVH. Also put vmx functions like vmr(),
> > > get_instruction_length(), inlined in header file to be used by PVH.
> > > No real code change.
> > 
> > Please don't export vmr().  It silently suppresses errors, which is OK
> > for its current use (as a helper function in the state dump) but not
> > for actual guest state manipulations.
> 
> Ok, done.

Thanks.

> I'll assueme it's ok to export get_instruction_length().

Yes, that's fine as long as the warnings about when it's safe to use
are preserved!  And, as Jan suggested, it should be renamed to
vmx_get_instruction_length().

Cheers,

Tim.