From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Guthro Subject: Re: [PATCH] Fix Vista screen clear on AMD Date: Thu, 25 Oct 2007 07:25:34 -0400 Message-ID: <47207D2E.8070209@virtualiron.com> References: <471FB40A.2020405@virtualiron.com> <20071025091630.GE23658@york.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070700030701070309070106" Return-path: In-Reply-To: <20071025091630.GE23658@york.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Tim Deegan Cc: Gary Grebus , xen-devel List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------070700030701070309070106 Content-Type: multipart/alternative; boundary="------------080503000400010003030401" --------------080503000400010003030401 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 >> #include >> #include >> +#include >> > > 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. > > --------------080503000400010003030401 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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.

--------------080503000400010003030401-- --------------070700030701070309070106 Content-Type: text/x-patch; name="xen-amd-vista-screen-clear.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="xen-amd-vista-screen-clear.patch" 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); } --------------070700030701070309070106 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------070700030701070309070106--