Message ID | 20220609083456.77946-1-yuan.yao@intel.com |
---|---|
State | New |
Headers | show |
Series | [1/1] i386/monitor: Fix page table walking issue for LA57 enabled guest | expand |
> -----Original Message----- > From: Qemu-devel <qemu-devel- > bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Yuan Yao > Sent: Thursday, June 9, 2022 4:35 PM > To: Paolo Bonzini <pbonzini@redhat.com>; Philippe Mathieu-Daudé > <f4bug@amsat.org>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Zhong, > Yang <yang.zhong@intel.com>; Connor Kuehl <ckuehl@redhat.com> > Cc: qemu-devel@nongnu.org; Yao, Yuan <yuan.yao@intel.com>; Yamahata, > Isaku <isaku.yamahata@intel.com> > Subject: [PATCH 1/1] i386/monitor: Fix page table walking issue for LA57 > enabled guest > > Don't skip next leve page table for pdpe/pde when the PG_PRESENT_MASK S/leve/level > is set. > > This fixs the issue that no mapping information was collected from "info > mem" for guest with LA57 enabled. > > Signed-off-by: Yuan Yao <yuan.yao@intel.com> LGTM. It should same with the excp_helper.c/mmu_translate() la57 implementation. Reviewed-by: Zhang Chen <chen.zhang@intel.com> Add Eric and Markus for double check. Thanks Chen > --- > target/i386/monitor.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/i386/monitor.c b/target/i386/monitor.c index > 8e4b4d600c..3339550bbe 100644 > --- a/target/i386/monitor.c > +++ b/target/i386/monitor.c > @@ -489,7 +489,7 @@ static void mem_info_la57(Monitor *mon, > CPUArchState *env) > cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8); > pdpe = le64_to_cpu(pdpe); > end = (l0 << 48) + (l1 << 39) + (l2 << 30); > - if (pdpe & PG_PRESENT_MASK) { > + if (!(pdpe & PG_PRESENT_MASK)) { > prot = 0; > mem_print(mon, env, &start, &last_prot, end, prot); > continue; > @@ -508,7 +508,7 @@ static void mem_info_la57(Monitor *mon, > CPUArchState *env) > cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8); > pde = le64_to_cpu(pde); > end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21); > - if (pde & PG_PRESENT_MASK) { > + if (!(pde & PG_PRESENT_MASK)) { > prot = 0; > mem_print(mon, env, &start, &last_prot, end, prot); > continue; > > base-commit: 6d940eff4734bcb40b1a25f62d7cec5a396f994a > -- > 2.27.0 >
Yuan Yao <yuan.yao@intel.com> writes: > Don't skip next leve page table for pdpe/pde when the level > PG_PRESENT_MASK is set. > > This fixs the issue that no mapping information was fixes > collected from "info mem" for guest with LA57 enabled. > > Signed-off-by: Yuan Yao <yuan.yao@intel.com> Should we add Fixes: 6c7c3c21f95dd9af8a0691c0dd29b07247984122 ? > --- > target/i386/monitor.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/i386/monitor.c b/target/i386/monitor.c > index 8e4b4d600c..3339550bbe 100644 > --- a/target/i386/monitor.c > +++ b/target/i386/monitor.c > @@ -489,7 +489,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) > cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8); > pdpe = le64_to_cpu(pdpe); > end = (l0 << 48) + (l1 << 39) + (l2 << 30); > - if (pdpe & PG_PRESENT_MASK) { > + if (!(pdpe & PG_PRESENT_MASK)) { > prot = 0; > mem_print(mon, env, &start, &last_prot, end, prot); > continue; > @@ -508,7 +508,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) > cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8); > pde = le64_to_cpu(pde); > end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21); > - if (pde & PG_PRESENT_MASK) { > + if (!(pde & PG_PRESENT_MASK)) { > prot = 0; > mem_print(mon, env, &start, &last_prot, end, prot); > continue; > > base-commit: 6d940eff4734bcb40b1a25f62d7cec5a396f994a The commit message talks about not skipping something when the flag is set. However, the patch *flips* the sense of conditions, which means were *also* changing behavior when the flag is unset. How?
>-----Original Message----- >From: Markus Armbruster <armbru@redhat.com> >Sent: Friday, June 24, 2022 13:35 >To: Yao, Yuan <yuan.yao@intel.com> >Cc: Paolo Bonzini <pbonzini@redhat.com>; Philippe Mathieu-Daudé <f4bug@amsat.org>; Dr. David Alan Gilbert ><dgilbert@redhat.com>; Zhong, Yang <yang.zhong@intel.com>; Connor Kuehl <ckuehl@redhat.com>; qemu-devel@nongnu.org; >Yamahata, Isaku <isaku.yamahata@intel.com> >Subject: Re: [PATCH 1/1] i386/monitor: Fix page table walking issue for LA57 enabled guest > >Yuan Yao <yuan.yao@intel.com> writes: > >> Don't skip next leve page table for pdpe/pde when the > >level Sorry for my typo. > >> PG_PRESENT_MASK is set. >> >> This fixs the issue that no mapping information was > >fixes > >> collected from "info mem" for guest with LA57 enabled. >> >> Signed-off-by: Yuan Yao <yuan.yao@intel.com> > >Should we add > > Fixes: 6c7c3c21f95dd9af8a0691c0dd29b07247984122 Yes, I will add this next time, thanks. > >? > >> --- >> target/i386/monitor.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target/i386/monitor.c b/target/i386/monitor.c >> index 8e4b4d600c..3339550bbe 100644 >> --- a/target/i386/monitor.c >> +++ b/target/i386/monitor.c >> @@ -489,7 +489,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) >> cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8); >> pdpe = le64_to_cpu(pdpe); >> end = (l0 << 48) + (l1 << 39) + (l2 << 30); >> - if (pdpe & PG_PRESENT_MASK) { >> + if (!(pdpe & PG_PRESENT_MASK)) { >> prot = 0; >> mem_print(mon, env, &start, &last_prot, end, prot); >> continue; >> @@ -508,7 +508,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) >> cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8); >> pde = le64_to_cpu(pde); >> end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21); >> - if (pde & PG_PRESENT_MASK) { >> + if (!(pde & PG_PRESENT_MASK)) { >> prot = 0; >> mem_print(mon, env, &start, &last_prot, end, prot); >> continue; >> >> base-commit: 6d940eff4734bcb40b1a25f62d7cec5a396f994a > >The commit message talks about not skipping something when the flag is >set. However, the patch *flips* the sense of conditions, which means >were *also* changing behavior when the flag is unset. How? Yes, this also changes the behavior when the flag is unset, because the original code does wrong for both set and unset , flips the checking condition bring all of them to right behavior. I think I can add these to the commit message in v2.
>-----Original Message----- >From: Zhang, Chen <chen.zhang@intel.com> >Sent: Friday, June 24, 2022 10:04 >To: Yao, Yuan <yuan.yao@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Philippe Mathieu-Daudé <f4bug@amsat.org>; Dr. >David Alan Gilbert <dgilbert@redhat.com>; Zhong, Yang <yang.zhong@intel.com>; Connor Kuehl <ckuehl@redhat.com>; Eric >Blake <eblake@redhat.com>; Markus Armbruster <armbru@redhat.com> >Cc: qemu-devel@nongnu.org; Yao, Yuan <yuan.yao@intel.com>; Yamahata, Isaku <isaku.yamahata@intel.com> >Subject: RE: [PATCH 1/1] i386/monitor: Fix page table walking issue for LA57 enabled guest > > > >> -----Original Message----- >> From: Qemu-devel <qemu-devel- >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Yuan Yao >> Sent: Thursday, June 9, 2022 4:35 PM >> To: Paolo Bonzini <pbonzini@redhat.com>; Philippe Mathieu-Daudé >> <f4bug@amsat.org>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Zhong, >> Yang <yang.zhong@intel.com>; Connor Kuehl <ckuehl@redhat.com> >> Cc: qemu-devel@nongnu.org; Yao, Yuan <yuan.yao@intel.com>; Yamahata, >> Isaku <isaku.yamahata@intel.com> >> Subject: [PATCH 1/1] i386/monitor: Fix page table walking issue for LA57 >> enabled guest >> >> Don't skip next leve page table for pdpe/pde when the PG_PRESENT_MASK > >S/leve/level Sorry for the typo. > >> is set. >> >> This fixs the issue that no mapping information was collected from "info >> mem" for guest with LA57 enabled. >> >> Signed-off-by: Yuan Yao <yuan.yao@intel.com> > >LGTM. >It should same with the excp_helper.c/mmu_translate() la57 implementation. >Reviewed-by: Zhang Chen <chen.zhang@intel.com> > >Add Eric and Markus for double check. Thanks for your comments. > >Thanks >Chen > >> --- >> target/i386/monitor.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/target/i386/monitor.c b/target/i386/monitor.c index >> 8e4b4d600c..3339550bbe 100644 >> --- a/target/i386/monitor.c >> +++ b/target/i386/monitor.c >> @@ -489,7 +489,7 @@ static void mem_info_la57(Monitor *mon, >> CPUArchState *env) >> cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8); >> pdpe = le64_to_cpu(pdpe); >> end = (l0 << 48) + (l1 << 39) + (l2 << 30); >> - if (pdpe & PG_PRESENT_MASK) { >> + if (!(pdpe & PG_PRESENT_MASK)) { >> prot = 0; >> mem_print(mon, env, &start, &last_prot, end, prot); >> continue; >> @@ -508,7 +508,7 @@ static void mem_info_la57(Monitor *mon, >> CPUArchState *env) >> cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8); >> pde = le64_to_cpu(pde); >> end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21); >> - if (pde & PG_PRESENT_MASK) { >> + if (!(pde & PG_PRESENT_MASK)) { >> prot = 0; >> mem_print(mon, env, &start, &last_prot, end, prot); >> continue; >> >> base-commit: 6d940eff4734bcb40b1a25f62d7cec5a396f994a >> -- >> 2.27.0 >>
diff --git a/target/i386/monitor.c b/target/i386/monitor.c index 8e4b4d600c..3339550bbe 100644 --- a/target/i386/monitor.c +++ b/target/i386/monitor.c @@ -489,7 +489,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) cpu_physical_memory_read(pdp_addr + l2 * 8, &pdpe, 8); pdpe = le64_to_cpu(pdpe); end = (l0 << 48) + (l1 << 39) + (l2 << 30); - if (pdpe & PG_PRESENT_MASK) { + if (!(pdpe & PG_PRESENT_MASK)) { prot = 0; mem_print(mon, env, &start, &last_prot, end, prot); continue; @@ -508,7 +508,7 @@ static void mem_info_la57(Monitor *mon, CPUArchState *env) cpu_physical_memory_read(pd_addr + l3 * 8, &pde, 8); pde = le64_to_cpu(pde); end = (l0 << 48) + (l1 << 39) + (l2 << 30) + (l3 << 21); - if (pde & PG_PRESENT_MASK) { + if (!(pde & PG_PRESENT_MASK)) { prot = 0; mem_print(mon, env, &start, &last_prot, end, prot); continue;
Don't skip next leve page table for pdpe/pde when the PG_PRESENT_MASK is set. This fixs the issue that no mapping information was collected from "info mem" for guest with LA57 enabled. Signed-off-by: Yuan Yao <yuan.yao@intel.com> --- target/i386/monitor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 6d940eff4734bcb40b1a25f62d7cec5a396f994a