| Message ID | 20220420141307.24153-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up common/arch split for debugger.h | expand |
On 20.04.2022 16:13, Andrew Cooper wrote: > From: Bobby Eshleman <bobby.eshleman@gmail.com> > > debug.c contains only dbg_rw_mem(). Rename it to gdbsx.c. > > Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c > to gdbsx.c, merging it with dbg_rw_mem(). > > Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > The semantics are rather broken. XEN_DOMCTL_gdbsx_guestmemio only sets > copyback when there's nothing to copy back, and skips copying back in the > -EFAULT case when the iop->remain field is relevant. Furthermore, it can be > asked to move up to 4GB in one go, with no continuability whatsoever. The last point perhaps isn't overly much of a problem for this specific operation. Jan
On 21/04/2022 14:06, Jan Beulich wrote: > On 20.04.2022 16:13, Andrew Cooper wrote: >> From: Bobby Eshleman <bobby.eshleman@gmail.com> >> >> debug.c contains only dbg_rw_mem(). Rename it to gdbsx.c. >> >> Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c >> to gdbsx.c, merging it with dbg_rw_mem(). >> >> Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > >> The semantics are rather broken. XEN_DOMCTL_gdbsx_guestmemio only sets >> copyback when there's nothing to copy back, and skips copying back in the >> -EFAULT case when the iop->remain field is relevant. Furthermore, it can be >> asked to move up to 4GB in one go, with no continuability whatsoever. > The last point perhaps isn't overly much of a problem for this specific > operation. It's also not terribly hard to fix, but I really don't have time to get bogged down in "make the gdbsx hypercalls sane". XEN_DOMCTL_gdbsx_domstatus is a disaster, and there are far better ways of doing this. https://www.youtube.com/watch?v=osZeioYKsxA is one which was presented at XenSummit in 2019. ~Andrew
On 20.04.2022 16:13, Andrew Cooper wrote: > From: Bobby Eshleman <bobby.eshleman@gmail.com> > > debug.c contains only dbg_rw_mem(). Rename it to gdbsx.c. > > Move gdbsx_guest_mem_io(), and the prior setup of iop->remain, from domctl.c > to gdbsx.c, merging it with dbg_rw_mem(). > > Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Wei Liu <wl@xen.org> > > v5: > * Consolidate hunks from multiple v4 patches > * Rewrite commit message > > The semantics are rather broken. XEN_DOMCTL_gdbsx_guestmemio only sets > copyback when there's nothing to copy back, and skips copying back in the > -EFAULT case when the iop->remain field is relevant. Furthermore, it can be > asked to move up to 4GB in one go, with no continuability whatsoever. > --- > xen/arch/x86/Makefile | 2 +- > xen/arch/x86/domctl.c | 14 ++------------ > xen/arch/x86/{debug.c => gdbsx.c} | 23 ++++++++++------------- > xen/arch/x86/include/asm/debugger.h | 6 ------ > xen/arch/x86/include/asm/gdbsx.h | 13 +++++++++++++ > 5 files changed, 26 insertions(+), 32 deletions(-) > rename xen/arch/x86/{debug.c => gdbsx.c} (89%) > create mode 100644 xen/arch/x86/include/asm/gdbsx.h As I've realized only while reviewing your newer gdbsx patch, this should have come with an update to ./MAINTAINERS. Quite possibly one simply deleting the entire entry there. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 717bcbcac7a0..177a2ff74272 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -22,7 +22,6 @@ obj-y += cpuid.o obj-$(CONFIG_PV) += compat.o obj-$(CONFIG_PV32) += x86_64/compat.o obj-$(CONFIG_KEXEC) += crash.o -obj-$(CONFIG_GDBSX) += debug.o obj-y += delay.o obj-y += desc.o obj-bin-y += dmi_scan.init.o @@ -34,6 +33,7 @@ obj-y += emul-i8254.o obj-y += extable.o obj-y += flushtlb.o obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o +obj-$(CONFIG_GDBSX) += gdbsx.o obj-y += hypercall.o obj-y += i387.o obj-y += i8259.o diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index a6aae500a30b..c20ab4352715 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -20,6 +20,8 @@ #include <xen/console.h> #include <xen/iocap.h> #include <xen/paging.h> + +#include <asm/gdbsx.h> #include <asm/irq.h> #include <asm/hvm/emulate.h> #include <asm/hvm/hvm.h> @@ -33,20 +35,9 @@ #include <public/vm_event.h> #include <asm/mem_sharing.h> #include <asm/xstate.h> -#include <asm/debugger.h> #include <asm/psr.h> #include <asm/cpuid.h> -#ifdef CONFIG_GDBSX -static int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop) -{ - iop->remain = dbg_rw_mem(iop->gva, guest_handle_from_ptr(iop->uva, void), - iop->len, d, iop->gwr, iop->pgd3val); - - return iop->remain ? -EFAULT : 0; -} -#endif - static int update_domain_cpu_policy(struct domain *d, xen_domctl_cpu_policy_t *xdpc) { @@ -827,7 +818,6 @@ long arch_do_domctl( #ifdef CONFIG_GDBSX case XEN_DOMCTL_gdbsx_guestmemio: - domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len; ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio); if ( !ret ) copyback = true; diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/gdbsx.c similarity index 89% rename from xen/arch/x86/debug.c rename to xen/arch/x86/gdbsx.c index 91034a852e5f..59eb31fc9a6a 100644 --- a/xen/arch/x86/debug.c +++ b/xen/arch/x86/gdbsx.c @@ -18,7 +18,7 @@ #include <xen/mm.h> #include <xen/domain_page.h> #include <xen/guest_access.h> -#include <asm/debugger.h> +#include <asm/gdbsx.h> #include <asm/p2m.h> typedef unsigned long dbgva_t; @@ -150,21 +150,18 @@ static unsigned int dbg_rw_guest_mem(struct domain *dp, unsigned long addr, return len; } -/* - * addr is guest addr - * buf is debugger buffer. - * if toaddr, then addr = buf (write to addr), else buf = addr (rd from guest) - * pgd3: value of init_mm.pgd[3] in guest. see above. - * Returns: number of bytes remaining to be copied. - */ -unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf, - unsigned int len, struct domain *d, bool toaddr, - uint64_t pgd3) +int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop) { if ( d && !d->is_dying ) - len = dbg_rw_guest_mem(d, gva, buf, len, toaddr, pgd3); + { + iop->remain = dbg_rw_guest_mem( + d, iop->gva, guest_handle_from_ptr(iop->uva, void), + iop->len, iop->gwr, iop->pgd3val); + } + else + iop->remain = iop->len; - return len; + return iop->remain ? -EFAULT : 0; } /* diff --git a/xen/arch/x86/include/asm/debugger.h b/xen/arch/x86/include/asm/debugger.h index e83b346a21d1..c5585752cae7 100644 --- a/xen/arch/x86/include/asm/debugger.h +++ b/xen/arch/x86/include/asm/debugger.h @@ -54,10 +54,4 @@ static inline bool debugger_trap_fatal( #endif -#ifdef CONFIG_GDBSX -unsigned int dbg_rw_mem(unsigned long gva, XEN_GUEST_HANDLE_PARAM(void) buf, - unsigned int len, struct domain *d, bool toaddr, - uint64_t pgd3); -#endif - #endif /* __X86_DEBUGGER_H__ */ diff --git a/xen/arch/x86/include/asm/gdbsx.h b/xen/arch/x86/include/asm/gdbsx.h new file mode 100644 index 000000000000..eee746fc01d0 --- /dev/null +++ b/xen/arch/x86/include/asm/gdbsx.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __X86_GDBX_H__ +#define __X86_GDBX_H__ + +#ifdef CONFIG_GDBSX + +struct domain; +struct xen_domctl_gdbsx_memio; + +int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop); + +#endif /* CONFIG_GDBSX */ +#endif /* __X86_GDBX_H__ */