Re: [PATCH 1/2] pvh: clearly specify used parameters in vcpu_guest_context

["Roger Pau Monné" <roger.pau@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 7367 bytes --]

On 19/11/13 17:53, Jan Beulich wrote:
>>>> On 19.11.13 at 17:42, Roger Pau Monné<roger.pau@citrix.com> wrote:
>> On 19/11/13 16:34, Jan Beulich wrote:
>>>>>> On 19.11.13 at 16:04, Roger Pau Monné<roger.pau@citrix.com> wrote:
>>>> On 19/11/13 14:32, Jan Beulich wrote:
>>>>> Also, by now honoring CR0 and CR4 settings, you again move
>>>>> towards the hybrid model (some fields honored, some refused)
>>>>> that was (I think by you) previously described as unacceptable.
>>>>
>>>> From a strict POV we should just set cr3, flags and user_regs, but then
>>>> Tim mentioned also honouring cr0/cr4,
>>>
>>> I understood his response to mean all fields, or none of them.
>>
>> Trying to make all those fields functional on PVH (or HVM) is quite
>> useless IMHO, it's going to add more code that I doubt anyone is going
>> to use when you can instead use the bare metal functions to set all
>> those things (and from an OS point of view it's also more comfortable
>> because you need less Xen specific stuff).
> 
> That last part I certainly agree to, but that would apply to CR0
> and CR4 just as much.

I've removed the usage of anything that's not strictly necessary in 
order to do AP bringup, so I've removed the setting of debugregs:

---
>From 1fa84464ca8b65860a21e4e3d9ac9646bfe5591b Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@citrix.com>
Date: Thu, 14 Nov 2013 18:07:51 +0100
Subject: [PATCH v2] pvh: clearly specify used parameters in
 vcpu_guest_context
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The aim of this patch is to define a stable way in which PVH is
going to do AP bringup.

Since we are running inside of a HVM container, PVH should only need
to set flags, cr3 and user_regs in order to bring up a vCPU, the rest
can be set once the vCPU is started using the bare metal methods.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/arch/x86/domain.c             |   24 +++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c        |    6 +-----
 xen/include/asm-x86/hvm/hvm.h     |    6 +++---
 xen/include/public/arch-x86/xen.h |    8 +++-----
 4 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a3868f9..aa043a8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -704,10 +704,16 @@ int arch_set_info_guest(
         /* PVH 32bitfixme */
         ASSERT(!compat);
 
-        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
-             c(user_regs.cs) || c(user_regs.ss) || c(user_regs.es) ||
-             c(user_regs.ds) || c(user_regs.fs) || c(user_regs.gs) ||
-             c.nat->gdt_ents || c.nat->fs_base || c.nat->gs_base_user )
+        if ( c(ctrlreg[0]) || c(ctrlreg[1]) || c(ctrlreg[2]) ||
+             c(ctrlreg[4]) || c(ctrlreg[5]) || c(ctrlreg[6]) ||
+             c(ctrlreg[7]) || c(debugreg[0]) || c(debugreg[1]) ||
+             c(debugreg[2]) || c(debugreg[3]) || c(debugreg[4]) ||
+             c(debugreg[5]) || c(debugreg[6]) || c(debugreg[7]) ||
+             c(ldt_base) || c(ldt_ents) || c(user_regs.cs) ||
+             c(user_regs.ss) || c(user_regs.es) || c(user_regs.ds) ||
+             c(user_regs.fs) || c(user_regs.gs) || c(kernel_ss) ||
+             c(kernel_sp) || c.nat->gs_base_kernel || c.nat->gdt_ents ||
+             c.nat->fs_base || c.nat->gs_base_user )
             return -EINVAL;
     }
 
@@ -740,18 +746,10 @@ int arch_set_info_guest(
             XLAT_trap_info(v->arch.pv_vcpu.trap_ctxt + i,
                            c.cmp->trap_ctxt + i);
     }
-    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
-        v->arch.debugreg[i] = c(debugreg[i]);
 
     if ( has_hvm_container_vcpu(v) )
     {
-        /*
-         * NB: TF_kernel_mode is set unconditionally for HVM guests,
-         * so we always use the gs_base_kernel here. If we change this
-         * function to imitate the PV functionality, we'll need to
-         * make it pay attention to the kernel bit.
-         */
-        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
+        hvm_set_info_guest(v);
 
         if ( is_hvm_vcpu(v) || v->is_initialised )
             goto out;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f0132a4..8d923e7 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1467,7 +1467,7 @@ static int vmx_event_pending(struct vcpu *v)
     return intr_info & INTR_INFO_VALID_MASK;
 }
 
-static void vmx_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
+static void vmx_set_info_guest(struct vcpu *v)
 {
     unsigned long intr_shadow;
 
@@ -1492,10 +1492,6 @@ static void vmx_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
         __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
     }
 
-    /* PVH 32bitfixme */
-    if ( is_pvh_vcpu(v) )
-        __vmwrite(GUEST_GS_BASE, gs_base_kernel);
-
     vmx_vmcs_exit(v);
 }
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index a8ba06d..ccca5df 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -160,7 +160,7 @@ struct hvm_function_table {
     int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
     void (*invlpg_intercept)(unsigned long vaddr);
     void (*handle_cd)(struct vcpu *v, unsigned long value);
-    void (*set_info_guest)(struct vcpu *v, uint64_t gs_base_kernel);
+    void (*set_info_guest)(struct vcpu *v);
     void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
 
     /* Nested HVM */
@@ -434,10 +434,10 @@ void *hvm_map_guest_frame_rw(unsigned long gfn, bool_t permanent);
 void *hvm_map_guest_frame_ro(unsigned long gfn, bool_t permanent);
 void hvm_unmap_guest_frame(void *p, bool_t permanent);
 
-static inline void hvm_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
+static inline void hvm_set_info_guest(struct vcpu *v)
 {
     if ( hvm_funcs.set_info_guest )
-        return hvm_funcs.set_info_guest(v, gs_base_kernel);
+        return hvm_funcs.set_info_guest(v);
 }
 
 int hvm_debug_op(struct vcpu *v, int32_t op);
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 5d220ce..8c92308 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -159,12 +159,10 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
  * for HVM and PVH guests, not all information in this structure is updated:
  *
  * - For HVM guests, the structures read include: fpu_ctxt (if
- * VGCT_I387_VALID is set), flags, user_regs, debugreg[*]
+ * VGCT_I387_VALID is set), flags and user_regs.
  *
