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

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

[Roger Pau Monne]

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.

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

[Roger Pau Monne]

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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

[-- 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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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

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

[Jan Beulich]

>>> 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

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

[Roger Pau Monné]

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

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)


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

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)


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

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

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

[George Dunlap]

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

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

[Roger Pau Monné]

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

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

[Mukesh Rathor]

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

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

[George Dunlap]

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

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

[Tim Deegan]

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.

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

[Roger Pau Monne]

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

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

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[George Dunlap]

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

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

[Mukesh Rathor]

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

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

[Dong, Eddie]

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