| Message ID | 20220812192448.43016-9-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: More clean-ups and improvement | expand |
Hi Julien, > -----Original Message----- > Subject: [PATCH 7/7] xen/arm32: traps: Dump more information for > hypervisor data abort > > From: Julien Grall <jgrall@amazon.com> > > Unlike arm64, on arm32 there are no extra information dumped (e.g. > page table walk) for hypervisor data abort. > > For data abort, the HSR will be set properly and so replace the call > to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error. > > Signed-off-by: Julien Grall <jgrall@amazon.com> I think this patch looks good to me so: Reviewed-by: Henry Wang <Henry.Wang@arm.com> But it seems that there is a duplicated patch at: https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-10-julien@xen.org/ Kind regards, Henry
Hi Julien, > On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > Unlike arm64, on arm32 there are no extra information dumped (e.g. > page table walk) for hypervisor data abort. The code in arch/arm/traps.c has nothing arm32 specific like that so could you explain this statement ? Here the arm32 code will call the generic function which has only something specific for BRK handling but the rest is generic. > > For data abort, the HSR will be set properly and so replace the call > to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error. I agree with that, I just do not understand your previous statement here. Cheers Bertrand > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/arch/arm/arm32/traps.c | 2 +- > xen/arch/arm/include/asm/traps.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c > index a4ce2b92d904..a2fc1c22cbc9 100644 > --- a/xen/arch/arm/arm32/traps.c > +++ b/xen/arch/arm/arm32/traps.c > @@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs) > if ( VABORT_GEN_BY_GUEST(regs) ) > do_trap_guest_serror(regs); > else > - do_unexpected_trap("Data Abort", regs); > + do_trap_hyp_sync(regs); > } > > void finalize_instr_emulation(const struct instr_details *instr) > diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h > index 08bc0b484c75..883dae368eac 100644 > --- a/xen/arch/arm/include/asm/traps.h > +++ b/xen/arch/arm/include/asm/traps.h > @@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc); > > void noreturn do_unexpected_trap(const char *msg, > const struct cpu_user_regs *regs); > +void do_trap_hyp_sync(struct cpu_user_regs *regs); > > /* Functions for pending virtual abort checking window. */ > void abort_guest_exit_start(void); > -- > 2.37.1 >
On 15/08/2022 02:40, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> Subject: [PATCH 7/7] xen/arm32: traps: Dump more information for >> hypervisor data abort >> >> From: Julien Grall <jgrall@amazon.com> >> >> Unlike arm64, on arm32 there are no extra information dumped (e.g. >> page table walk) for hypervisor data abort. >> >> For data abort, the HSR will be set properly and so replace the call >> to do_unexpected_trap() with do_trap_hyp_sync() to dispatch the error. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > I think this patch looks good to me so: > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > > But it seems that there is a duplicated patch at: > https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-10-julien@xen.org/ Hmmm... I dropped a patch from the series and it looks like I didn't properly clean the directory before doing that. Please ignore patch #8. Cheers,
On 15/08/2022 17:39, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, >> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote: >> >> From: Julien Grall <jgrall@amazon.com> >> >> Unlike arm64, on arm32 there are no extra information dumped (e.g. >> page table walk) for hypervisor data abort. > > The code in arch/arm/traps.c has nothing arm32 specific like that so > could you explain this statement ? > > Here the arm32 code will call the generic function which has only > something specific for BRK handling but the rest is generic. The statement is not related to the code but the console output. On arm64, a data abort will decode the HSR and provide a dump of the page-table walk. This doesn't happen on arm32 because Xen will call do_unexpected_trap(). So the only information we have is the HSR and FAR. This is not very helpful for debugging page-table walk. After this patch, the same information will be printed on arm32 and arm64. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH 7/7] xen/arm32: traps: Dump more information for > hypervisor data abort > Hmmm... I dropped a patch from the series and it looks like I didn't > properly clean the directory before doing that. Please ignore patch #8. Sure, I guess the patch that you dropped is this one? https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-8-julien@xen.org/ Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Julien, > On 15 Aug 2022, at 18:04, Julien Grall <julien@xen.org> wrote: > > > > On 15/08/2022 17:39, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote: >>> >>> From: Julien Grall <jgrall@amazon.com> >>> >>> Unlike arm64, on arm32 there are no extra information dumped (e.g. >>> page table walk) for hypervisor data abort. >> The code in arch/arm/traps.c has nothing arm32 specific like that so >> could you explain this statement ? >> Here the arm32 code will call the generic function which has only >> something specific for BRK handling but the rest is generic. > > The statement is not related to the code but the console output. On arm64, a data abort will decode the HSR and provide a dump of the page-table walk. > > This doesn't happen on arm32 because Xen will call do_unexpected_trap(). So the only information we have is the HSR and FAR. This is not very helpful for debugging page-table walk. > > After this patch, the same information will be printed on arm32 and arm64. Ok then this is what I understood. Your commit message is maybe a bit unclear. I would add a sentence like that: Call do_trap_hyp_sync for hypervisor data aborts on arm32 to have the same information than on arm64. This can be done on commit so feel free to add my: Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > > Cheers, > > -- > Julien Grall
On 16/08/2022 04:29, Henry Wang wrote: > Hi Julien, Hi Henry, Sorry for the late reply. > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Subject: Re: [PATCH 7/7] xen/arm32: traps: Dump more information for >> hypervisor data abort >> Hmmm... I dropped a patch from the series and it looks like I didn't >> properly clean the directory before doing that. Please ignore patch #8. > > Sure, I guess the patch that you dropped is this one? > https://patchwork.kernel.org/project/xen-devel/patch/20220812192448.43016-8-julien@xen.org/ That's correct. Cheers,
On 16/08/2022 08:28, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, >> On 15 Aug 2022, at 18:04, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 15/08/2022 17:39, Bertrand Marquis wrote: >>> Hi Julien, >> >> Hi Bertrand, >> >>>> On 12 Aug 2022, at 20:24, Julien Grall <julien@xen.org> wrote: >>>> >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> Unlike arm64, on arm32 there are no extra information dumped (e.g. >>>> page table walk) for hypervisor data abort. >>> The code in arch/arm/traps.c has nothing arm32 specific like that so >>> could you explain this statement ? >>> Here the arm32 code will call the generic function which has only >>> something specific for BRK handling but the rest is generic. >> >> The statement is not related to the code but the console output. On arm64, a data abort will decode the HSR and provide a dump of the page-table walk. >> >> This doesn't happen on arm32 because Xen will call do_unexpected_trap(). So the only information we have is the HSR and FAR. This is not very helpful for debugging page-table walk. >> >> After this patch, the same information will be printed on arm32 and arm64. > > Ok then this is what I understood. Your commit message is maybe a bit unclear. > > I would add a sentence like that: Call do_trap_hyp_sync for hypervisor data aborts on arm32 to have the same information than on arm64. Below the new commit message: Unlike arm64, on arm32 there are no extra information dumped (e.g. page table walk) for hypervisor data abort. For data abort, the HSR will be set properly and so call do_trap_hyp_sync() instead of do_unexpected_trap() on arm32 to have the print the same information as arm64. > > This can be done on commit so feel free to add my: > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Thanks! I have committed the series. Cheers,
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index a4ce2b92d904..a2fc1c22cbc9 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -81,7 +81,7 @@ void do_trap_data_abort(struct cpu_user_regs *regs) if ( VABORT_GEN_BY_GUEST(regs) ) do_trap_guest_serror(regs); else - do_unexpected_trap("Data Abort", regs); + do_trap_hyp_sync(regs); } void finalize_instr_emulation(const struct instr_details *instr) diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h index 08bc0b484c75..883dae368eac 100644 --- a/xen/arch/arm/include/asm/traps.h +++ b/xen/arch/arm/include/asm/traps.h @@ -73,6 +73,7 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc); void noreturn do_unexpected_trap(const char *msg, const struct cpu_user_regs *regs); +void do_trap_hyp_sync(struct cpu_user_regs *regs); /* Functions for pending virtual abort checking window. */ void abort_guest_exit_start(void);