[PATCH RFC] xen: fix cpuid reporting on PVH Dom0

[PATCH RFC] xen: fix cpuid reporting on PVH Dom0

[Roger Pau Monne]

dab11417d also caused some problems regarding HVM guest creation on
PVH Dom0, mainly the CR4 mask returned by hvm_cr4_guest_reserved_bits
changed from 0xfffffffffffff800 to 0xfffffffffffff893, which means HVM
guests created from a PVH Dom0 are unable to set VME, PVI, PSE or PGE
CR4 flags.

This is because cpuid on PVH guests mask PSE, PGE, PSE36 and VME
flags, so the white listing done in xc_cpuid_hvm_policy doesn't enable
those features, and the guest ends up with a very restrictive cpuid
policy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
TBH, I'm not sure what's the best way to fix this, the cpuid stuff is
so convoluted and it's done in so many different places that I've
probably missed something.
---
 xen/arch/x86/traps.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 677074b..0a46f75 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -803,12 +803,17 @@ void pv_cpuid(struct cpu_user_regs *regs)
     if ( (regs->eax & 0x7fffffff) == 0x00000001 )
     {
         /* Modify Feature Information. */
-        __clear_bit(X86_FEATURE_VME, &d);
         if ( !cpu_has_apic )
             __clear_bit(X86_FEATURE_APIC, &d);
-        __clear_bit(X86_FEATURE_PSE, &d);
-        __clear_bit(X86_FEATURE_PGE, &d);
-        __clear_bit(X86_FEATURE_PSE36, &d);
+        if ( !is_pvh_vcpu(curr) || !is_control_domain(curr->domain) ||
+             !is_hardware_domain(curr->domain) )
+        {
+            __clear_bit(X86_FEATURE_PSE, &d);
+            __clear_bit(X86_FEATURE_PGE, &d);
+            __clear_bit(X86_FEATURE_PSE36, &d);
+            __clear_bit(X86_FEATURE_VME, &d);
+        }
+
     }
 
     switch ( (uint32_t)regs->eax )
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH RFC] xen: fix cpuid reporting on PVH Dom0

[Andrew Cooper]

On 24/07/14 17:42, Roger Pau Monne wrote:
> dab11417d also caused some problems regarding HVM guest creation on
> PVH Dom0, mainly the CR4 mask returned by hvm_cr4_guest_reserved_bits
> changed from 0xfffffffffffff800 to 0xfffffffffffff893, which means HVM
> guests created from a PVH Dom0 are unable to set VME, PVI, PSE or PGE
> CR4 flags.
>
> This is because cpuid on PVH guests mask PSE, PGE, PSE36 and VME
> flags, so the white listing done in xc_cpuid_hvm_policy doesn't enable
> those features, and the guest ends up with a very restrictive cpuid
> policy.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> TBH, I'm not sure what's the best way to fix this, the cpuid stuff is
> so convoluted and it's done in so many different places that I've
> probably missed something.

The whole cpuid setup is a mess which I plan to address once migration
v2 is sorted.

For now, my best suggestion is hack it until it works.

> ---
>  xen/arch/x86/traps.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 677074b..0a46f75 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -803,12 +803,17 @@ void pv_cpuid(struct cpu_user_regs *regs)
>      if ( (regs->eax & 0x7fffffff) == 0x00000001 )
>      {
>          /* Modify Feature Information. */
> -        __clear_bit(X86_FEATURE_VME, &d);
>          if ( !cpu_has_apic )
>              __clear_bit(X86_FEATURE_APIC, &d);
> -        __clear_bit(X86_FEATURE_PSE, &d);
> -        __clear_bit(X86_FEATURE_PGE, &d);
> -        __clear_bit(X86_FEATURE_PSE36, &d);
> +        if ( !is_pvh_vcpu(curr) || !is_control_domain(curr->domain) ||
> +             !is_hardware_domain(curr->domain) )

I am not really sure what the expected difference between
is_control_domain() and is_hardware_domain() actually is, but they are
synonymous for anyone not using hardware_dom=<something other than 0>

I suspect that is_control_domain() is irrelevant here however.

~Andrew

> +        {
> +            __clear_bit(X86_FEATURE_PSE, &d);
> +            __clear_bit(X86_FEATURE_PGE, &d);
> +            __clear_bit(X86_FEATURE_PSE36, &d);
> +            __clear_bit(X86_FEATURE_VME, &d);
> +        }
> +
>      }
>  
>      switch ( (uint32_t)regs->eax )


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

Re: [PATCH RFC] xen: fix cpuid reporting on PVH Dom0

[Jan Beulich]

>>> On 24.07.14 at 18:42, <roger.pau@citrix.com> wrote:
> dab11417d also caused some problems regarding HVM guest creation on
> PVH Dom0, mainly the CR4 mask returned by hvm_cr4_guest_reserved_bits
> changed from 0xfffffffffffff800 to 0xfffffffffffff893, which means HVM
> guests created from a PVH Dom0 are unable to set VME, PVI, PSE or PGE
> CR4 flags.
> 
> This is because cpuid on PVH guests mask PSE, PGE, PSE36 and VME
> flags, so the white listing done in xc_cpuid_hvm_policy doesn't enable
> those features, and the guest ends up with a very restrictive cpuid
> policy.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> TBH, I'm not sure what's the best way to fix this, the cpuid stuff is
> so convoluted and it's done in so many different places that I've
> probably missed something.
> ---
>  xen/arch/x86/traps.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 677074b..0a46f75 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -803,12 +803,17 @@ void pv_cpuid(struct cpu_user_regs *regs)
>      if ( (regs->eax & 0x7fffffff) == 0x00000001 )
>      {
>          /* Modify Feature Information. */
> -        __clear_bit(X86_FEATURE_VME, &d);
>          if ( !cpu_has_apic )
>              __clear_bit(X86_FEATURE_APIC, &d);
> -        __clear_bit(X86_FEATURE_PSE, &d);
> -        __clear_bit(X86_FEATURE_PGE, &d);
> -        __clear_bit(X86_FEATURE_PSE36, &d);
> +        if ( !is_pvh_vcpu(curr) || !is_control_domain(curr->domain) ||
> +             !is_hardware_domain(curr->domain) )

Don't you rather mean

        if ( !is_pvh_vcpu(curr) || (!is_control_domain(curr->domain) &&
             !is_hardware_domain(curr->domain)) )

in which case, considering earlier logic in this function, this just
becomes

        if ( !is_pvh_vcpu(curr) )

? And I can't see anyway why the control/hardware domain
property would matter for any of these features, so I think even
from a logical standpoint it should just be the latter.

Jan

> +        {
> +            __clear_bit(X86_FEATURE_PSE, &d);
> +            __clear_bit(X86_FEATURE_PGE, &d);
> +            __clear_bit(X86_FEATURE_PSE36, &d);
> +            __clear_bit(X86_FEATURE_VME, &d);
> +        }
> +
>      }
>  
>      switch ( (uint32_t)regs->eax )
> -- 
> 1.7.7.5 (Apple Git-26)



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