Re: [PATCH 1/4] build: use common stubs for debugger_trap_* functions if !CONFIG_CRASH_DEBUG

[Jan Beulich <jbeulich@suse.com>]

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