On 13.07.2021 03:59, Bobby Eshleman wrote: > --- a/xen/arch/x86/gdbstub.c > +++ b/xen/arch/x86/gdbstub.c > @@ -18,7 +18,9 @@ > * You should have received a copy of the GNU General Public License > * along with this program; If not, see <http://www.gnu.org/licenses/>. > */ > -#include <asm/debugger.h> > +#include <asm/uaccess.h> > +#include <xen/debugger.h> > +#include <xen/gdbstub.h>
Here and in at least one more case below: Our usual pattern is to have xen/ ones before asm/ ones. And (leaving aside existing screwed code) ... > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -58,7 +58,7 @@ > #include <asm/hvm/trace.h> > #include <asm/hap.h> > #include <asm/apic.h> > -#include <asm/debugger.h> > +#include <xen/debugger.h> > #include <asm/hvm/monitor.h> > #include <asm/monitor.h> > #include <asm/xstate.h> ... we also try to avoid introducing any mixture. Plus ... > --- a/xen/arch/x86/hvm/vmx/realmode.c > +++ b/xen/arch/x86/hvm/vmx/realmode.c > @@ -14,7 +14,7 @@ > #include <xen/sched.h> > #include <xen/paging.h> > #include <xen/softirq.h> > -#include <asm/debugger.h> > +#include <xen/debugger.h> ... we strive to have new insertions be sorted alphabetically. When the existing section to insert into isn't suitably sorted yet, what I normally do is try to find a place where at least the immediately adjacent neighbors then fit the sorting goal. Sorry, all just nits, but their scope is about the entire patch. > --- /dev/null > +++ b/xen/include/xen/debugger.h > @@ -0,0 +1,81 @@ > +/****************************************************************************** > + * Generic hooks into arch-dependent Xen. Now that you move this to be generic, I think it better also would indeed be. See below. > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; If not, see <http://www.gnu.org/licenses/>. > + * > + * Nit: No double blank (comment) lines please. > + * Each debugger should define three functions here: > + * > + * 1. debugger_trap_entry(): > + * Called at start of any synchronous fault or trap, before any other work > + * is done. The idea is that if your debugger deliberately caused the trap > + * (e.g. to implement breakpoints or data watchpoints) then you can take > + * appropriate action and return a non-zero value to cause early exit from > + * the trap function. > + * > + * 2. debugger_trap_fatal(): > + * Called when Xen is about to give up and crash. Typically you will use > this > + * hook to drop into a debug session. It can also be used to hook off > + * deliberately caused traps (which you then handle and return non-zero). > + * > + * 3. debugger_trap_immediate(): > + * Called if we want to drop into a debugger now. This is essentially the > + * same as debugger_trap_fatal, except that we use the current register > state > + * rather than the state which was in effect when we took the trap. > + * For example: if we're dying because of an unhandled exception, we call > + * debugger_trap_fatal; if we're dying because of a panic() we call > + * debugger_trap_immediate(). > + */ > + > +#ifndef __XEN_DEBUGGER_H__ > +#define __XEN_DEBUGGER_H__ > + > +/* Dummy value used by ARM stubs. */ > +#ifndef TRAP_invalid_op > +# define TRAP_invalid_op 6 > +#endif To avoid the need to introduce this, please flip ordering with the subsequent patch. > +#ifdef CONFIG_CRASH_DEBUG > + > +#include <asm/debugger.h> > + > +#else > + > +#include <asm/regs.h> > +#include <asm/processor.h> > + > +static inline void domain_pause_for_debugger(void) > +{ > +} > + > +static inline bool debugger_trap_fatal( > + unsigned int vector, const struct cpu_user_regs *regs) I'm afraid the concept of a vector may not be arch-independent. > +{ > + return false; > +} > + > +static inline void debugger_trap_immediate(void) > +{ > +} > + > +static inline bool debugger_trap_entry( > + unsigned int vector, const struct cpu_user_regs *regs) > +{ > + return false; > +} > + > +#endif /* CONFIG_CRASH_DEBUG */ > + > +#ifdef CONFIG_GDBSX > +unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf, > + unsigned int len, domid_t domid, bool toaddr, > + uint64_t pgd3); > +#endif /* CONFIG_GDBSX */ I'm afraid this whole construct isn't arch independent, at least as long as it has the "pgd3" parameter, documented elsewhere to mean "the value of init_mm.pgd[3] in a PV guest" (whatever this really is in a 64-bit guest, or in a non-Linux one). But I don't see why this needs moving to common code in the first place: It's x86-specific both on the producer and the consumer sides. Jan
