* [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses
@ 2013-05-28 18:19 Luiz Capitulino
2013-05-30 12:59 ` Luiz Capitulino
2013-05-30 14:29 ` Andreas Färber
0 siblings, 2 replies; 9+ messages in thread
From: Luiz Capitulino @ 2013-05-28 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: qiaonuohan, afaerber, qemu-stable
The code used to walk IA-32e page-tables, and possibly PAE page-tables,
uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address.
However, as we use a uint64_t to store the resulting address, that mask
gets expanded to 0xfffffffffffff000 which not only ends up selecting
reserved bits but also selects the XD bit (execute-disable) which
happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort.
This commit fixes that problem by replacing ~0xfff by a correct mask
that only selects the address bit range (ie. bits 51:12).
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
PS: I (obviously) don't any more core dumps with this patch applied, but
I couldn't check if the Windows dump is correct (does anyone know
how to do this?). I did quickly check on Linux though.
target-i386/arch_memory_mapping.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c
index 844893f..24884bd 100644
--- a/target-i386/arch_memory_mapping.c
+++ b/target-i386/arch_memory_mapping.c
@@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list,
}
/* PAE Paging or IA-32e Paging */
+#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */
+
static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
int32_t a20_mask, target_ulong start_line_addr)
{
@@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr,
continue;
}
- pte_start_addr = (pde & ~0xfff) & a20_mask;
+ pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask;
walk_pte(list, pte_start_addr, a20_mask, line_addr);
}
}
@@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list,
continue;
}
- pde_start_addr = (pdpe & ~0xfff) & a20_mask;
+ pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask;
walk_pde(list, pde_start_addr, a20_mask, line_addr);
}
}
@@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list,
}
line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48);
- pdpe_start_addr = (pml4e & ~0xfff) & a20_mask;
+ pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask;
walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr);
}
}
@@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env)
if (env->hflags & HF_LMA_MASK) {
hwaddr pml4e_addr;
- pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask;
+ pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask;
walk_pml4e(list, pml4e_addr, env->a20_mask);
} else
#endif
--
1.8.1.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-28 18:19 [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses Luiz Capitulino @ 2013-05-30 12:59 ` Luiz Capitulino 2013-05-30 13:16 ` Laszlo Ersek 2013-05-30 14:29 ` Andreas Färber 1 sibling, 1 reply; 9+ messages in thread From: Luiz Capitulino @ 2013-05-30 12:59 UTC (permalink / raw) To: wency; +Cc: qiaonuohan, qemu-stable, qemu-devel, afaerber On Tue, 28 May 2013 14:19:22 -0400 Luiz Capitulino <lcapitulino@redhat.com> wrote: > The code used to walk IA-32e page-tables, and possibly PAE page-tables, > uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. > > However, as we use a uint64_t to store the resulting address, that mask > gets expanded to 0xfffffffffffff000 which not only ends up selecting > reserved bits but also selects the XD bit (execute-disable) which > happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. > > This commit fixes that problem by replacing ~0xfff by a correct mask > that only selects the address bit range (ie. bits 51:12). > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> Ping? Wen? Would be nice get a Reviewed-by before merging... > --- > > PS: I (obviously) don't any more core dumps with this patch applied, but > I couldn't check if the Windows dump is correct (does anyone know > how to do this?). I did quickly check on Linux though. > > target-i386/arch_memory_mapping.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c > index 844893f..24884bd 100644 > --- a/target-i386/arch_memory_mapping.c > +++ b/target-i386/arch_memory_mapping.c > @@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list, > } > > /* PAE Paging or IA-32e Paging */ > +#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */ > + > static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, > int32_t a20_mask, target_ulong start_line_addr) > { > @@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, > continue; > } > > - pte_start_addr = (pde & ~0xfff) & a20_mask; > + pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask; > walk_pte(list, pte_start_addr, a20_mask, line_addr); > } > } > @@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list, > continue; > } > > - pde_start_addr = (pdpe & ~0xfff) & a20_mask; > + pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask; > walk_pde(list, pde_start_addr, a20_mask, line_addr); > } > } > @@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list, > } > > line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48); > - pdpe_start_addr = (pml4e & ~0xfff) & a20_mask; > + pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask; > walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr); > } > } > @@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env) > if (env->hflags & HF_LMA_MASK) { > hwaddr pml4e_addr; > > - pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask; > + pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask; > walk_pml4e(list, pml4e_addr, env->a20_mask); > } else > #endif ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 12:59 ` Luiz Capitulino @ 2013-05-30 13:16 ` Laszlo Ersek 2013-05-30 13:16 ` Luiz Capitulino 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2013-05-30 13:16 UTC (permalink / raw) To: Luiz Capitulino; +Cc: qiaonuohan, afaerber, qemu-stable, qemu-devel On 05/30/13 14:59, Luiz Capitulino wrote: > On Tue, 28 May 2013 14:19:22 -0400 > Luiz Capitulino <lcapitulino@redhat.com> wrote: > >> The code used to walk IA-32e page-tables, and possibly PAE page-tables, >> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. >> >> However, as we use a uint64_t to store the resulting address, that mask >> gets expanded to 0xfffffffffffff000 which not only ends up selecting >> reserved bits but also selects the XD bit (execute-disable) which >> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. >> >> This commit fixes that problem by replacing ~0xfff by a correct mask >> that only selects the address bit range (ie. bits 51:12). >> >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > Ping? Wen? > > Would be nice get a Reviewed-by before merging... I didn't miss your submission and did find it OK, I just felt unsure about stating so, because "simple" patches like this are prime territory to burn someone's R-b's worth (ie. to expose a reviewer's lack of information / experience). But hey, what can I lose? The patch does look good to me, so Reviewed-by: Laszlo Ersek <lersek@redhat.com> > >> --- >> >> PS: I (obviously) don't any more core dumps with this patch applied, but >> I couldn't check if the Windows dump is correct (does anyone know >> how to do this?). I did quickly check on Linux though. >> >> target-i386/arch_memory_mapping.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c >> index 844893f..24884bd 100644 >> --- a/target-i386/arch_memory_mapping.c >> +++ b/target-i386/arch_memory_mapping.c >> @@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list, >> } >> >> /* PAE Paging or IA-32e Paging */ >> +#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */ >> + >> static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, >> int32_t a20_mask, target_ulong start_line_addr) >> { >> @@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, >> continue; >> } >> >> - pte_start_addr = (pde & ~0xfff) & a20_mask; >> + pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask; >> walk_pte(list, pte_start_addr, a20_mask, line_addr); >> } >> } >> @@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list, >> continue; >> } >> >> - pde_start_addr = (pdpe & ~0xfff) & a20_mask; >> + pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask; >> walk_pde(list, pde_start_addr, a20_mask, line_addr); >> } >> } >> @@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list, >> } >> >> line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48); >> - pdpe_start_addr = (pml4e & ~0xfff) & a20_mask; >> + pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask; >> walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr); >> } >> } >> @@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env) >> if (env->hflags & HF_LMA_MASK) { >> hwaddr pml4e_addr; >> >> - pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask; >> + pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask; >> walk_pml4e(list, pml4e_addr, env->a20_mask); >> } else >> #endif > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 13:16 ` Laszlo Ersek @ 2013-05-30 13:16 ` Luiz Capitulino 2013-05-30 14:10 ` Andreas Färber 0 siblings, 1 reply; 9+ messages in thread From: Luiz Capitulino @ 2013-05-30 13:16 UTC (permalink / raw) To: Laszlo Ersek; +Cc: qiaonuohan, afaerber, qemu-stable, qemu-devel On Thu, 30 May 2013 15:16:18 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 05/30/13 14:59, Luiz Capitulino wrote: > > On Tue, 28 May 2013 14:19:22 -0400 > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > >> The code used to walk IA-32e page-tables, and possibly PAE page-tables, > >> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. > >> > >> However, as we use a uint64_t to store the resulting address, that mask > >> gets expanded to 0xfffffffffffff000 which not only ends up selecting > >> reserved bits but also selects the XD bit (execute-disable) which > >> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. > >> > >> This commit fixes that problem by replacing ~0xfff by a correct mask > >> that only selects the address bit range (ie. bits 51:12). > >> > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > > > Ping? Wen? > > > > Would be nice get a Reviewed-by before merging... > > I didn't miss your submission and did find it OK, I just felt unsure > about stating so, because "simple" patches like this are prime territory > to burn someone's R-b's worth (ie. to expose a reviewer's lack of > information / experience). But hey, what can I lose? The patch does look > good to me, so Thank you Laszlo! It's also new territory for me, that's why I'm asking for reviews (otherwise I'd just sneak it in some pull request :-) > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > > > >> --- > >> > >> PS: I (obviously) don't any more core dumps with this patch applied, but > >> I couldn't check if the Windows dump is correct (does anyone know > >> how to do this?). I did quickly check on Linux though. > >> > >> target-i386/arch_memory_mapping.c | 10 ++++++---- > >> 1 file changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c > >> index 844893f..24884bd 100644 > >> --- a/target-i386/arch_memory_mapping.c > >> +++ b/target-i386/arch_memory_mapping.c > >> @@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list, > >> } > >> > >> /* PAE Paging or IA-32e Paging */ > >> +#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */ > >> + > >> static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, > >> int32_t a20_mask, target_ulong start_line_addr) > >> { > >> @@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, > >> continue; > >> } > >> > >> - pte_start_addr = (pde & ~0xfff) & a20_mask; > >> + pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask; > >> walk_pte(list, pte_start_addr, a20_mask, line_addr); > >> } > >> } > >> @@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list, > >> continue; > >> } > >> > >> - pde_start_addr = (pdpe & ~0xfff) & a20_mask; > >> + pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask; > >> walk_pde(list, pde_start_addr, a20_mask, line_addr); > >> } > >> } > >> @@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list, > >> } > >> > >> line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48); > >> - pdpe_start_addr = (pml4e & ~0xfff) & a20_mask; > >> + pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask; > >> walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr); > >> } > >> } > >> @@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env) > >> if (env->hflags & HF_LMA_MASK) { > >> hwaddr pml4e_addr; > >> > >> - pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask; > >> + pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask; > >> walk_pml4e(list, pml4e_addr, env->a20_mask); > >> } else > >> #endif > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 13:16 ` Luiz Capitulino @ 2013-05-30 14:10 ` Andreas Färber 2013-05-30 14:14 ` Luiz Capitulino 0 siblings, 1 reply; 9+ messages in thread From: Andreas Färber @ 2013-05-30 14:10 UTC (permalink / raw) To: Luiz Capitulino; +Cc: qiaonuohan, Laszlo Ersek, qemu-stable, qemu-devel Am 30.05.2013 15:16, schrieb Luiz Capitulino: > On Thu, 30 May 2013 15:16:18 +0200 > Laszlo Ersek <lersek@redhat.com> wrote: > >> On 05/30/13 14:59, Luiz Capitulino wrote: >>> On Tue, 28 May 2013 14:19:22 -0400 >>> Luiz Capitulino <lcapitulino@redhat.com> wrote: >>> >>>> The code used to walk IA-32e page-tables, and possibly PAE page-tables, >>>> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. >>>> >>>> However, as we use a uint64_t to store the resulting address, that mask >>>> gets expanded to 0xfffffffffffff000 which not only ends up selecting >>>> reserved bits but also selects the XD bit (execute-disable) which >>>> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. >>>> >>>> This commit fixes that problem by replacing ~0xfff by a correct mask >>>> that only selects the address bit range (ie. bits 51:12). >>>> >>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>> >>> Ping? Wen? >>> >>> Would be nice get a Reviewed-by before merging... >> >> I didn't miss your submission and did find it OK, I just felt unsure >> about stating so, because "simple" patches like this are prime territory >> to burn someone's R-b's worth (ie. to expose a reviewer's lack of >> information / experience). But hey, what can I lose? The patch does look >> good to me, so > > Thank you Laszlo! It's also new territory for me, that's why I'm asking > for reviews (otherwise I'd just sneak it in some pull request :-) Luiz, you aware aware that I have another fix by Nuohan queued that seemed orthogonal? If someone reviews my refactoring series (which resent that patch) I would like to send out a PULL for that rather soon, since it blocks further CPU work. I would then include your fix as well to avoid merge conflicts. Andreas > >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >>> >>>> --- >>>> >>>> PS: I (obviously) don't any more core dumps with this patch applied, but >>>> I couldn't check if the Windows dump is correct (does anyone know >>>> how to do this?). I did quickly check on Linux though. >>>> >>>> target-i386/arch_memory_mapping.c | 10 ++++++---- >>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/target-i386/arch_memory_mapping.c b/target-i386/arch_memory_mapping.c >>>> index 844893f..24884bd 100644 >>>> --- a/target-i386/arch_memory_mapping.c >>>> +++ b/target-i386/arch_memory_mapping.c >>>> @@ -75,6 +75,8 @@ static void walk_pte2(MemoryMappingList *list, >>>> } >>>> >>>> /* PAE Paging or IA-32e Paging */ >>>> +#define PLM4_ADDR_MASK 0xffffffffff000 /* selects bits 51:12 */ >>>> + >>>> static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, >>>> int32_t a20_mask, target_ulong start_line_addr) >>>> { >>>> @@ -105,7 +107,7 @@ static void walk_pde(MemoryMappingList *list, hwaddr pde_start_addr, >>>> continue; >>>> } >>>> >>>> - pte_start_addr = (pde & ~0xfff) & a20_mask; >>>> + pte_start_addr = (pde & PLM4_ADDR_MASK) & a20_mask; >>>> walk_pte(list, pte_start_addr, a20_mask, line_addr); >>>> } >>>> } >>>> @@ -208,7 +210,7 @@ static void walk_pdpe(MemoryMappingList *list, >>>> continue; >>>> } >>>> >>>> - pde_start_addr = (pdpe & ~0xfff) & a20_mask; >>>> + pde_start_addr = (pdpe & PLM4_ADDR_MASK) & a20_mask; >>>> walk_pde(list, pde_start_addr, a20_mask, line_addr); >>>> } >>>> } >>>> @@ -231,7 +233,7 @@ static void walk_pml4e(MemoryMappingList *list, >>>> } >>>> >>>> line_addr = ((i & 0x1ffULL) << 39) | (0xffffULL << 48); >>>> - pdpe_start_addr = (pml4e & ~0xfff) & a20_mask; >>>> + pdpe_start_addr = (pml4e & PLM4_ADDR_MASK) & a20_mask; >>>> walk_pdpe(list, pdpe_start_addr, a20_mask, line_addr); >>>> } >>>> } >>>> @@ -249,7 +251,7 @@ int cpu_get_memory_mapping(MemoryMappingList *list, CPUArchState *env) >>>> if (env->hflags & HF_LMA_MASK) { >>>> hwaddr pml4e_addr; >>>> >>>> - pml4e_addr = (env->cr[3] & ~0xfff) & env->a20_mask; >>>> + pml4e_addr = (env->cr[3] & PLM4_ADDR_MASK) & env->a20_mask; >>>> walk_pml4e(list, pml4e_addr, env->a20_mask); >>>> } else >>>> #endif >>> >>> >> > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 14:10 ` Andreas Färber @ 2013-05-30 14:14 ` Luiz Capitulino 2013-05-30 14:22 ` Andreas Färber 0 siblings, 1 reply; 9+ messages in thread From: Luiz Capitulino @ 2013-05-30 14:14 UTC (permalink / raw) To: Andreas Färber; +Cc: qiaonuohan, Laszlo Ersek, qemu-stable, qemu-devel On Thu, 30 May 2013 16:10:28 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 30.05.2013 15:16, schrieb Luiz Capitulino: > > On Thu, 30 May 2013 15:16:18 +0200 > > Laszlo Ersek <lersek@redhat.com> wrote: > > > >> On 05/30/13 14:59, Luiz Capitulino wrote: > >>> On Tue, 28 May 2013 14:19:22 -0400 > >>> Luiz Capitulino <lcapitulino@redhat.com> wrote: > >>> > >>>> The code used to walk IA-32e page-tables, and possibly PAE page-tables, > >>>> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. > >>>> > >>>> However, as we use a uint64_t to store the resulting address, that mask > >>>> gets expanded to 0xfffffffffffff000 which not only ends up selecting > >>>> reserved bits but also selects the XD bit (execute-disable) which > >>>> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. > >>>> > >>>> This commit fixes that problem by replacing ~0xfff by a correct mask > >>>> that only selects the address bit range (ie. bits 51:12). > >>>> > >>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >>> > >>> Ping? Wen? > >>> > >>> Would be nice get a Reviewed-by before merging... > >> > >> I didn't miss your submission and did find it OK, I just felt unsure > >> about stating so, because "simple" patches like this are prime territory > >> to burn someone's R-b's worth (ie. to expose a reviewer's lack of > >> information / experience). But hey, what can I lose? The patch does look > >> good to me, so > > > > Thank you Laszlo! It's also new territory for me, that's why I'm asking > > for reviews (otherwise I'd just sneak it in some pull request :-) > > Luiz, you aware aware that I have another fix by Nuohan queued that > seemed orthogonal? Yes, they should be orthogonal. > If someone reviews my refactoring series (which > resent that patch) I would like to send out a PULL for that rather soon, > since it blocks further CPU work. I would then include your fix as well > to avoid merge conflicts. Thanks, although I was going to include it in my tomorrow's QMP pull request. Will this disturb your work? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 14:14 ` Luiz Capitulino @ 2013-05-30 14:22 ` Andreas Färber 2013-05-30 14:25 ` Luiz Capitulino 0 siblings, 1 reply; 9+ messages in thread From: Andreas Färber @ 2013-05-30 14:22 UTC (permalink / raw) To: Luiz Capitulino; +Cc: qiaonuohan, Laszlo Ersek, qemu-stable, qemu-devel Am 30.05.2013 16:14, schrieb Luiz Capitulino: > On Thu, 30 May 2013 16:10:28 +0200 > Andreas Färber <afaerber@suse.de> wrote: > >> Am 30.05.2013 15:16, schrieb Luiz Capitulino: >>> On Thu, 30 May 2013 15:16:18 +0200 >>> Laszlo Ersek <lersek@redhat.com> wrote: >>> >>>> On 05/30/13 14:59, Luiz Capitulino wrote: >>>>> On Tue, 28 May 2013 14:19:22 -0400 >>>>> Luiz Capitulino <lcapitulino@redhat.com> wrote: >>>>> >>>>>> The code used to walk IA-32e page-tables, and possibly PAE page-tables, >>>>>> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. >>>>>> >>>>>> However, as we use a uint64_t to store the resulting address, that mask >>>>>> gets expanded to 0xfffffffffffff000 which not only ends up selecting >>>>>> reserved bits but also selects the XD bit (execute-disable) which >>>>>> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. >>>>>> >>>>>> This commit fixes that problem by replacing ~0xfff by a correct mask >>>>>> that only selects the address bit range (ie. bits 51:12). >>>>>> >>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>>>> >>>>> Ping? Wen? >>>>> >>>>> Would be nice get a Reviewed-by before merging... >>>> >>>> I didn't miss your submission and did find it OK, I just felt unsure >>>> about stating so, because "simple" patches like this are prime territory >>>> to burn someone's R-b's worth (ie. to expose a reviewer's lack of >>>> information / experience). But hey, what can I lose? The patch does look >>>> good to me, so >>> >>> Thank you Laszlo! It's also new territory for me, that's why I'm asking >>> for reviews (otherwise I'd just sneak it in some pull request :-) >> >> Luiz, you aware aware that I have another fix by Nuohan queued that >> seemed orthogonal? > > Yes, they should be orthogonal. > >> If someone reviews my refactoring series (which >> resent that patch) I would like to send out a PULL for that rather soon, >> since it blocks further CPU work. I would then include your fix as well >> to avoid merge conflicts. > > Thanks, although I was going to include it in my tomorrow's QMP pull > request. Will this disturb your work? I hope not - could you then please pick up Nuohan's bugfix as well? Still I'd be interested in your review - and I have one more patch to add that I created offline yesterday wrt first paging-enabled CPU. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-30 14:22 ` Andreas Färber @ 2013-05-30 14:25 ` Luiz Capitulino 0 siblings, 0 replies; 9+ messages in thread From: Luiz Capitulino @ 2013-05-30 14:25 UTC (permalink / raw) To: Andreas Färber; +Cc: qiaonuohan, Laszlo Ersek, qemu-stable, qemu-devel On Thu, 30 May 2013 16:22:32 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 30.05.2013 16:14, schrieb Luiz Capitulino: > > On Thu, 30 May 2013 16:10:28 +0200 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Am 30.05.2013 15:16, schrieb Luiz Capitulino: > >>> On Thu, 30 May 2013 15:16:18 +0200 > >>> Laszlo Ersek <lersek@redhat.com> wrote: > >>> > >>>> On 05/30/13 14:59, Luiz Capitulino wrote: > >>>>> On Tue, 28 May 2013 14:19:22 -0400 > >>>>> Luiz Capitulino <lcapitulino@redhat.com> wrote: > >>>>> > >>>>>> The code used to walk IA-32e page-tables, and possibly PAE page-tables, > >>>>>> uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. > >>>>>> > >>>>>> However, as we use a uint64_t to store the resulting address, that mask > >>>>>> gets expanded to 0xfffffffffffff000 which not only ends up selecting > >>>>>> reserved bits but also selects the XD bit (execute-disable) which > >>>>>> happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. > >>>>>> > >>>>>> This commit fixes that problem by replacing ~0xfff by a correct mask > >>>>>> that only selects the address bit range (ie. bits 51:12). > >>>>>> > >>>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >>>>> > >>>>> Ping? Wen? > >>>>> > >>>>> Would be nice get a Reviewed-by before merging... > >>>> > >>>> I didn't miss your submission and did find it OK, I just felt unsure > >>>> about stating so, because "simple" patches like this are prime territory > >>>> to burn someone's R-b's worth (ie. to expose a reviewer's lack of > >>>> information / experience). But hey, what can I lose? The patch does look > >>>> good to me, so > >>> > >>> Thank you Laszlo! It's also new territory for me, that's why I'm asking > >>> for reviews (otherwise I'd just sneak it in some pull request :-) > >> > >> Luiz, you aware aware that I have another fix by Nuohan queued that > >> seemed orthogonal? > > > > Yes, they should be orthogonal. > > > >> If someone reviews my refactoring series (which > >> resent that patch) I would like to send out a PULL for that rather soon, > >> since it blocks further CPU work. I would then include your fix as well > >> to avoid merge conflicts. > > > > Thanks, although I was going to include it in my tomorrow's QMP pull > > request. Will this disturb your work? > > I hope not - could you then please pick up Nuohan's bugfix as well? Sure. > Still I'd be interested in your review - and I have one more patch to > add that I created offline yesterday wrt first paging-enabled CPU. You can CC me. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses 2013-05-28 18:19 [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses Luiz Capitulino 2013-05-30 12:59 ` Luiz Capitulino @ 2013-05-30 14:29 ` Andreas Färber 1 sibling, 0 replies; 9+ messages in thread From: Andreas Färber @ 2013-05-30 14:29 UTC (permalink / raw) To: Luiz Capitulino; +Cc: qiaonuohan, qemu-devel, qemu-stable Am 28.05.2013 20:19, schrieb Luiz Capitulino: > The code used to walk IA-32e page-tables, and possibly PAE page-tables, > uses the bit mask ~0xfff to get the next PML4E/PDPTE/PDE/PTE address. > > However, as we use a uint64_t to store the resulting address, that mask > gets expanded to 0xfffffffffffff000 which not only ends up selecting > reserved bits but also selects the XD bit (execute-disable) which > happens to be enabled by Windows 8, causing qemu_get_ram_ptr() to abort. > > This commit fixes that problem by replacing ~0xfff by a correct mask > that only selects the address bit range (ie. bits 51:12). > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> Reviewed-by: Andreas Färber <afaerber@suse.de> But please add a target-i386: prefix when queuing it. Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-30 14:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-28 18:19 [Qemu-devel] [PATCH] walk_pml4e(): fix abort on bad PML4E/PDPTE/PDE/PTE addresses Luiz Capitulino 2013-05-30 12:59 ` Luiz Capitulino 2013-05-30 13:16 ` Laszlo Ersek 2013-05-30 13:16 ` Luiz Capitulino 2013-05-30 14:10 ` Andreas Färber 2013-05-30 14:14 ` Luiz Capitulino 2013-05-30 14:22 ` Andreas Färber 2013-05-30 14:25 ` Luiz Capitulino 2013-05-30 14:29 ` Andreas Färber
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).