thr3ads.net - Xen devel - [PATCH 1/2] pvh: proposed BSP/AP bringup changes [Nov 2013]

If this information is useful, please help other people find it:
Share via:

Roger Pau Monne

2013-Nov-19 12:34 UTC

[PATCH 1/2] pvh: proposed BSP/AP bringup changes

This series tries to fix one of the shortcomings of PVH, the lack of a 
stable interface to do AP bringup, that''s patch 1.

While there, I''ve realized that Xen sets a bunch of cr0 and cr4 
flags that don''t seem obviously necessary, so patch 2 tries to fix 
that by just setting the minimal flags needed in order to boot with 
paging enabled.

Roger Pau Monne

2013-Nov-19 12:34 UTC

head link

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

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.
Additionally, the guest can also set cr0 and cr4, and those values
will be appended to the default values set by Xen.

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             |   25 +++++++++++++++++--------
 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, 25 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a3868f9..9b422f4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -51,6 +51,7 @@
 #include <asm/fixmap.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
+#include <asm/hvm/nestedhvm.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/traps.h>
@@ -704,9 +705,13 @@ int arch_set_info_guest(
         /* PVH 32bitfixme */
         ASSERT(!compat);
 
-        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
+        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
+             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v)) ||
+             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
+             c(ctrlreg[6]) || c(ctrlreg[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;
     }
@@ -745,17 +750,21 @@ int arch_set_info_guest(
 
     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;
 
+        if ( c.nat->ctrlreg[0] ) {
+            v->arch.hvm_vcpu.guest_cr[0] |= c.nat->ctrlreg[0];
+            hvm_update_guest_cr(v, 0);
+        }
+
+        if ( c.nat->ctrlreg[4] ) {
+            v->arch.hvm_vcpu.guest_cr[4] |= c.nat->ctrlreg[4];
+            hvm_update_guest_cr(v, 4);
+        }
+
         /* NB: No need to use PV cr3 un-pickling macros */
         cr3_gfn = c(ctrlreg[3]) >> PAGE_SHIFT;
         cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
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..77d1b98 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -161,10 +161,10 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
  * - For HVM guests, the structures read include: fpu_ctxt (if
  * VGCT_I387_VALID is set), flags, user_regs, debugreg[*]
  *
- * - 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[0] to
+ * set cr0, ctrlreg[3] to set cr3 and ctrlreg[4] to set cr4. The values passed
+ * for cr0 and cr4 will be appended to the default cr0 and cr4 values set by
+ * Xen.
  */
 struct vcpu_guest_context {
     /* FPU registers come first so they can be aligned for FXSAVE/FXRSTOR. */
-- 
1.7.7.5 (Apple Git-26)


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

Roger Pau Monne

2013-Nov-19 12:34 UTC

head link

[PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

Right now Xen sets the WP and NE flags on cr0 for PVH, which are not
needed in order to boot with paging enabled. The same happens with
cr4, at least on my system OSFXSR, OSXMMEXCPT and MCE are enabled by
default when there's no need.

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/hvm/vmx/vmcs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 290b42f..33e2544 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -28,7 +28,6 @@
 #include <asm/msr.h>
 #include <asm/xstate.h>
 #include <asm/hvm/hvm.h>
-#include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
@@ -1089,13 +1088,11 @@ static int construct_vmcs(struct vcpu *v)
 
     /* PVH domains always start in paging mode */
     if ( is_pvh_domain(d) )
-        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG | X86_CR0_NE |
X86_CR0_WP;
+        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
 
     hvm_update_guest_cr(v, 0);
 
-    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ?
-        (real_cr4_to_pv_guest_cr4(mmu_cr4_features)
-         & ~HVM_CR4_GUEST_RESERVED_BITS(v)) : 0;
+    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ? X86_CR4_PAE : 0;
     hvm_update_guest_cr(v, 4);
 
     if ( cpu_has_vmx_tpr_shadow )
-- 
1.7.7.5 (Apple Git-26)


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

Jan Beulich

2013-Nov-19 13:32 UTC

head link

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

>>> On 19.11.13 at 13:34, Roger Pau Monne <roger.pau@citrix.com>
wrote:
> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>          /* PVH 32bitfixme */
>          ASSERT(!compat);
>  
> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
> +             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v)) ||
> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
> +             c(ctrlreg[6]) || c(ctrlreg[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;
>      }
Still no checking of debugreg[]?
> @@ -745,17 +750,21 @@ int arch_set_info_guest(
>  
>      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;
>  
> +        if ( c.nat->ctrlreg[0] ) {
Coding style.
> +            v->arch.hvm_vcpu.guest_cr[0] |= c.nat->ctrlreg[0];
Did you really mean |= here? I would expect to simply fail a
request when certain required bits aren''t set.

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.

Jan

Jan Beulich

2013-Nov-19 13:34 UTC

head link

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

>>> On 19.11.13 at 13:34, Roger Pau Monne <roger.pau@citrix.com>
wrote:
> Right now Xen sets the WP and NE flags on cr0 for PVH, which are not
> needed in order to boot with paging enabled. The same happens with
> cr4, at least on my system OSFXSR, OSXMMEXCPT and MCE are enabled by
> default when there's no need.
> 
> 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>
This finally is what I had been asking for from the beginning.
Thanks!

Acked-by: Jan Beulich <jbeulich@suse.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Keir Fraser <keir@xen.org>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 290b42f..33e2544 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -28,7 +28,6 @@
>  #include <asm/msr.h>
>  #include <asm/xstate.h>
>  #include <asm/hvm/hvm.h>
> -#include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/vmx/vmx.h>
> @@ -1089,13 +1088,11 @@ static int construct_vmcs(struct vcpu *v)
>  
>      /* PVH domains always start in paging mode */
>      if ( is_pvh_domain(d) )
> -        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG | X86_CR0_NE | 
> X86_CR0_WP;
> +        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
>  
>      hvm_update_guest_cr(v, 0);
>  
> -    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ?
> -        (real_cr4_to_pv_guest_cr4(mmu_cr4_features)
> -         & ~HVM_CR4_GUEST_RESERVED_BITS(v)) : 0;
> +    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ? X86_CR4_PAE : 0;
>      hvm_update_guest_cr(v, 4);
>  
>      if ( cpu_has_vmx_tpr_shadow )
> -- 
> 1.7.7.5 (Apple Git-26)


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

Roger Pau Monné

2013-Nov-19 15:04 UTC

head link

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

On 19/11/13 14:32, Jan Beulich wrote:>>>> On 19.11.13 at 13:34, Roger Pau Monne
<roger.pau@citrix.com> wrote:
>> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>>          /* PVH 32bitfixme */
>>          ASSERT(!compat);
>>  
>> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
>> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
>> +             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v)) ||
>> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
>> +             c(ctrlreg[6]) || c(ctrlreg[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;
>>      }
> 
> Still no checking of debugreg[]?
Why do we need to check debugreg[]? This code is executed on PVH (and PV
and HVM), and I guessed it does the right thing:

    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
        v->arch.debugreg[i] = c(debugreg[i]);
>> @@ -745,17 +750,21 @@ int arch_set_info_guest(
>>  
>>      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;
>>  
>> +        if ( c.nat->ctrlreg[0] ) {
> 
> Coding style.
Ack.
> 
>> +            v->arch.hvm_vcpu.guest_cr[0] |= c.nat->ctrlreg[0];
> 
> Did you really mean |= here? I would expect to simply fail a
> request when certain required bits aren''t set.
Yes, I wanted to do |= because as described on the public header, flags
specified by the user are appended to PVH mandatory flags. This is kind
of awkward, so I wouldn''t mind making cr0/cr4 mandatory for PVH AP
bringup, so we would have to check that:

cr0 & (X86_CR0_PE | X86_CR0_ET | X86_CR0_PG) == (X86_CR0_PE | X86_CR0_ET
| X86_CR0_PG)

And:

cr4 & X86_CR4_PAE == X86_CR4_PAE
> 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, and I don''t really have a strong
opinion against that. What I think was definitely wrong was only using
gs_base_kernel and not the other gs_* or fs_* fields.

Since we need cr3, using only those and not the other cr* fields seems
strange.

Jan Beulich

2013-Nov-19 15:34 UTC

head link

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

>>> On 19.11.13 at 16:04, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 19/11/13 14:32, Jan Beulich wrote:
>>>>> On 19.11.13 at 13:34, Roger Pau Monne
<roger.pau@citrix.com> wrote:
>>> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>>>          /* PVH 32bitfixme */
>>>          ASSERT(!compat);
>>>  
>>> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
>>> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS) ||
>>> +             (c(ctrlreg[4]) & HVM_CR4_GUEST_RESERVED_BITS(v))
||
>>> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5]) ||
>>> +             c(ctrlreg[6]) || c(ctrlreg[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;
>>>      }
>> 
>> Still no checking of debugreg[]?
> 
> Why do we need to check debugreg[]? This code is executed on PVH (and PV
> and HVM), and I guessed it does the right thing:
> 
>     for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
>         v->arch.debugreg[i] = c(debugreg[i]);
If it does - fine; I didn't verify whether that would actually result in
the debug registers getting properly loaded with the requested
values.
>>> +            v->arch.hvm_vcpu.guest_cr[0] |=
c.nat->ctrlreg[0];
>> 
>> Did you really mean |= here? I would expect to simply fail a
>> request when certain required bits aren't set.
> 
> Yes, I wanted to do |= because as described on the public header, flags
> specified by the user are appended to PVH mandatory flags. This is kind
> of awkward, so I wouldn't mind making cr0/cr4 mandatory for PVH AP
> bringup, so we would have to check that:
> 
> cr0 & (X86_CR0_PE | X86_CR0_ET | X86_CR0_PG) == (X86_CR0_PE |
X86_CR0_ET
> | X86_CR0_PG)
Except that I'm not sure the ET check is really needed.
> And:
> 
> cr4 & X86_CR4_PAE == X86_CR4_PAE
> 
>> 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.
> and I don't really have a strong
> opinion against that. What I think was definitely wrong was only using
> gs_base_kernel and not the other gs_* or fs_* fields.
> 
> Since we need cr3, using only those and not the other cr* fields seems
> strange.
Hmm, I think it's going to remain looking strange as long as some
of the fields are used and some are not (of course ignoring those
that are PV specific).

Jan

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

Roger Pau Monné

2013-Nov-19 16:42 UTC

head link

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

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:
>>>>>> On 19.11.13 at 13:34, Roger Pau Monne
<roger.pau@citrix.com> wrote:
>>>> @@ -704,9 +705,13 @@ int arch_set_info_guest(
>>>>          /* PVH 32bitfixme */
>>>>          ASSERT(!compat);
>>>>  
>>>> -        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
>>>> +        if ( (c(ctrlreg[0]) & HVM_CR0_GUEST_RESERVED_BITS)
||
>>>> +             (c(ctrlreg[4]) &
HVM_CR4_GUEST_RESERVED_BITS(v)) ||
>>>> +             c(ctrlreg[1]) || c(ctrlreg[2]) || c(ctrlreg[5])
||
>>>> +             c(ctrlreg[6]) || c(ctrlreg[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;
>>>>      }
>>>
>>> Still no checking of debugreg[]?
>>
>> Why do we need to check debugreg[]? This code is executed on PVH (and
PV
>> and HVM), and I guessed it does the right thing:
>>
>>     for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
>>         v->arch.debugreg[i] = c(debugreg[i]);
> 
> If it does - fine; I didn't verify whether that would actually result
in
> the debug registers getting properly loaded with the requested
> values.
I've done a quick test by setting debugreg[0] from vcpu_guest_context
and then reading dr0 from inside the guest AP and it seems to work fine.
>>>> +            v->arch.hvm_vcpu.guest_cr[0] |=
c.nat->ctrlreg[0];
>>>
>>> Did you really mean |= here? I would expect to simply fail a
>>> request when certain required bits aren't set.
>>
>> Yes, I wanted to do |= because as described on the public header, flags
>> specified by the user are appended to PVH mandatory flags. This is kind
>> of awkward, so I wouldn't mind making cr0/cr4 mandatory for PVH AP
>> bringup, so we would have to check that:
>>
>> cr0 & (X86_CR0_PE | X86_CR0_ET | X86_CR0_PG) == (X86_CR0_PE |
X86_CR0_ET
>> | X86_CR0_PG)
> 
> Except that I'm not sure the ET check is really needed.
> 
>> And:
>>
>> cr4 & X86_CR4_PAE == X86_CR4_PAE
>>
>>> 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).

When you refer to not using any fields, does this mean to enable LAPIC
for PVH and use the bare metal CPU bringup method?

And I guess introducing a new hypercall (that also uses a different
vcpu_guest_context struct) to bringup vcpus inside of HVM domains is
completely out of the picture?
> 
>> and I don't really have a strong
>> opinion against that. What I think was definitely wrong was only using
>> gs_base_kernel and not the other gs_* or fs_* fields.
>>
>> Since we need cr3, using only those and not the other cr* fields seems
>> strange.
> 
> Hmm, I think it's going to remain looking strange as long as some
> of the fields are used and some are not (of course ignoring those
> that are PV specific).
> 
> Jan
> 

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

Jan Beulich

2013-Nov-19 16:53 UTC

head link

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

>>> 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.
> When you refer to not using any fields, does this mean to enable LAPIC
> for PVH and use the bare metal CPU bringup method?
Clearly not.
> And I guess introducing a new hypercall (that also uses a different
> vcpu_guest_context struct) to bringup vcpus inside of HVM domains is
> completely out of the picture?
It would seem awkward.

Jan

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

Roger Pau Monné

2013-Nov-20 09:18 UTC

head link

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

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)


--------------010402080106080707090107
Content-Type: text/plain; charset="UTF-8"; x-mac-type=0;
x-mac-creator=0;
	name="0001-pvh-clearly-specify-used-parameters-in-vcpu_guest_co.patch"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
	filename*0="0001-pvh-clearly-specify-used-parameters-in-vcpu_guest_co.pa";
	filename*1="tch"

RnJvbSAxZmE4NDQ2NGNhOGI2NTg2MGEyMWU0ZTNkOWFjOTY0NmJmZTU1OTFiIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBSb2dlciBQYXUgTW9ubmUgPHJvZ2VyLnBhdUBjaXRy
aXguY29tPgpEYXRlOiBUaHUsIDE0IE5vdiAyMDEzIDE4OjA3OjUxICswMTAwClN1YmplY3Q6
IFtQQVRDSCB2Ml0gcHZoOiBjbGVhcmx5IHNwZWNpZnkgdXNlZCBwYXJhbWV0ZXJzIGluCiB2
Y3B1X2d1ZXN0X2NvbnRleHQKTUlNRS1WZXJzaW9uOiAxLjAKQ29udGVudC1UeXBlOiB0ZXh0
L3BsYWluOyBjaGFyc2V0PVVURi04CkNvbnRlbnQtVHJhbnNmZXItRW5jb2Rpbmc6IDhiaXQK
ClRoZSBhaW0gb2YgdGhpcyBwYXRjaCBpcyB0byBkZWZpbmUgYSBzdGFibGUgd2F5IGluIHdo
aWNoIFBWSCBpcwpnb2luZyB0byBkbyBBUCBicmluZ3VwLgoKU2luY2Ugd2UgYXJlIHJ1bm5p
bmcgaW5zaWRlIG9mIGEgSFZNIGNvbnRhaW5lciwgUFZIIHNob3VsZCBvbmx5IG5lZWQKdG8g
c2V0IGZsYWdzLCBjcjMgYW5kIHVzZXJfcmVncyBpbiBvcmRlciB0byBicmluZyB1cCBhIHZD
UFUsIHRoZSByZXN0CmNhbiBiZSBzZXQgb25jZSB0aGUgdkNQVSBpcyBzdGFydGVkIHVzaW5n
IHRoZSBiYXJlIG1ldGFsIG1ldGhvZHMuCgpTaWduZWQtb2ZmLWJ5OiBSb2dlciBQYXUgTW9u
bsOpIDxyb2dlci5wYXVAY2l0cml4LmNvbT4KQ2M6IEdlb3JnZSBEdW5sYXAgPGdlb3JnZS5k
dW5sYXBAZXUuY2l0cml4LmNvbT4KQ2M6IE11a2VzaCBSYXRob3IgPG11a2VzaC5yYXRob3JA
b3JhY2xlLmNvbT4KQ2M6IEphbiBCZXVsaWNoIDxKQmV1bGljaEBzdXNlLmNvbT4KQ2M6IFRp
bSBEZWVnYW4gPHRpbUB4ZW4ub3JnPgpDYzogS2VpciBGcmFzZXIgPGtlaXJAeGVuLm9yZz4K
LS0tCiB4ZW4vYXJjaC94ODYvZG9tYWluLmMgICAgICAgICAgICAgfCAgIDI0ICsrKysrKysr
KysrLS0tLS0tLS0tLS0tLQogeGVuL2FyY2gveDg2L2h2bS92bXgvdm14LmMgICAgICAgIHwg
ICAgNiArLS0tLS0KIHhlbi9pbmNsdWRlL2FzbS14ODYvaHZtL2h2bS5oICAgICB8ICAgIDYg
KysrLS0tCiB4ZW4vaW5jbHVkZS9wdWJsaWMvYXJjaC14ODYveGVuLmggfCAgICA4ICsrKy0t
LS0tCiA0IGZpbGVzIGNoYW5nZWQsIDE4IGluc2VydGlvbnMoKyksIDI2IGRlbGV0aW9ucygt
KQoKZGlmZiAtLWdpdCBhL3hlbi9hcmNoL3g4Ni9kb21haW4uYyBiL3hlbi9hcmNoL3g4Ni9k
b21haW4uYwppbmRleCBhMzg2OGY5Li5hYTA0M2E4IDEwMDY0NAotLS0gYS94ZW4vYXJjaC94
ODYvZG9tYWluLmMKKysrIGIveGVuL2FyY2gveDg2L2RvbWFpbi5jCkBAIC03MDQsMTAgKzcw
NCwxNiBAQCBpbnQgYXJjaF9zZXRfaW5mb19ndWVzdCgKICAgICAgICAgLyogUFZIIDMyYml0
Zml4bWUgKi8KICAgICAgICAgQVNTRVJUKCFjb21wYXQpOwogCi0gICAgICAgIGlmICggYyhj
dHJscmVnWzFdKSB8fCBjKGxkdF9iYXNlKSB8fCBjKGxkdF9lbnRzKSB8fAotICAgICAgICAg
ICAgIGModXNlcl9yZWdzLmNzKSB8fCBjKHVzZXJfcmVncy5zcykgfHwgYyh1c2VyX3JlZ3Mu
ZXMpIHx8Ci0gICAgICAgICAgICAgYyh1c2VyX3JlZ3MuZHMpIHx8IGModXNlcl9yZWdzLmZz
KSB8fCBjKHVzZXJfcmVncy5ncykgfHwKLSAgICAgICAgICAgICBjLm5hdC0+Z2R0X2VudHMg
fHwgYy5uYXQtPmZzX2Jhc2UgfHwgYy5uYXQtPmdzX2Jhc2VfdXNlciApCisgICAgICAgIGlm
ICggYyhjdHJscmVnWzBdKSB8fCBjKGN0cmxyZWdbMV0pIHx8IGMoY3RybHJlZ1syXSkgfHwK
KyAgICAgICAgICAgICBjKGN0cmxyZWdbNF0pIHx8IGMoY3RybHJlZ1s1XSkgfHwgYyhjdHJs
cmVnWzZdKSB8fAorICAgICAgICAgICAgIGMoY3RybHJlZ1s3XSkgfHwgYyhkZWJ1Z3JlZ1sw
XSkgfHwgYyhkZWJ1Z3JlZ1sxXSkgfHwKKyAgICAgICAgICAgICBjKGRlYnVncmVnWzJdKSB8
fCBjKGRlYnVncmVnWzNdKSB8fCBjKGRlYnVncmVnWzRdKSB8fAorICAgICAgICAgICAgIGMo
ZGVidWdyZWdbNV0pIHx8IGMoZGVidWdyZWdbNl0pIHx8IGMoZGVidWdyZWdbN10pIHx8Cisg
ICAgICAgICAgICAgYyhsZHRfYmFzZSkgfHwgYyhsZHRfZW50cykgfHwgYyh1c2VyX3JlZ3Mu
Y3MpIHx8CisgICAgICAgICAgICAgYyh1c2VyX3JlZ3Muc3MpIHx8IGModXNlcl9yZWdzLmVz
KSB8fCBjKHVzZXJfcmVncy5kcykgfHwKKyAgICAgICAgICAgICBjKHVzZXJfcmVncy5mcykg
fHwgYyh1c2VyX3JlZ3MuZ3MpIHx8IGMoa2VybmVsX3NzKSB8fAorICAgICAgICAgICAgIGMo
a2VybmVsX3NwKSB8fCBjLm5hdC0+Z3NfYmFzZV9rZXJuZWwgfHwgYy5uYXQtPmdkdF9lbnRz
IHx8CisgICAgICAgICAgICAgYy5uYXQtPmZzX2Jhc2UgfHwgYy5uYXQtPmdzX2Jhc2VfdXNl
ciApCiAgICAgICAgICAgICByZXR1cm4gLUVJTlZBTDsKICAgICB9CiAKQEAgLTc0MCwxOCAr
NzQ2LDEwIEBAIGludCBhcmNoX3NldF9pbmZvX2d1ZXN0KAogICAgICAgICAgICAgWExBVF90
cmFwX2luZm8odi0+YXJjaC5wdl92Y3B1LnRyYXBfY3R4dCArIGksCiAgICAgICAgICAgICAg
ICAgICAgICAgICAgICBjLmNtcC0+dHJhcF9jdHh0ICsgaSk7CiAgICAgfQotICAgIGZvciAo
IGkgPSAwOyBpIDwgQVJSQVlfU0laRSh2LT5hcmNoLmRlYnVncmVnKTsgKytpICkKLSAgICAg
ICAgdi0+YXJjaC5kZWJ1Z3JlZ1tpXSA9IGMoZGVidWdyZWdbaV0pOwogCiAgICAgaWYgKCBo
YXNfaHZtX2NvbnRhaW5lcl92Y3B1KHYpICkKICAgICB7Ci0gICAgICAgIC8qCi0gICAgICAg
ICAqIE5COiBURl9rZXJuZWxfbW9kZSBpcyBzZXQgdW5jb25kaXRpb25hbGx5IGZvciBIVk0g
Z3Vlc3RzLAotICAgICAgICAgKiBzbyB3ZSBhbHdheXMgdXNlIHRoZSBnc19iYXNlX2tlcm5l
bCBoZXJlLiBJZiB3ZSBjaGFuZ2UgdGhpcwotICAgICAgICAgKiBmdW5jdGlvbiB0byBpbWl0
YXRlIHRoZSBQViBmdW5jdGlvbmFsaXR5LCB3ZSdsbCBuZWVkIHRvCi0gICAgICAgICAqIG1h
a2UgaXQgcGF5IGF0dGVudGlvbiB0byB0aGUga2VybmVsIGJpdC4KLSAgICAgICAgICovCi0g
ICAgICAgIGh2bV9zZXRfaW5mb19ndWVzdCh2LCBjb21wYXQgPyAwIDogYy5uYXQtPmdzX2Jh
c2Vfa2VybmVsKTsKKyAgICAgICAgaHZtX3NldF9pbmZvX2d1ZXN0KHYpOwogCiAgICAgICAg
IGlmICggaXNfaHZtX3ZjcHUodikgfHwgdi0+aXNfaW5pdGlhbGlzZWQgKQogICAgICAgICAg
ICAgZ290byBvdXQ7CmRpZmYgLS1naXQgYS94ZW4vYXJjaC94ODYvaHZtL3ZteC92bXguYyBi
L3hlbi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jCmluZGV4IGYwMTMyYTQuLjhkOTIzZTcgMTAw
NjQ0Ci0tLSBhL3hlbi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jCisrKyBiL3hlbi9hcmNoL3g4
Ni9odm0vdm14L3ZteC5jCkBAIC0xNDY3LDcgKzE0NjcsNyBAQCBzdGF0aWMgaW50IHZteF9l
dmVudF9wZW5kaW5nKHN0cnVjdCB2Y3B1ICp2KQogICAgIHJldHVybiBpbnRyX2luZm8gJiBJ
TlRSX0lORk9fVkFMSURfTUFTSzsKIH0KIAotc3RhdGljIHZvaWQgdm14X3NldF9pbmZvX2d1
ZXN0KHN0cnVjdCB2Y3B1ICp2LCB1aW50NjRfdCBnc19iYXNlX2tlcm5lbCkKK3N0YXRpYyB2
b2lkIHZteF9zZXRfaW5mb19ndWVzdChzdHJ1Y3QgdmNwdSAqdikKIHsKICAgICB1bnNpZ25l
ZCBsb25nIGludHJfc2hhZG93OwogCkBAIC0xNDkyLDEwICsxNDkyLDYgQEAgc3RhdGljIHZv
aWQgdm14X3NldF9pbmZvX2d1ZXN0KHN0cnVjdCB2Y3B1ICp2LCB1aW50NjRfdCBnc19iYXNl
X2tlcm5lbCkKICAgICAgICAgX192bXdyaXRlKEdVRVNUX0lOVEVSUlVQVElCSUxJVFlfSU5G
TywgaW50cl9zaGFkb3cpOwogICAgIH0KIAotICAgIC8qIFBWSCAzMmJpdGZpeG1lICovCi0g
ICAgaWYgKCBpc19wdmhfdmNwdSh2KSApCi0gICAgICAgIF9fdm13cml0ZShHVUVTVF9HU19C
QVNFLCBnc19iYXNlX2tlcm5lbCk7Ci0KICAgICB2bXhfdm1jc19leGl0KHYpOwogfQogCmRp
ZmYgLS1naXQgYS94ZW4vaW5jbHVkZS9hc20teDg2L2h2bS9odm0uaCBiL3hlbi9pbmNsdWRl
L2FzbS14ODYvaHZtL2h2bS5oCmluZGV4IGE4YmEwNmQuLmNjY2E1ZGYgMTAwNjQ0Ci0tLSBh
L3hlbi9pbmNsdWRlL2FzbS14ODYvaHZtL2h2bS5oCisrKyBiL3hlbi9pbmNsdWRlL2FzbS14
ODYvaHZtL2h2bS5oCkBAIC0xNjAsNyArMTYwLDcgQEAgc3RydWN0IGh2bV9mdW5jdGlvbl90
YWJsZSB7CiAgICAgaW50ICgqbXNyX3dyaXRlX2ludGVyY2VwdCkodW5zaWduZWQgaW50IG1z
ciwgdWludDY0X3QgbXNyX2NvbnRlbnQpOwogICAgIHZvaWQgKCppbnZscGdfaW50ZXJjZXB0
KSh1bnNpZ25lZCBsb25nIHZhZGRyKTsKICAgICB2b2lkICgqaGFuZGxlX2NkKShzdHJ1Y3Qg
dmNwdSAqdiwgdW5zaWduZWQgbG9uZyB2YWx1ZSk7Ci0gICAgdm9pZCAoKnNldF9pbmZvX2d1
ZXN0KShzdHJ1Y3QgdmNwdSAqdiwgdWludDY0X3QgZ3NfYmFzZV9rZXJuZWwpOworICAgIHZv
aWQgKCpzZXRfaW5mb19ndWVzdCkoc3RydWN0IHZjcHUgKnYpOwogICAgIHZvaWQgKCpzZXRf
cmR0c2NfZXhpdGluZykoc3RydWN0IHZjcHUgKnYsIGJvb2xfdCk7CiAKICAgICAvKiBOZXN0
ZWQgSFZNICovCkBAIC00MzQsMTAgKzQzNCwxMCBAQCB2b2lkICpodm1fbWFwX2d1ZXN0X2Zy
YW1lX3J3KHVuc2lnbmVkIGxvbmcgZ2ZuLCBib29sX3QgcGVybWFuZW50KTsKIHZvaWQgKmh2
bV9tYXBfZ3Vlc3RfZnJhbWVfcm8odW5zaWduZWQgbG9uZyBnZm4sIGJvb2xfdCBwZXJtYW5l
bnQpOwogdm9pZCBodm1fdW5tYXBfZ3Vlc3RfZnJhbWUodm9pZCAqcCwgYm9vbF90IHBlcm1h
bmVudCk7CiAKLXN0YXRpYyBpbmxpbmUgdm9pZCBodm1fc2V0X2luZm9fZ3Vlc3Qoc3RydWN0
IHZjcHUgKnYsIHVpbnQ2NF90IGdzX2Jhc2Vfa2VybmVsKQorc3RhdGljIGlubGluZSB2b2lk
IGh2bV9zZXRfaW5mb19ndWVzdChzdHJ1Y3QgdmNwdSAqdikKIHsKICAgICBpZiAoIGh2bV9m
dW5jcy5zZXRfaW5mb19ndWVzdCApCi0gICAgICAgIHJldHVybiBodm1fZnVuY3Muc2V0X2lu
Zm9fZ3Vlc3QodiwgZ3NfYmFzZV9rZXJuZWwpOworICAgICAgICByZXR1cm4gaHZtX2Z1bmNz
LnNldF9pbmZvX2d1ZXN0KHYpOwogfQogCiBpbnQgaHZtX2RlYnVnX29wKHN0cnVjdCB2Y3B1
ICp2LCBpbnQzMl90IG9wKTsKZGlmZiAtLWdpdCBhL3hlbi9pbmNsdWRlL3B1YmxpYy9hcmNo
LXg4Ni94ZW4uaCBiL3hlbi9pbmNsdWRlL3B1YmxpYy9hcmNoLXg4Ni94ZW4uaAppbmRleCA1
ZDIyMGNlLi44YzkyMzA4IDEwMDY0NAotLS0gYS94ZW4vaW5jbHVkZS9wdWJsaWMvYXJjaC14
ODYveGVuLmgKKysrIGIveGVuL2luY2x1ZGUvcHVibGljL2FyY2gteDg2L3hlbi5oCkBAIC0x
NTksMTIgKzE1OSwxMCBAQCB0eXBlZGVmIHVpbnQ2NF90IHRzY190aW1lc3RhbXBfdDsgLyog
UkRUU0MgdGltZXN0YW1wICovCiAgKiBmb3IgSFZNIGFuZCBQVkggZ3Vlc3RzLCBub3QgYWxs
IGluZm9ybWF0aW9uIGluIHRoaXMgc3RydWN0dXJlIGlzIHVwZGF0ZWQ6CiAgKgogICogLSBG
b3IgSFZNIGd1ZXN0cywgdGhlIHN0cnVjdHVyZXMgcmVhZCBpbmNsdWRlOiBmcHVfY3R4dCAo
aWYKLSAqIFZHQ1RfSTM4N19WQUxJRCBpcyBzZXQpLCBmbGFncywgdXNlcl9yZWdzLCBkZWJ1
Z3JlZ1sqXQorICogVkdDVF9JMzg3X1ZBTElEIGlzIHNldCksIGZsYWdzIGFuZCB1c2VyX3Jl
Z3MuCiAgKgotICogLSBQVkggZ3Vlc3RzIGFyZSB0aGUgc2FtZSBhcyBIVk0gZ3Vlc3RzLCBi
dXQgYWRkaXRpb25hbGx5IHNldCBjcjMsCi0gKiBhbmQgZm9yIDY0LWJpdCBndWVzdHMsIGdz
X2Jhc2Vfa2VybmVsLiAgQWRkaXRpb25hbGx5LCB0aGUgZm9sbG93aW5nCi0gKiBlbnRyaWVz
IG11c3QgYmUgMDogY3RybHJlZ1sxXSwgbGR0X2Jhc2UsIGxkdF9lbnRzLCB1c2VyX3JlZ3Mu
e2NzLAotICogc3MsIGVzLCBkcywgZnMsIGdzKSwgZ2R0X2VudHMsIGZzX2Jhc2UsIGFuZCBn
c19iYXNlX3VzZXIuCisgKiAtIFBWSCBndWVzdHMgYXJlIHRoZSBzYW1lIGFzIEhWTSBndWVz
dHMsIGJ1dCBhZGRpdGlvbmFsbHkgdXNlIGN0cmxyZWdbM10gdG8KKyAqIHNldCBjcjMuIEFs
bCBvdGhlciBmaWVsZHMgbm90IHVzZWQgc2hvdWxkIGJlIHNldCB0byAwLgogICovCiBzdHJ1
Y3QgdmNwdV9ndWVzdF9jb250ZXh0IHsKICAgICAvKiBGUFUgcmVnaXN0ZXJzIGNvbWUgZmly
c3Qgc28gdGhleSBjYW4gYmUgYWxpZ25lZCBmb3IgRlhTQVZFL0ZYUlNUT1IuICovCi0tIAox
LjcuNy41IChBcHBsZSBHaXQtMjYpCgo--------------010402080106080707090107
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--------------010402080106080707090107--

Jan Beulich

2013-Nov-20 09:28 UTC

head link

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

>>> On 20.11.13 at 10:18, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 19/11/13 17:53, Jan Beulich wrote:
>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>> 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:
You can't - this code is also used for HVM guests.

Jan

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

Roger Pau Monné

2013-Nov-20 09:37 UTC

head link

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

On 20/11/13 10:28, Jan Beulich wrote:>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>> 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:
> 
> You can't - this code is also used for HVM guests.
Yes, my fault, I erroneously thought this was introduced by 35b1e076,
but it has been there longer than that. Would you agree to a patch
similar to the one posted, but without touching the setting of debugregs?


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

Jan Beulich

2013-Nov-20 09:54 UTC

head link

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

>>> On 20.11.13 at 10:37, Roger Pau Monné<roger.pau@citrix.com>
wrote:
> On 20/11/13 10:28, Jan Beulich wrote:
>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>> 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:
>> 
>> You can't - this code is also used for HVM guests.
> 
> Yes, my fault, I erroneously thought this was introduced by 35b1e076,
> but it has been there longer than that. Would you agree to a patch
> similar to the one posted, but without touching the setting of debugregs?
Yes, if Mukesh and George confirm that this is not going to break
things.

Jan

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

Roger Pau Monné

2013-Nov-20 10:29 UTC

head link

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

On 20/11/13 10:54, Jan Beulich wrote:>>>> On 20.11.13 at 10:37, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>> On 20/11/13 10:28, Jan Beulich wrote:
>>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>>> 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:
>>>
>>> You can''t - this code is also used for HVM guests.
>>
>> Yes, my fault, I erroneously thought this was introduced by 35b1e076,
>> but it has been there longer than that. Would you agree to a patch
>> similar to the one posted, but without touching the setting of
debugregs?
> 
> Yes, if Mukesh and George confirm that this is not going to break
> things.
Here it is:

---
From dc19632f361b2737791821232ce9b38508f1cd7f 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 v3] 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             |   13 +++++--------
 xen/arch/x86/hvm/vmx/vmx.c        |    6 +-----
 xen/include/asm-x86/hvm/hvm.h     |    6 +++---
 xen/include/public/arch-x86/xen.h |    6 ++----
 4 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index a3868f9..c0ac5d6 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -704,9 +704,12 @@ int arch_set_info_guest(
         /* PVH 32bitfixme */
         ASSERT(!compat);
 
-        if ( c(ctrlreg[1]) || c(ldt_base) || c(ldt_ents) ||
+        if ( c(ctrlreg[0]) || c(ctrlreg[1]) || c(ctrlreg[2]) ||
+             c(ctrlreg[4]) || c(ctrlreg[5]) || c(ctrlreg[6]) ||
+             c(ctrlreg[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;
     }
@@ -745,13 +748,7 @@ int arch_set_info_guest(
 
     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..f35804b 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -161,10 +161,8 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
  * - For HVM guests, the structures read include: fpu_ctxt (if
  * VGCT_I387_VALID is set), flags, user_regs, debugreg[*]
  *
- * - 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)


--------------020801030003080800040506
Content-Type: text/plain; charset="UTF-8"; x-mac-type=0;
x-mac-creator=0;
	name="0001-pvh-clearly-specify-used-parameters-in-vcpu_guest_co.patch"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
	filename*0="0001-pvh-clearly-specify-used-parameters-in-vcpu_guest_co.pa";
	filename*1="tch"

RnJvbSBkYzE5NjMyZjM2MWIyNzM3NzkxODIxMjMyY2U5YjM4NTA4ZjFjZDdmIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBSb2dlciBQYXUgTW9ubmUgPHJvZ2VyLnBhdUBjaXRy
aXguY29tPgpEYXRlOiBUaHUsIDE0IE5vdiAyMDEzIDE4OjA3OjUxICswMTAwClN1YmplY3Q6
IFtQQVRDSCB2M10gcHZoOiBjbGVhcmx5IHNwZWNpZnkgdXNlZCBwYXJhbWV0ZXJzIGluCiB2
Y3B1X2d1ZXN0X2NvbnRleHQKTUlNRS1WZXJzaW9uOiAxLjAKQ29udGVudC1UeXBlOiB0ZXh0
L3BsYWluOyBjaGFyc2V0PVVURi04CkNvbnRlbnQtVHJhbnNmZXItRW5jb2Rpbmc6IDhiaXQK
ClRoZSBhaW0gb2YgdGhpcyBwYXRjaCBpcyB0byBkZWZpbmUgYSBzdGFibGUgd2F5IGluIHdo
aWNoIFBWSCBpcwpnb2luZyB0byBkbyBBUCBicmluZ3VwLgoKU2luY2Ugd2UgYXJlIHJ1bm5p
bmcgaW5zaWRlIG9mIGEgSFZNIGNvbnRhaW5lciwgUFZIIHNob3VsZCBvbmx5IG5lZWQKdG8g
c2V0IGZsYWdzLCBjcjMgYW5kIHVzZXJfcmVncyBpbiBvcmRlciB0byBicmluZyB1cCBhIHZD
UFUsIHRoZSByZXN0CmNhbiBiZSBzZXQgb25jZSB0aGUgdkNQVSBpcyBzdGFydGVkIHVzaW5n
IHRoZSBiYXJlIG1ldGFsIG1ldGhvZHMuCgpTaWduZWQtb2ZmLWJ5OiBSb2dlciBQYXUgTW9u
bsOpIDxyb2dlci5wYXVAY2l0cml4LmNvbT4KQ2M6IEdlb3JnZSBEdW5sYXAgPGdlb3JnZS5k
dW5sYXBAZXUuY2l0cml4LmNvbT4KQ2M6IE11a2VzaCBSYXRob3IgPG11a2VzaC5yYXRob3JA
b3JhY2xlLmNvbT4KQ2M6IEphbiBCZXVsaWNoIDxKQmV1bGljaEBzdXNlLmNvbT4KQ2M6IFRp
bSBEZWVnYW4gPHRpbUB4ZW4ub3JnPgpDYzogS2VpciBGcmFzZXIgPGtlaXJAeGVuLm9yZz4K
LS0tCiB4ZW4vYXJjaC94ODYvZG9tYWluLmMgICAgICAgICAgICAgfCAgIDEzICsrKysrLS0t
LS0tLS0KIHhlbi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jICAgICAgICB8ICAgIDYgKy0tLS0t
CiB4ZW4vaW5jbHVkZS9hc20teDg2L2h2bS9odm0uaCAgICAgfCAgICA2ICsrKy0tLQogeGVu
L2luY2x1ZGUvcHVibGljL2FyY2gteDg2L3hlbi5oIHwgICAgNiArKy0tLS0KIDQgZmlsZXMg
Y2hhbmdlZCwgMTEgaW5zZXJ0aW9ucygrKSwgMjAgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0
IGEveGVuL2FyY2gveDg2L2RvbWFpbi5jIGIveGVuL2FyY2gveDg2L2RvbWFpbi5jCmluZGV4
IGEzODY4ZjkuLmMwYWM1ZDYgMTAwNjQ0Ci0tLSBhL3hlbi9hcmNoL3g4Ni9kb21haW4uYwor
KysgYi94ZW4vYXJjaC94ODYvZG9tYWluLmMKQEAgLTcwNCw5ICs3MDQsMTIgQEAgaW50IGFy
Y2hfc2V0X2luZm9fZ3Vlc3QoCiAgICAgICAgIC8qIFBWSCAzMmJpdGZpeG1lICovCiAgICAg
ICAgIEFTU0VSVCghY29tcGF0KTsKIAotICAgICAgICBpZiAoIGMoY3RybHJlZ1sxXSkgfHwg
YyhsZHRfYmFzZSkgfHwgYyhsZHRfZW50cykgfHwKKyAgICAgICAgaWYgKCBjKGN0cmxyZWdb
MF0pIHx8IGMoY3RybHJlZ1sxXSkgfHwgYyhjdHJscmVnWzJdKSB8fAorICAgICAgICAgICAg
IGMoY3RybHJlZ1s0XSkgfHwgYyhjdHJscmVnWzVdKSB8fCBjKGN0cmxyZWdbNl0pIHx8Cisg
ICAgICAgICAgICAgYyhjdHJscmVnWzddKSB8fCAgYyhsZHRfYmFzZSkgfHwgYyhsZHRfZW50
cykgfHwKICAgICAgICAgICAgICBjKHVzZXJfcmVncy5jcykgfHwgYyh1c2VyX3JlZ3Muc3Mp
IHx8IGModXNlcl9yZWdzLmVzKSB8fAogICAgICAgICAgICAgIGModXNlcl9yZWdzLmRzKSB8
fCBjKHVzZXJfcmVncy5mcykgfHwgYyh1c2VyX3JlZ3MuZ3MpIHx8CisgICAgICAgICAgICAg
YyhrZXJuZWxfc3MpIHx8IGMoa2VybmVsX3NwKSB8fCBjLm5hdC0+Z3NfYmFzZV9rZXJuZWwg
fHwKICAgICAgICAgICAgICBjLm5hdC0+Z2R0X2VudHMgfHwgYy5uYXQtPmZzX2Jhc2UgfHwg
Yy5uYXQtPmdzX2Jhc2VfdXNlciApCiAgICAgICAgICAgICByZXR1cm4gLUVJTlZBTDsKICAg
ICB9CkBAIC03NDUsMTMgKzc0OCw3IEBAIGludCBhcmNoX3NldF9pbmZvX2d1ZXN0KAogCiAg
ICAgaWYgKCBoYXNfaHZtX2NvbnRhaW5lcl92Y3B1KHYpICkKICAgICB7Ci0gICAgICAgIC8q
Ci0gICAgICAgICAqIE5COiBURl9rZXJuZWxfbW9kZSBpcyBzZXQgdW5jb25kaXRpb25hbGx5
IGZvciBIVk0gZ3Vlc3RzLAotICAgICAgICAgKiBzbyB3ZSBhbHdheXMgdXNlIHRoZSBnc19i
YXNlX2tlcm5lbCBoZXJlLiBJZiB3ZSBjaGFuZ2UgdGhpcwotICAgICAgICAgKiBmdW5jdGlv
biB0byBpbWl0YXRlIHRoZSBQViBmdW5jdGlvbmFsaXR5LCB3ZSdsbCBuZWVkIHRvCi0gICAg
ICAgICAqIG1ha2UgaXQgcGF5IGF0dGVudGlvbiB0byB0aGUga2VybmVsIGJpdC4KLSAgICAg
ICAgICovCi0gICAgICAgIGh2bV9zZXRfaW5mb19ndWVzdCh2LCBjb21wYXQgPyAwIDogYy5u
YXQtPmdzX2Jhc2Vfa2VybmVsKTsKKyAgICAgICAgaHZtX3NldF9pbmZvX2d1ZXN0KHYpOwog
CiAgICAgICAgIGlmICggaXNfaHZtX3ZjcHUodikgfHwgdi0+aXNfaW5pdGlhbGlzZWQgKQog
ICAgICAgICAgICAgZ290byBvdXQ7CmRpZmYgLS1naXQgYS94ZW4vYXJjaC94ODYvaHZtL3Zt
eC92bXguYyBiL3hlbi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jCmluZGV4IGYwMTMyYTQuLjhk
OTIzZTcgMTAwNjQ0Ci0tLSBhL3hlbi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jCisrKyBiL3hl
bi9hcmNoL3g4Ni9odm0vdm14L3ZteC5jCkBAIC0xNDY3LDcgKzE0NjcsNyBAQCBzdGF0aWMg
aW50IHZteF9ldmVudF9wZW5kaW5nKHN0cnVjdCB2Y3B1ICp2KQogICAgIHJldHVybiBpbnRy
X2luZm8gJiBJTlRSX0lORk9fVkFMSURfTUFTSzsKIH0KIAotc3RhdGljIHZvaWQgdm14X3Nl
dF9pbmZvX2d1ZXN0KHN0cnVjdCB2Y3B1ICp2LCB1aW50NjRfdCBnc19iYXNlX2tlcm5lbCkK
K3N0YXRpYyB2b2lkIHZteF9zZXRfaW5mb19ndWVzdChzdHJ1Y3QgdmNwdSAqdikKIHsKICAg
ICB1bnNpZ25lZCBsb25nIGludHJfc2hhZG93OwogCkBAIC0xNDkyLDEwICsxNDkyLDYgQEAg
c3RhdGljIHZvaWQgdm14X3NldF9pbmZvX2d1ZXN0KHN0cnVjdCB2Y3B1ICp2LCB1aW50NjRf
dCBnc19iYXNlX2tlcm5lbCkKICAgICAgICAgX192bXdyaXRlKEdVRVNUX0lOVEVSUlVQVElC
SUxJVFlfSU5GTywgaW50cl9zaGFkb3cpOwogICAgIH0KIAotICAgIC8qIFBWSCAzMmJpdGZp
eG1lICovCi0gICAgaWYgKCBpc19wdmhfdmNwdSh2KSApCi0gICAgICAgIF9fdm13cml0ZShH
VUVTVF9HU19CQVNFLCBnc19iYXNlX2tlcm5lbCk7Ci0KICAgICB2bXhfdm1jc19leGl0KHYp
OwogfQogCmRpZmYgLS1naXQgYS94ZW4vaW5jbHVkZS9hc20teDg2L2h2bS9odm0uaCBiL3hl
bi9pbmNsdWRlL2FzbS14ODYvaHZtL2h2bS5oCmluZGV4IGE4YmEwNmQuLmNjY2E1ZGYgMTAw
NjQ0Ci0tLSBhL3hlbi9pbmNsdWRlL2FzbS14ODYvaHZtL2h2bS5oCisrKyBiL3hlbi9pbmNs
dWRlL2FzbS14ODYvaHZtL2h2bS5oCkBAIC0xNjAsNyArMTYwLDcgQEAgc3RydWN0IGh2bV9m
dW5jdGlvbl90YWJsZSB7CiAgICAgaW50ICgqbXNyX3dyaXRlX2ludGVyY2VwdCkodW5zaWdu
ZWQgaW50IG1zciwgdWludDY0X3QgbXNyX2NvbnRlbnQpOwogICAgIHZvaWQgKCppbnZscGdf
aW50ZXJjZXB0KSh1bnNpZ25lZCBsb25nIHZhZGRyKTsKICAgICB2b2lkICgqaGFuZGxlX2Nk
KShzdHJ1Y3QgdmNwdSAqdiwgdW5zaWduZWQgbG9uZyB2YWx1ZSk7Ci0gICAgdm9pZCAoKnNl
dF9pbmZvX2d1ZXN0KShzdHJ1Y3QgdmNwdSAqdiwgdWludDY0X3QgZ3NfYmFzZV9rZXJuZWwp
OworICAgIHZvaWQgKCpzZXRfaW5mb19ndWVzdCkoc3RydWN0IHZjcHUgKnYpOwogICAgIHZv
aWQgKCpzZXRfcmR0c2NfZXhpdGluZykoc3RydWN0IHZjcHUgKnYsIGJvb2xfdCk7CiAKICAg
ICAvKiBOZXN0ZWQgSFZNICovCkBAIC00MzQsMTAgKzQzNCwxMCBAQCB2b2lkICpodm1fbWFw
X2d1ZXN0X2ZyYW1lX3J3KHVuc2lnbmVkIGxvbmcgZ2ZuLCBib29sX3QgcGVybWFuZW50KTsK
IHZvaWQgKmh2bV9tYXBfZ3Vlc3RfZnJhbWVfcm8odW5zaWduZWQgbG9uZyBnZm4sIGJvb2xf
dCBwZXJtYW5lbnQpOwogdm9pZCBodm1fdW5tYXBfZ3Vlc3RfZnJhbWUodm9pZCAqcCwgYm9v
bF90IHBlcm1hbmVudCk7CiAKLXN0YXRpYyBpbmxpbmUgdm9pZCBodm1fc2V0X2luZm9fZ3Vl
c3Qoc3RydWN0IHZjcHUgKnYsIHVpbnQ2NF90IGdzX2Jhc2Vfa2VybmVsKQorc3RhdGljIGlu
bGluZSB2b2lkIGh2bV9zZXRfaW5mb19ndWVzdChzdHJ1Y3QgdmNwdSAqdikKIHsKICAgICBp
ZiAoIGh2bV9mdW5jcy5zZXRfaW5mb19ndWVzdCApCi0gICAgICAgIHJldHVybiBodm1fZnVu
Y3Muc2V0X2luZm9fZ3Vlc3QodiwgZ3NfYmFzZV9rZXJuZWwpOworICAgICAgICByZXR1cm4g
aHZtX2Z1bmNzLnNldF9pbmZvX2d1ZXN0KHYpOwogfQogCiBpbnQgaHZtX2RlYnVnX29wKHN0
cnVjdCB2Y3B1ICp2LCBpbnQzMl90IG9wKTsKZGlmZiAtLWdpdCBhL3hlbi9pbmNsdWRlL3B1
YmxpYy9hcmNoLXg4Ni94ZW4uaCBiL3hlbi9pbmNsdWRlL3B1YmxpYy9hcmNoLXg4Ni94ZW4u
aAppbmRleCA1ZDIyMGNlLi5mMzU4MDRiIDEwMDY0NAotLS0gYS94ZW4vaW5jbHVkZS9wdWJs
aWMvYXJjaC14ODYveGVuLmgKKysrIGIveGVuL2luY2x1ZGUvcHVibGljL2FyY2gteDg2L3hl
bi5oCkBAIC0xNjEsMTAgKzE2MSw4IEBAIHR5cGVkZWYgdWludDY0X3QgdHNjX3RpbWVzdGFt
cF90OyAvKiBSRFRTQyB0aW1lc3RhbXAgKi8KICAqIC0gRm9yIEhWTSBndWVzdHMsIHRoZSBz
dHJ1Y3R1cmVzIHJlYWQgaW5jbHVkZTogZnB1X2N0eHQgKGlmCiAgKiBWR0NUX0kzODdfVkFM
SUQgaXMgc2V0KSwgZmxhZ3MsIHVzZXJfcmVncywgZGVidWdyZWdbKl0KICAqCi0gKiAtIFBW
SCBndWVzdHMgYXJlIHRoZSBzYW1lIGFzIEhWTSBndWVzdHMsIGJ1dCBhZGRpdGlvbmFsbHkg
c2V0IGNyMywKLSAqIGFuZCBmb3IgNjQtYml0IGd1ZXN0cywgZ3NfYmFzZV9rZXJuZWwuICBB
ZGRpdGlvbmFsbHksIHRoZSBmb2xsb3dpbmcKLSAqIGVudHJpZXMgbXVzdCBiZSAwOiBjdHJs
cmVnWzFdLCBsZHRfYmFzZSwgbGR0X2VudHMsIHVzZXJfcmVncy57Y3MsCi0gKiBzcywgZXMs
IGRzLCBmcywgZ3MpLCBnZHRfZW50cywgZnNfYmFzZSwgYW5kIGdzX2Jhc2VfdXNlci4KKyAq
IC0gUFZIIGd1ZXN0cyBhcmUgdGhlIHNhbWUgYXMgSFZNIGd1ZXN0cywgYnV0IGFkZGl0aW9u
YWxseSB1c2UgY3RybHJlZ1szXSB0bworICogc2V0IGNyMy4gQWxsIG90aGVyIGZpZWxkcyBu
b3QgdXNlZCBzaG91bGQgYmUgc2V0IHRvIDAuCiAgKi8KIHN0cnVjdCB2Y3B1X2d1ZXN0X2Nv
bnRleHQgewogICAgIC8qIEZQVSByZWdpc3RlcnMgY29tZSBmaXJzdCBzbyB0aGV5IGNhbiBi
ZSBhbGlnbmVkIGZvciBGWFNBVkUvRlhSU1RPUi4gKi8KLS0gCjEuNy43LjUgKEFwcGxlIEdp
dC0yNikKCg=--------------020801030003080800040506
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--------------020801030003080800040506--

George Dunlap

2013-Nov-20 18:19 UTC

head link

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

On 20/11/13 10:29, Roger Pau Monné wrote:> On 20/11/13 10:54, Jan Beulich wrote:
>>>>> On 20.11.13 at 10:37, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>> On 20/11/13 10:28, Jan Beulich wrote:
>>>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>>>> 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:
>>>> You can't - this code is also used for HVM guests.
>>> Yes, my fault, I erroneously thought this was introduced by
35b1e076,
>>> but it has been there longer than that. Would you agree to a patch
>>> similar to the one posted, but without touching the setting of
debugregs?
>> Yes, if Mukesh and George confirm that this is not going to break
>> things.
Well it does change the interface, by not setting gs_base_kernel; but 
that was part of the point. :-)

The rest of it looks OK to me -- Roger, have you tested Linux?

  -George


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

Roger Pau Monné

2013-Nov-20 18:24 UTC

head link

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

On 20/11/13 19:19, George Dunlap wrote:> On 20/11/13 10:29, Roger Pau Monné wrote:
>> On 20/11/13 10:54, Jan Beulich wrote:
>>>>>> On 20.11.13 at 10:37, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>> On 20/11/13 10:28, Jan Beulich wrote:
>>>>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com>
>>>>>>>>>> wrote:
>>>>>>>> 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:
>>>>> You can't - this code is also used for HVM guests.
>>>> Yes, my fault, I erroneously thought this was introduced by
35b1e076,
>>>> but it has been there longer than that. Would you agree to a
patch
>>>> similar to the one posted, but without touching the setting of
>>>> debugregs?
>>> Yes, if Mukesh and George confirm that this is not going to break
>>> things.
> 
> Well it does change the interface, by not setting gs_base_kernel; but
> that was part of the point. :-)
> 
> The rest of it looks OK to me -- Roger, have you tested Linux?
No, I expect Mukesh to test it, since this requires Linux to not try to
set gs_base_kernel.


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

Mukesh Rathor

2013-Nov-20 21:19 UTC

head link

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

On Wed, 20 Nov 2013 19:24:33 +0100
Roger Pau Monné <roger.pau@citrix.com> wrote:
> On 20/11/13 19:19, George Dunlap wrote:
> > On 20/11/13 10:29, Roger Pau Monné wrote:
> >> On 20/11/13 10:54, Jan Beulich wrote:
> >>>>>> On 20.11.13 at 10:37, Roger Pau
Monné<roger.pau@citrix.com>
> >>>>>> wrote:
> >>>> On 20/11/13 10:28, Jan Beulich wrote:
> >>>>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com>
> >>>>>>>> wrote:
> >>>>>> On 19/11/13 17:53, Jan Beulich wrote:
> >>>>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com>
> >>>>>>>>>> wrote:
> >>>>>>>> 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:
> >>>>> You can't - this code is also used for HVM guests.
> >>>> Yes, my fault, I erroneously thought this was introduced
by
> >>>> 35b1e076, but it has been there longer than that. Would
you
> >>>> agree to a patch similar to the one posted, but without
touching
> >>>> the setting of debugregs?
> >>> Yes, if Mukesh and George confirm that this is not going to
break
> >>> things.
> > 
> > Well it does change the interface, by not setting gs_base_kernel;
> > but that was part of the point. :-)
> > 
> > The rest of it looks OK to me -- Roger, have you tested Linux?
> 
> No, I expect Mukesh to test it, since this requires Linux to not try
> to set gs_base_kernel.

Yes, I've modified linux to be ok without gs_base_kernel.

thanks
mukesh


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

Tim Deegan

2013-Nov-21 13:16 UTC

head link

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

At 15:34 +0000 on 19 Nov (1384871686), 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.
Yes, that is what I meant.  I think this API should either honour all
the fields (as it does for PV) or none of them (as for HVM).

I think it would be fine to do some subset for now and leave the API
experimental for 4.4, especially since we don''t know what this will
look like on 32-bit yet.

Tim.

George Dunlap

2013-Nov-22 17:38 UTC

head link

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

On 20/11/13 10:29, Roger Pau Monné wrote:> On 20/11/13 10:54, Jan Beulich wrote:
>>>>> On 20.11.13 at 10:37, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>> On 20/11/13 10:28, Jan Beulich wrote:
>>>>>>> On 20.11.13 at 10:18, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>> On 19/11/13 17:53, Jan Beulich wrote:
>>>>>>>>> On 19.11.13 at 17:42, Roger Pau
Monné<roger.pau@citrix.com> wrote:
>>>>>>> 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:
>>>> You can't - this code is also used for HVM guests.
>>> Yes, my fault, I erroneously thought this was introduced by
35b1e076,
>>> but it has been there longer than that. Would you agree to a patch
>>> similar to the one posted, but without touching the setting of
debugregs?
>> Yes, if Mukesh and George confirm that this is not going to break
>> things.
> Here it is:
>
> ---
>  From dc19632f361b2737791821232ce9b38508f1cd7f 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 v3] 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>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Release-wise, this only affects PVH codepaths, so should be fine.

  -George


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

Jan Beulich

2013-Nov-25 13:25 UTC

head link

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

>>> On 19.11.13 at 13:34, Roger Pau Monne <roger.pau@citrix.com>
wrote:
> Right now Xen sets the WP and NE flags on cr0 for PVH, which are not
> needed in order to boot with paging enabled. The same happens with
> cr4, at least on my system OSFXSR, OSXMMEXCPT and MCE are enabled by
> default when there's no need.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
So I'm still having this patch sitting in my queue, and I don't think
I've
seen an ack from the VMX maintainers or the main PVH contributors.
At least one of the two I'd like to have in place before applying.

Thanks, Jan
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -28,7 +28,6 @@
>  #include <asm/msr.h>
>  #include <asm/xstate.h>
>  #include <asm/hvm/hvm.h>
> -#include <asm/hvm/nestedhvm.h>
>  #include <asm/hvm/io.h>
>  #include <asm/hvm/support.h>
>  #include <asm/hvm/vmx/vmx.h>
> @@ -1089,13 +1088,11 @@ static int construct_vmcs(struct vcpu *v)
>  
>      /* PVH domains always start in paging mode */
>      if ( is_pvh_domain(d) )
> -        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG | X86_CR0_NE |
X86_CR0_WP;
> +        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
>  
>      hvm_update_guest_cr(v, 0);
>  
> -    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ?
> -        (real_cr4_to_pv_guest_cr4(mmu_cr4_features)
> -         & ~HVM_CR4_GUEST_RESERVED_BITS(v)) : 0;
> +    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ? X86_CR4_PAE : 0;
>      hvm_update_guest_cr(v, 4);
>  
>      if ( cpu_has_vmx_tpr_shadow )
> -- 
> 1.7.7.5 (Apple Git-26)


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

George Dunlap

2013-Nov-25 14:53 UTC

head link

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

On 11/25/2013 01:25 PM, Jan Beulich wrote:>>>> On 19.11.13 at 13:34, Roger Pau Monne
<roger.pau@citrix.com> wrote:
>> Right now Xen sets the WP and NE flags on cr0 for PVH, which are not
>> needed in order to boot with paging enabled. The same happens with
>> cr4, at least on my system OSFXSR, OSXMMEXCPT and MCE are enabled by
>> default when there's no need.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> So I'm still having this patch sitting in my queue, and I don't
think I've
> seen an ack from the VMX maintainers or the main PVH contributors.
> At least one of the two I'd like to have in place before applying.
This sounds like a good idea to me, but I don't feel like I have a firm 
enough grasp to give a reviewed-by; so:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>



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

Mukesh Rathor

2013-Nov-25 22:39 UTC

head link

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

On Mon, 25 Nov 2013 14:53:51 +0000
George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 11/25/2013 01:25 PM, Jan Beulich wrote:
> >>>> On 19.11.13 at 13:34, Roger Pau Monne
<roger.pau@citrix.com>
> >>>> wrote:
> >> Right now Xen sets the WP and NE flags on cr0 for PVH, which are
> >> not needed in order to boot with paging enabled. The same happens
> >> with cr4, at least on my system OSFXSR, OSXMMEXCPT and MCE are
> >> enabled by default when there's no need.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > So I'm still having this patch sitting in my queue, and I
don't
> > think I've seen an ack from the VMX maintainers or the main PVH
> > contributors. At least one of the two I'd like to have in place
> > before applying.
> 
> This sounds like a good idea to me, but I don't feel like I have a
> firm enough grasp to give a reviewed-by; so:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
Also,

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

thanks
mukesh

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

Dong, Eddie

2013-Nov-26 00:29 UTC

head link

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

Acked-by Eddie Dong <eddie.dong@intel.com>

-----Original Message-----
From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-bounces@lists.xen.org]
On Behalf Of Roger Pau Monne
Sent: Tuesday, November 19, 2013 8:35 PM
To: xen-devel@lists.xenproject.org
Cc: Keir Fraser; George Dunlap; Tim Deegan; Jan Beulich; Roger Pau Monne
Subject: [Xen-devel] [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in
order to use paging

Right now Xen sets the WP and NE flags on cr0 for PVH, which are not needed in
order to boot with paging enabled. The same happens with cr4, at least on my
system OSFXSR, OSXMMEXCPT and MCE are enabled by default when there's no
need.

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/hvm/vmx/vmcs.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index
290b42f..33e2544 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -28,7 +28,6 @@
 #include <asm/msr.h>
 #include <asm/xstate.h>
 #include <asm/hvm/hvm.h>
-#include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/io.h>
 #include <asm/hvm/support.h>
 #include <asm/hvm/vmx/vmx.h>
@@ -1089,13 +1088,11 @@ static int construct_vmcs(struct vcpu *v)
 
     /* PVH domains always start in paging mode */
     if ( is_pvh_domain(d) )
-        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG | X86_CR0_NE |
X86_CR0_WP;
+        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
 
     hvm_update_guest_cr(v, 0);
 
-    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ?
-        (real_cr4_to_pv_guest_cr4(mmu_cr4_features)
-         & ~HVM_CR4_GUEST_RESERVED_BITS(v)) : 0;
+    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_domain(d) ? X86_CR4_PAE : 0;
     hvm_update_guest_cr(v, 4);
 
     if ( cpu_has_vmx_tpr_shadow )
--
1.7.7.5 (Apple Git-26)


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

Xen devel - Nov 2013 - [PATCH 1/2] pvh: proposed BSP/AP bringup changes

[PATCH 1/2] pvh: proposed BSP/AP bringup changes

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

[PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

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

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging

Re: [PATCH 2/2] pvh: set only minimal cr0 and cr4 flags in order to use paging