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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.14 at 18:42, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx>
> Cc: Jan Beulich <JBeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel