Xen devel - Jul 2011 - [PATCH] support 1gb pages in guest page table walker

Support 1gb pages in guest page table walker.
This is needed for superpage support for
nested virtualization.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

>>> On 22.07.11 at 16:51, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> Support 1gb pages in guest page table walker.
Why do you do this for GUEST_PAGING_LEVELS >= 3 rather than
GUEST_PAGING_LEVELS >= 4?

Jan
> This is needed for superpage support for
> nested virtualization.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>




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

On 07/22/11 17:43, Jan Beulich wrote:
>>>> On 22.07.11 at 16:51, Christoph
Egger<Christoph.Egger@amd.com>  wrote:
>> Support 1gb pages in guest page table walker.
>
> Why do you do this for GUEST_PAGING_LEVELS>= 3 rather than
> GUEST_PAGING_LEVELS>= 4?
Xen can run in 32bit PAE mode but the guest in 64bit mode.
We can still change this when 32bit support is actually dropped.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

>>> On 25.07.11 at 11:19, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> On 07/22/11 17:43, Jan Beulich wrote:
>>>>> On 22.07.11 at 16:51, Christoph
Egger<Christoph.Egger@amd.com>  wrote:
>>> Support 1gb pages in guest page table walker.
>>
>> Why do you do this for GUEST_PAGING_LEVELS>= 3 rather than
>> GUEST_PAGING_LEVELS>= 4?
> 
> Xen can run in 32bit PAE mode but the guest in 64bit mode.
But that is what distinguishes GUEST_PAGING_LEVELS from
CONFIG_PAGING_LEVELS - you''re allowing a PAE *guest* to use
1Gb mappings, no matter what host it runs on.

Jan
> We can still change this when 32bit support is actually dropped.
> 
> Christoph
> 




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

On 07/25/11 11:31, Jan Beulich wrote:
>>>> On 25.07.11 at 11:19, Christoph
Egger<Christoph.Egger@amd.com>  wrote:
>> On 07/22/11 17:43, Jan Beulich wrote:
>>>>>> On 22.07.11 at 16:51, Christoph
Egger<Christoph.Egger@amd.com>   wrote:
>>>> Support 1gb pages in guest page table walker.
>>>
>>> Why do you do this for GUEST_PAGING_LEVELS>= 3 rather than
>>> GUEST_PAGING_LEVELS>= 4?
>>
>> Xen can run in 32bit PAE mode but the guest in 64bit mode.
>
> But that is what distinguishes GUEST_PAGING_LEVELS from
> CONFIG_PAGING_LEVELS - you''re allowing a PAE *guest* to use
> 1Gb mappings, no matter what host it runs on.
>
Oh, then I mixed those two up. New fixed patch attached.
Thank you for your review.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

Hi, 

Thanks for this.  Do you have a matching patch that enables 1GB mappings
in the appropriate guests?

At 11:46 +0200 on 25 Jul (1311594401), Christoph Egger
wrote:
> +#if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
> +    pse1G = (guest_supports_1G_superpages(v) && 
> +           (guest_l3e_get_flags(gw->l3e) & _PAGE_PSE)); 
> +
> +    if ( pse1G )
> +    {
> +        /* Special case: this guest VA is in a PSE superpage, so
there''s
> +         * no guest l1e.  We make one up so that the propagation code
> +         * can generate a shadow l1 table.  Start with the gfn of the 
> +         * first 4k-page of the superpage. */
Shadow pagetables don''t support 1GB mappings, so this comment needs to
be updated to say why you''re making the fake l1e (basically as a
vehicle
for the GFN of the target frame).
> +        gfn_t start = guest_l2e_get_gfn(gw->l2e);
> +        /* Grant full access in the l1e, since all the guest
entry''s
> +         * access controls are enforced in the shadow l2e. */
> +        int flags = (_PAGE_PRESENT|_PAGE_USER|_PAGE_RW|
> +                     _PAGE_ACCESSED|_PAGE_DIRTY);
> +        /* Import cache-control bits. Note that _PAGE_PAT is actually
> +         * _PAGE_PSE, and it is always set. We will clear it in case
> +         * _PAGE_PSE_PAT (bit 12, i.e. first bit of gfn) is clear. */
> +        flags |= (guest_l2e_get_flags(gw->l2e)
> +                  & (_PAGE_PAT|_PAGE_PWT|_PAGE_PCD));
You should be taking these from the l3e, not the l2e.  Also, you should
probably do it before the code that maps the l2 table. :)
> diff -r 4f2c59fb28e6 xen/include/asm-x86/guest_pt.h
> --- a/xen/include/asm-x86/guest_pt.h	Mon Jul 18 11:43:45 2011 +0200
> +++ b/xen/include/asm-x86/guest_pt.h	Mon Jul 25 11:43:36 2011 +0200
> @@ -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
> +           && cpu_has_page1gb
> +           && hvm_long_mode_enabled(v));
There needs to be some interlock to make sure this returns false for
all guests except HVM HAP ones.  

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.  
(I think that involves moving it out of this header file -- it should
probably go into hvm.h and be renamed to match the hvm_*_enabled macros
already there.)

Cheers,

Tim.

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

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

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

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

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

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.
Something is still wrong: The l2 guest kernel crashes with
an unexpected null pointer derefencing but it boots w/o
this patch.

Is it a problem when l1 guest uses 1gb pages where the host
uses 4k or 2m pages?

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

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@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

On 07/26/11 15:19, Tim Deegan wrote:
> 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.
Ah, I see now. I misunderstood you.

I forgot to increase the pfn by the right number of 2m pages for
the l1e calculation.

Now it works for me: The l2 guest boots and passes my tests.



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


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

At 15:15 +0200 on 27 Jul (1311779749), Christoph Egger
wrote:
> # HG changeset patch
> # User cegger
> # Date 1311772079 -7200
> support 1gb pages for guests
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
Thanks.  Applied, with some changes, as 23755:ba78ea7784c9

Tim.

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

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