* [Qemu-devel] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug
@ 2013-04-04 23:13 Brendan Dolan-Gavitt
2013-04-04 23:25 ` Max Filippov
2013-04-05 13:07 ` [Qemu-devel] [Qemu-trivial] " Stefan Hajnoczi
0 siblings, 2 replies; 3+ messages in thread
From: Brendan Dolan-Gavitt @ 2013-04-04 23:13 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Brendan Dolan-Gavitt
In target-i386 cpu_get_phys_page_debug, the CR4_PAE bit is checked
before CR0_PG. This means that if paging is disabled but the PAE bit has
been set in CR4, cpu_get_phys_page_debug will return the wrong result
(it will try to translate the address as virtual rather than using it as
a physical address). This patch fixes that by moving the CR0_PG check to
the beginning of the function.
This shows up when booting the Linux kernel on amd64 with "-d in_asm".
The kernel turns on the PAE bit in CR4 before turning on paging, and so
QEMU's disassembler will fail because it will try to walk the page
tables to fetch code even though paging is disabled. The symptom is
incorrect disassembly and some "Disassembler disagrees with translator
over instruction decoding" messages.
This was also reported as bug #1163065.
Signed-off-by: Brendan Dolan-Gavitt <brendandg@gatech.edu>
---
target-i386/helper.c | 121 ++++++++++++++++++++++++++------------------------
1 file changed, 64 insertions(+), 57 deletions(-)
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 282494f..0707a44 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -891,70 +891,77 @@ hwaddr cpu_get_phys_page_debug(CPUX86State *env, target_ulong addr)
uint32_t page_offset;
int page_size;
- if (env->cr[4] & CR4_PAE_MASK) {
- target_ulong pdpe_addr;
- uint64_t pde, pdpe;
-
-#ifdef TARGET_X86_64
- if (env->hflags & HF_LMA_MASK) {
- uint64_t pml4e_addr, pml4e;
- int32_t sext;
+ if (!(env->cr[0] & CR0_PG_MASK)) {
+ pte = addr;
+ page_size = 4096;
+ pte = pte & env->a20_mask;
+ } else {
+ if (env->cr[4] & CR4_PAE_MASK) {
+ target_ulong pdpe_addr;
+ uint64_t pde, pdpe;
+
+ #ifdef TARGET_X86_64
+ if (env->hflags & HF_LMA_MASK) {
+ uint64_t pml4e_addr, pml4e;
+ int32_t sext;
+
+ /* test virtual address sign extension */
+ sext = (int64_t)addr >> 47;
+ if (sext != 0 && sext != -1) {
+ return -1;
+ }
- /* test virtual address sign extension */
- sext = (int64_t)addr >> 47;
- if (sext != 0 && sext != -1)
- return -1;
+ pml4e_addr = ((env->cr[3] & ~0xfff) + (((addr >> 39) & 0x1ff) << 3)) &
+ env->a20_mask;
+ pml4e = ldq_phys(pml4e_addr);
+ if (!(pml4e & PG_PRESENT_MASK)) {
+ return -1;
+ }
- pml4e_addr = ((env->cr[3] & ~0xfff) + (((addr >> 39) & 0x1ff) << 3)) &
- env->a20_mask;
- pml4e = ldq_phys(pml4e_addr);
- if (!(pml4e & PG_PRESENT_MASK))
- return -1;
+ pdpe_addr = ((pml4e & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
+ (((addr >> 30) & 0x1ff) << 3)) & env->a20_mask;
+ pdpe = ldq_phys(pdpe_addr);
+ if (!(pdpe & PG_PRESENT_MASK)) {
+ return -1;
+ }
+ } else
+ #endif
+ {
+ pdpe_addr = ((env->cr[3] & ~0x1f) + ((addr >> 27) & 0x18)) &
+ env->a20_mask;
+ pdpe = ldq_phys(pdpe_addr);
+ if (!(pdpe & PG_PRESENT_MASK)) {
+ return -1;
+ }
+ }
- pdpe_addr = ((pml4e & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
- (((addr >> 30) & 0x1ff) << 3)) & env->a20_mask;
- pdpe = ldq_phys(pdpe_addr);
- if (!(pdpe & PG_PRESENT_MASK))
+ pde_addr = ((pdpe & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
+ (((addr >> 21) & 0x1ff) << 3)) & env->a20_mask;
+ pde = ldq_phys(pde_addr);
+ if (!(pde & PG_PRESENT_MASK)) {
return -1;
- } else
-#endif
- {
- pdpe_addr = ((env->cr[3] & ~0x1f) + ((addr >> 27) & 0x18)) &
- env->a20_mask;
- pdpe = ldq_phys(pdpe_addr);
- if (!(pdpe & PG_PRESENT_MASK))
+ }
+ if (pde & PG_PSE_MASK) {
+ /* 2 MB page */
+ page_size = 2048 * 1024;
+ /* align to page_size */
+ pte = pde & ~((page_size - 1) & ~0xfff);
+ } else {
+ /* 4 KB page */
+ pte_addr = ((pde & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
+ (((addr >> 12) & 0x1ff) << 3)) & env->a20_mask;
+ page_size = 4096;
+ pte = ldq_phys(pte_addr);
+ }
+ pte &= ~(PG_NX_MASK | PG_HI_USER_MASK);
+ if (!(pte & PG_PRESENT_MASK))
return -1;
- }
-
- pde_addr = ((pdpe & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
- (((addr >> 21) & 0x1ff) << 3)) & env->a20_mask;
- pde = ldq_phys(pde_addr);
- if (!(pde & PG_PRESENT_MASK)) {
- return -1;
- }
- if (pde & PG_PSE_MASK) {
- /* 2 MB page */
- page_size = 2048 * 1024;
- pte = pde & ~( (page_size - 1) & ~0xfff); /* align to page_size */
} else {
- /* 4 KB page */
- pte_addr = ((pde & ~0xfff & ~(PG_NX_MASK | PG_HI_USER_MASK)) +
- (((addr >> 12) & 0x1ff) << 3)) & env->a20_mask;
- page_size = 4096;
- pte = ldq_phys(pte_addr);
- }
- pte &= ~(PG_NX_MASK | PG_HI_USER_MASK);
- if (!(pte & PG_PRESENT_MASK))
- return -1;
- } else {
- uint32_t pde;
+ uint32_t pde;
- if (!(env->cr[0] & CR0_PG_MASK)) {
- pte = addr;
- page_size = 4096;
- } else {
/* page directory entry */
- pde_addr = ((env->cr[3] & ~0xfff) + ((addr >> 20) & 0xffc)) & env->a20_mask;
+ pde_addr = ((env->cr[3] & ~0xfff) + ((addr >> 20) & 0xffc))
+ & env->a20_mask;
pde = ldl_phys(pde_addr);
if (!(pde & PG_PRESENT_MASK))
return -1;
@@ -969,8 +976,8 @@ hwaddr cpu_get_phys_page_debug(CPUX86State *env, target_ulong addr)
return -1;
page_size = 4096;
}
+ pte = pte & env->a20_mask;
}
- pte = pte & env->a20_mask;
}
page_offset = (addr & TARGET_PAGE_MASK) & (page_size - 1);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug
2013-04-04 23:13 [Qemu-devel] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug Brendan Dolan-Gavitt
@ 2013-04-04 23:25 ` Max Filippov
2013-04-05 13:07 ` [Qemu-devel] [Qemu-trivial] " Stefan Hajnoczi
1 sibling, 0 replies; 3+ messages in thread
From: Max Filippov @ 2013-04-04 23:25 UTC (permalink / raw)
To: Brendan Dolan-Gavitt; +Cc: qemu-trivial, qemu-devel
On Fri, Apr 5, 2013 at 3:13 AM, Brendan Dolan-Gavitt
<brendandg@gatech.edu> wrote:
> In target-i386 cpu_get_phys_page_debug, the CR4_PAE bit is checked
> before CR0_PG. This means that if paging is disabled but the PAE bit has
> been set in CR4, cpu_get_phys_page_debug will return the wrong result
> (it will try to translate the address as virtual rather than using it as
> a physical address). This patch fixes that by moving the CR0_PG check to
> the beginning of the function.
>
> This shows up when booting the Linux kernel on amd64 with "-d in_asm".
> The kernel turns on the PAE bit in CR4 before turning on paging, and so
> QEMU's disassembler will fail because it will try to walk the page
> tables to fetch code even though paging is disabled. The symptom is
> incorrect disassembly and some "Disassembler disagrees with translator
> over instruction decoding" messages.
>
> This was also reported as bug #1163065.
Hi,
a while ago I sent similar patch:
http://comments.gmane.org/gmane.comp.emulators.qemu/180776
and a suggestion for me was to unify cpu_get_phys_page_debug and
cpu_x86_handle_mmu_fault implementations.
--
Thanks.
-- Max
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug
2013-04-04 23:13 [Qemu-devel] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug Brendan Dolan-Gavitt
2013-04-04 23:25 ` Max Filippov
@ 2013-04-05 13:07 ` Stefan Hajnoczi
1 sibling, 0 replies; 3+ messages in thread
From: Stefan Hajnoczi @ 2013-04-05 13:07 UTC (permalink / raw)
To: Brendan Dolan-Gavitt; +Cc: qemu-trivial, qemu-devel
On Thu, Apr 04, 2013 at 07:13:03PM -0400, Brendan Dolan-Gavitt wrote:
> In target-i386 cpu_get_phys_page_debug, the CR4_PAE bit is checked
> before CR0_PG. This means that if paging is disabled but the PAE bit has
> been set in CR4, cpu_get_phys_page_debug will return the wrong result
> (it will try to translate the address as virtual rather than using it as
> a physical address). This patch fixes that by moving the CR0_PG check to
> the beginning of the function.
>
> This shows up when booting the Linux kernel on amd64 with "-d in_asm".
> The kernel turns on the PAE bit in CR4 before turning on paging, and so
> QEMU's disassembler will fail because it will try to walk the page
> tables to fetch code even though paging is disabled. The symptom is
> incorrect disassembly and some "Disassembler disagrees with translator
> over instruction decoding" messages.
>
> This was also reported as bug #1163065.
>
> Signed-off-by: Brendan Dolan-Gavitt <brendandg@gatech.edu>
> ---
> target-i386/helper.c | 121 ++++++++++++++++++++++++++------------------------
> 1 file changed, 64 insertions(+), 57 deletions(-)
Sorry, not trivial :).
Stefan
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-04-05 13:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-04 23:13 [Qemu-devel] [PATCH] target-i386: fix order of checks in cpu_get_phys_page_debug Brendan Dolan-Gavitt
2013-04-04 23:25 ` Max Filippov
2013-04-05 13:07 ` [Qemu-devel] [Qemu-trivial] " Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).