[PATCH] Fix Vista screen clear on AMD

[PATCH] Fix Vista screen clear on AMD

[Ben Guthro]

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

Patch to fix horribly slow screen clear during Vista 32 installation
on AMD.

Signed-off-by: Ben Guthro <bguthro@virtualron.com>
Signed-off-by: Gary Grebus <ggrebus@virtualiron.com>

[-- Attachment #2: xen-amd-vista-screen-clear.patch --]
[-- Type: text/x-patch, Size: 1271 bytes --]

diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 11:10:36 2007 -0400
+++ b/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 12:50:46 2007 -0400
@@ -36,6 +36,7 @@
 #include <asm/current.h>
 #include <asm/flushtlb.h>
 #include <asm/shadow.h>
+#include <asm/paging.h>
 #include "private.h"
 
 
@@ -2725,17 +2726,18 @@ shadow_write_p2m_entry(struct vcpu *v, u
     safe_write_pte(p, new);
 
     /* install P2M in monitors for PAE Xen */
-#if CONFIG_PAGING_LEVELS == 3
+#if CONFIG_PAGING_LEVELS >= 3
     if ( level == 3 ) {
         struct vcpu *v;
+#if CONFIG_PAGING_LEVELS == 3
         /* We have written to the p2m l3: need to sync the per-vcpu
          * copies of it in the monitor tables */
         p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
+#endif
         /* Also, any vcpus running on shadows of the p2m need to 
          * reload their CR3s so the change propagates to the shadow */
         for_each_vcpu(d, v) {
-            if ( pagetable_get_pfn(v->arch.guest_table) 
-                 == pagetable_get_pfn(d->arch.phys_table) 
+            if ( likely(!paging_mode_translate(d))
                  && v->arch.paging.mode != NULL )
                 v->arch.paging.mode->update_cr3(v, 0);
         }

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

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

Re: [PATCH] Fix Vista screen clear on AMD

[Tim Deegan]

Hi Ben, Gary,

At 17:07 -0400 on 24 Oct (1193245642), Ben Guthro wrote:
> Patch to fix horribly slow screen clear during Vista 32 installation
> on AMD.

Can you explain what the patch does?  It seems to extend the need for
extra update_cr3() calls to the 64bit hypervisor case and then cause it
never to happen at all.

> diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c
> --- a/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 11:10:36 2007 -0400
> +++ b/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 12:50:46 2007 -0400
> @@ -36,6 +36,7 @@
>  #include <asm/current.h>
>  #include <asm/flushtlb.h>
>  #include <asm/shadow.h>
> +#include <asm/paging.h>

Inside shadow code, it's best to use the shadow_* versions of things.
The paging_* ones will just check for being in shadow mode, which we
know is true.

>  #include "private.h"
>  
>  
> @@ -2725,17 +2726,18 @@ shadow_write_p2m_entry(struct vcpu *v, u
>      safe_write_pte(p, new);
>  
>      /* install P2M in monitors for PAE Xen */
> -#if CONFIG_PAGING_LEVELS == 3
> +#if CONFIG_PAGING_LEVELS >= 3
>      if ( level == 3 ) {
>          struct vcpu *v;
> +#if CONFIG_PAGING_LEVELS == 3
>          /* We have written to the p2m l3: need to sync the per-vcpu
>           * copies of it in the monitor tables */
>          p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
> +#endif
>          /* Also, any vcpus running on shadows of the p2m need to 
>           * reload their CR3s so the change propagates to the shadow */
>          for_each_vcpu(d, v) {
> -            if ( pagetable_get_pfn(v->arch.guest_table) 
> -                 == pagetable_get_pfn(d->arch.phys_table) 
> +            if ( likely(!paging_mode_translate(d))

This test will never be true; if you aren't in paging_mode_translate()
you won't be writing p2m entries in the first place.

That said, I think it's probably right to just get rid of the call
entirely, since we don't shadow the p2m any more. 

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508

Re: [PATCH] Fix Vista screen clear on AMD

[Ben Guthro]

[-- Attachment #1.1: Type: text/plain, Size: 2450 bytes --]

hmm,

Its certainly possiible that I ported this incorrectly to unstable. I 
have attached the original patch that applies to 3.1.1 for your review. 
As for the details on why this patch was written this way - I will let 
the original author field that (Gary)

If this is not necessary anymore, as you said below - perhaps this code 
should be removed entirely?

Ben

Tim Deegan wrote:
> Hi Ben, Gary,
>
> At 17:07 -0400 on 24 Oct (1193245642), Ben Guthro wrote:
>   
>> Patch to fix horribly slow screen clear during Vista 32 installation
>> on AMD.
>>     
>
> Can you explain what the patch does?  It seems to extend the need for
> extra update_cr3() calls to the 64bit hypervisor case and then cause it
> never to happen at all.
>
>   
>> diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c
>> --- a/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 11:10:36 2007 -0400
>> +++ b/xen/arch/x86/mm/shadow/common.c	Wed Oct 10 12:50:46 2007 -0400
>> @@ -36,6 +36,7 @@
>>  #include <asm/current.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/shadow.h>
>> +#include <asm/paging.h>
>>     
>
> Inside shadow code, it's best to use the shadow_* versions of things.
> The paging_* ones will just check for being in shadow mode, which we
> know is true.
>
>   
>>  #include "private.h"
>>  
>>  
>> @@ -2725,17 +2726,18 @@ shadow_write_p2m_entry(struct vcpu *v, u
>>      safe_write_pte(p, new);
>>  
>>      /* install P2M in monitors for PAE Xen */
>> -#if CONFIG_PAGING_LEVELS == 3
>> +#if CONFIG_PAGING_LEVELS >= 3
>>      if ( level == 3 ) {
>>          struct vcpu *v;
>> +#if CONFIG_PAGING_LEVELS == 3
>>          /* We have written to the p2m l3: need to sync the per-vcpu
>>           * copies of it in the monitor tables */
>>          p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
>> +#endif
>>          /* Also, any vcpus running on shadows of the p2m need to 
>>           * reload their CR3s so the change propagates to the shadow */
>>          for_each_vcpu(d, v) {
>> -            if ( pagetable_get_pfn(v->arch.guest_table) 
>> -                 == pagetable_get_pfn(d->arch.phys_table) 
>> +            if ( likely(!paging_mode_translate(d))
>>     
>
> This test will never be true; if you aren't in paging_mode_translate()
> you won't be writing p2m entries in the first place.
>
> That said, I think it's probably right to just get rid of the call
> entirely, since we don't shadow the p2m any more. 
>
> Cheers,
>
> Tim.
>
>   


[-- Attachment #1.2: Type: text/html, Size: 2979 bytes --]

[-- Attachment #2: xen-amd-vista-screen-clear.patch --]
[-- Type: text/x-patch, Size: 1174 bytes --]

diff -r 16bf26cdf06b xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c	Tue Jul 24 16:49:50 2007 -0400
+++ b/xen/arch/x86/mm/shadow/common.c	Tue Jul 24 16:49:53 2007 -0400
@@ -2891,17 +2891,18 @@ shadow_write_p2m_entry(struct vcpu *v, u
         (void)sh_validate_guest_entry(d->vcpu[0], table_mfn, p, sizeof(*p));
 
     /* install P2M in monitors for PAE Xen */
-#if CONFIG_PAGING_LEVELS == 3
+#if CONFIG_PAGING_LEVELS >= 3
     if ( level == 3 ) {
         struct vcpu *v;
+#if CONFIG_PAGING_LEVELS == 3
         /* We have written to the p2m l3: need to sync the per-vcpu
          * copies of it in the monitor tables */
         p2m_install_entry_in_monitors(d, (l3_pgentry_t *)p);
+#endif
         /* Also, any vcpus running on shadows of the p2m need to 
          * reload their CR3s so the change propagates to the shadow */
         for_each_vcpu(d, v) {
-            if ( pagetable_get_pfn(v->arch.guest_table) 
-                 == pagetable_get_pfn(d->arch.phys_table) 
+            if ( ! v->arch.paging.translate_enabled
                  && v->arch.paging.mode != NULL )
                 v->arch.paging.mode->update_cr3(v, 0);
         }

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

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

Re: [PATCH] Fix Vista screen clear on AMD

[Tim Deegan]

At 07:25 -0400 on 25 Oct (1193297134), Ben Guthro wrote:
> hmm,
> 
> Its certainly possiible that I ported this incorrectly to unstable. I 
> have attached the original patch that applies to 3.1.1 for your review. 

Yep, that's it; there is no equivalent of v->arch.paging.translate_enabled
in -unstable any more since I got rid of the shadowing of the p2m and
the idea of vcpus that don't need p2m translation.

Tim.

-- 
Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508

Re: [PATCH] Fix Vista screen clear on AMD

[Gary Grebus]

On Thu, 2007-10-25 at 12:34 +0100, Tim Deegan wrote:
> At 07:25 -0400 on 25 Oct (1193297134), Ben Guthro wrote:
> > hmm,
> > 
> > Its certainly possiible that I ported this incorrectly to unstable. I 
> > have attached the original patch that applies to 3.1.1 for your review. 
> 
> Yep, that's it; there is no equivalent of v->arch.paging.translate_enabled
> in -unstable any more since I got rid of the shadowing of the p2m and
> the idea of vcpus that don't need p2m translation.

OK... sounds like this isn't needed for unstable.

The symptom was that the Vista bootloader (in protected mode but not
paging enabled) was taking a VMEXIT for every access to the VGA
framebuffer.  The PCI address region had been mapped in the p2m, but
that change wasn't being reflected to the shadow.  I never tracked down
why this only happened for AMD.

	/gary
--
Gary Grebus, Virtual Iron Software Inc.

Re: [PATCH] Fix Vista screen clear on AMD

[Tim Deegan]

Hi,

At 10:13 -0400 on 25 Oct (1193307212), Gary Grebus wrote:
> The symptom was that the Vista bootloader (in protected mode but not
> paging enabled) was taking a VMEXIT for every access to the VGA
> framebuffer.  The PCI address region had been mapped in the p2m, but
> that change wasn't being reflected to the shadow.  I never tracked down
> why this only happened for AMD.

Righto.  Yes, that's not needed any more since we stopped shadowing the
p2m directly.

Thanks,

Tim.

-- 
Tim Deegan <Tim.Deegan@xensource.com>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508