- * - PVH guests are the same as HVM guests, but additionally set cr3,
- * and for 64-bit guests, gs_base_kernel.  Additionally, the following
- * entries must be 0: ctrlreg[1], ldt_base, ldt_ents, user_regs.{cs,
- * ss, es, ds, fs, gs), gdt_ents, fs_base, and gs_base_user.
+ * - PVH guests are the same as HVM guests, but additionally use ctrlreg[3] to
+ * set cr3. All other fields not used should be set to 0.
  */
 struct vcpu_guest_context {
     /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
-- 
1.7.7.5 (Apple Git-26)


[-- Attachment #2: 0001-pvh-clearly-specify-used-parameters-in-vcpu_guest_co.patch --]
[-- Type: text/plain, Size: 6125 bytes --]

From 1fa84464ca8b65860a21e4e3d9ac9646bfe5591b Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@citrix.com>
Date: Thu, 14 Nov 2013 18:07:51 +0100
Subject: [PATCH v2] pvh: clearly specify used parameters in
 vcpu_guest_context
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The aim of this patch is to define a stable way in which PVH is
going to do AP bringup.

Since we are running inside of a HVM container, PVH should only need
to set flags, cr3 and user_regs in order to bring up a vCPU, the rest
can be set once the vCPU is started using the bare metal methods.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Keir Fraser <keir@xen.org>
---
 xen/arch/x86/domain.c             |   24 +++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c        |    6 +-----
 xen/include/asm-x86/hvm/hvm.h     |    6 +++---
 xen/include/public/arch-x86/xen.h |    8 +++-----
 4 files changed, 18 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a3868f9..aa043a8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -704,10 +704,16 @@ int arch_set_info_guest(
         /* PVH 32bitfixme */
         ASSERT(!compat);
 
-        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
-             c(user_regs.cs) || c(user_regs.ss) || c(user_regs.es) ||
-             c(user_regs.ds) || c(user_regs.fs) || c(user_regs.gs) ||
-             c.nat->gdt_ents || c.nat->fs_base || c.nat->gs_base_user )
+        if ( c(ctrlreg[0]) || c(ctrlreg[1]) || c(ctrlreg[2]) ||
+             c(ctrlreg[4]) || c(ctrlreg[5]) || c(ctrlreg[6]) ||
+             c(ctrlreg[7]) || c(debugreg[0]) || c(debugreg[1]) ||
+             c(debugreg[2]) || c(debugreg[3]) || c(debugreg[4]) ||
+             c(debugreg[5]) || c(debugreg[6]) || c(debugreg[7]) ||
+             c(ldt_base) || c(ldt_ents) || c(user_regs.cs) ||
+             c(user_regs.ss) || c(user_regs.es) || c(user_regs.ds) ||
+             c(user_regs.fs) || c(user_regs.gs) || c(kernel_ss) ||
+             c(kernel_sp) || c.nat->gs_base_kernel || c.nat->gdt_ents ||
+             c.nat->fs_base || c.nat->gs_base_user )
             return -EINVAL;
     }
 
@@ -740,18 +746,10 @@ int arch_set_info_guest(
             XLAT_trap_info(v->arch.pv_vcpu.trap_ctxt + i,
                            c.cmp->trap_ctxt + i);
     }
-    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
-        v->arch.debugreg[i] = c(debugreg[i]);
 
     if ( has_hvm_container_vcpu(v) )
     {
-        /*
-         * NB: TF_kernel_mode is set unconditionally for HVM guests,
-         * so we always use the gs_base_kernel here. If we change this
-         * function to imitate the PV functionality, we'll need to
-         * make it pay attention to the kernel bit.
-         */
-        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
+        hvm_set_info_guest(v);
 
         if ( is_hvm_vcpu(v) || v->is_initialised )
             goto out;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index f0132a4..8d923e7 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1467,7 +1467,7 @@ static int vmx_event_pending(struct vcpu *v)
     return intr_info & INTR_INFO_VALID_MASK;
 }
 
-static void vmx_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
+static void vmx_set_info_guest(struct vcpu *v)
 {
     unsigned long intr_shadow;
 
@@ -1492,10 +1492,6 @@ static void vmx_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
         __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
     }
 
-    /* PVH 32bitfixme */
-    if ( is_pvh_vcpu(v) )
-        __vmwrite(GUEST_GS_BASE, gs_base_kernel);
-
     vmx_vmcs_exit(v);
 }
 
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index a8ba06d..ccca5df 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -160,7 +160,7 @@ struct hvm_function_table {
     int (*msr_write_intercept)(unsigned int msr, uint64_t msr_content);
     void (*invlpg_intercept)(unsigned long vaddr);
     void (*handle_cd)(struct vcpu *v, unsigned long value);
-    void (*set_info_guest)(struct vcpu *v, uint64_t gs_base_kernel);
+    void (*set_info_guest)(struct vcpu *v);
     void (*set_rdtsc_exiting)(struct vcpu *v, bool_t);
 
     /* Nested HVM */
@@ -434,10 +434,10 @@ void *hvm_map_guest_frame_rw(unsigned long gfn, bool_t permanent);
 void *hvm_map_guest_frame_ro(unsigned long gfn, bool_t permanent);
 void hvm_unmap_guest_frame(void *p, bool_t permanent);
 
-static inline void hvm_set_info_guest(struct vcpu *v, uint64_t gs_base_kernel)
+static inline void hvm_set_info_guest(struct vcpu *v)
 {
     if ( hvm_funcs.set_info_guest )
-        return hvm_funcs.set_info_guest(v, gs_base_kernel);
+        return hvm_funcs.set_info_guest(v);
 }
 
 int hvm_debug_op(struct vcpu *v, int32_t op);
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 5d220ce..8c92308 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -159,12 +159,10 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
  * for HVM and PVH guests, not all information in this structure is updated:
  *
  * - For HVM guests, the structures read include: fpu_ctxt (if
- * VGCT_I387_VALID is set), flags, user_regs, debugreg[*]
+ * VGCT_I387_VALID is set), flags and user_regs.
  *
- * - PVH guests are the same as HVM guests, but additionally set cr3,
- * and for 64-bit guests, gs_base_kernel.  Additionally, the following
- * entries must be 0: ctrlreg[1], ldt_base, ldt_ents, user_regs.{cs,
- * ss, es, ds, fs, gs), gdt_ents, fs_base, and gs_base_user.
+ * - PVH guests are the same as HVM guests, but additionally use ctrlreg[3] to
+ * set cr3. All other fields not used should be set to 0.
  */
 struct vcpu_guest_context {
     /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
-- 
1.7.7.5 (Apple Git-26)


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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