Re: [Xen-devel] [PATCH] support 1gb pages in guest page table walker

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 14:11 +0200 on 26 Jul (1311689501), Christoph Egger wrote:
> On 07/25/11 12:30, Tim Deegan wrote:
> >At 11:25 +0100 on 25 Jul (1311593146), Tim Deegan wrote:
> >>Also this function should be called from the CPUID trap handler to make
> >>sure we never advertise PSE1GB when we're not going to support it.
> >
> >Er, not this function exactly, since CPUID should report the feature
> >even when the guest's not in long_mode.  I think it needs a
> >hvm_pse1G_supported that can be called from CPUID, and then
> >guest_supports_1G_superpages() boils down to
> >"(GUEST_PAGING_LEVEL>= 4)&&  hvm_pse1G_supported(v)"
> 
> New version attached. I removed the fake l1e calculation.

If you just remove it, you need to update hap_p2m_ga_to_gfn to figure
out the GFN some other way!  I expect that's what's causing your
problem.  You should either provide the fake l1e, and say why in the
comment, or audit all callers of the function to make sure they don't
need it.

> @@ -2444,9 +2445,13 @@ void hvm_cpuid(unsigned int input, unsig
>      case 0x80000001:
>          /* We expose RDTSCP feature to guest only when
>             tsc_mode == TSC_MODE_DEFAULT and host_tsc_is_safe() returns 1 */
> -        if ( v->domain->arch.tsc_mode != TSC_MODE_DEFAULT ||
> +        if ( d->arch.tsc_mode != TSC_MODE_DEFAULT ||
>               !host_tsc_is_safe() )
>              *edx &= ~cpufeat_mask(X86_FEATURE_RDTSCP);
> +        /* Expose 1gb page feature for HVM HAP guests and hw support is
> +         * available. */
> +        if (hvm_pse1gb_supported(d))
> +            *edx |= cpufeat_mask(X86_FEATURE_PAGE1GB);

I asked for the opposite of this: the flag must be _cleared_ if the
feature's _not_ supported.  It's really up to the userspace tools to
decide whether they want the flag set in the first place.

> diff -r 4f2c59fb28e6 -r 6d15152fb59a xen/include/asm-x86/guest_pt.h
> --- a/xen/include/asm-x86/guest_pt.h
> +++ b/xen/include/asm-x86/guest_pt.h
> @@ -194,6 +194,17 @@ guest_supports_superpages(struct vcpu *v
>  }
>  
>  static inline int
> +guest_supports_1G_superpages(struct vcpu *v)
> +{
> +    if (!guest_supports_superpages(v))
> +        return 0;
> +
> +    return (GUEST_PAGING_LEVELS >= 4
> +           && hvm_pse1gb_supported(v->domain)
> +           && hvm_long_mode_enabled(v));

You don't need to check GUEST_PAGING_LEVELS and hvm_long_mode_enabled();
this file is compiled multiple times so they're always the same on any
given code path.  Check only GUEST_PAGING_LEVELS.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel