Skip to contentSkip to conversation

Fix Vista screen clear on AMD

PATCH In review Ben Guthro 3 participants 6 messages Oct 24, 2007 – Oct 25, 2007

lore
Share & tools
lore

Ben Guthro

author
#0 lore raw

PATCH 1/1 Fix Vista screen clear on AMD

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

xen/arch/x86/mm/shadow/common.c +5 -3
diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c
@@ -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);
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw

Hi Ben, Gary,

Quoted 2 lines · Ben Guthro

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.

xen/arch/x86/mm/shadow/common.c line 39

Quoted 8 lines

diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c

xen/arch/x86/mm/shadow/common.c +1 -0
@@ -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.

Quoted 12 lines
xen/arch/x86/mm/shadow/common.c +2 -1
 #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

xen/arch/x86/mm/shadow/common.c shadow_write_p2m_entry(struct vcpu *v, u · line 2740

Quoted 10 lines
xen/arch/x86/mm/shadow/common.c +2 -2
         /* 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.

Signature · 2 lines
Tim Deegan <[email protected]>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508
author
#2 lore raw

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

Quoted 55 lines · Tim Deegan

Hi Ben, Gary,

Quoted 3 lines · Ben Guthro

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.

xen/arch/x86/mm/shadow/common.c line 40

Quoted 9 lines

diff -r 64544443e6d6 xen/arch/x86/mm/shadow/common.c

xen/arch/x86/mm/shadow/common.c +1 -0
@@ -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.

Quoted 13 lines
xen/arch/x86/mm/shadow/common.c +2 -1
 #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

xen/arch/x86/mm/shadow/common.c shadow_write_p2m_entry(struct vcpu *v, u · line 2741

Quoted 10 lines
xen/arch/x86/mm/shadow/common.c +2 -2
          * 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.

Quoted 8 lines

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.

xen/arch/x86/mm/shadow/common.c +4 -3
diff -r 16bf26cdf06b xen/arch/x86/mm/shadow/common.c
@@ -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);
         }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#3 lore raw
Quoted 4 lines · Ben Guthro

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.

Signature · 2 lines
Tim Deegan <[email protected]>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508
#4 lore raw
Quoted 9 lines · Tim Deegan
Quoted 4 lines · Ben Guthro

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.

#5 lore raw

Hi,

Quoted 5 lines · Gary Grebus

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.

Signature · 2 lines
Tim Deegan <[email protected]>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508