|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 1/2]: hypervisor debugger
> diff -r 32034d1914a6 xen/Rules.mk
> --- a/xen/Rules.mk Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/Rules.mk Wed Aug 29 14:39:57 2012 -0700
> @@ -53,6 +55,7 @@
> CFLAGS-$(HAS_ACPI) += -DHAS_ACPI
> CFLAGS-$(HAS_PASSTHROUGH) += -DHAS_PASSTHROUGH
> CFLAGS-$(frame_pointer) += -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER
> +CFLAGS-$(kdb) += -DXEN_KDB_CONFIG
Pretty much everywhere else uses CONFIG_FOO not FOO_CONFIG. Also isn't
XEN rather redundant in the context?
>
> ifneq ($(max_phys_cpus),)
> CFLAGS-y += -DMAX_PHYS_CPUS=$(max_phys_cpus)
> diff -r 32034d1914a6 xen/arch/x86/hvm/svm/entry.S
> --- a/xen/arch/x86/hvm/svm/entry.S Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/entry.S Wed Aug 29 14:39:57 2012 -0700
> @@ -59,12 +59,23 @@
> get_current(bx)
> CLGI
>
> +#ifdef XEN_KDB_CONFIG
> +#if defined(__x86_64__)
> + testl $1, kdb_session_begun(%rip)
> +#else
> + testl $1, kdb_session_begun
> +#endif
This inner #ifdef is what the addr_of macro does.
> + jnz .Lkdb_skip_softirq
> +#endif
> mov VCPU_processor(r(bx)),%eax
> shl $IRQSTAT_shift,r(ax)
> lea addr_of(irq_stat),r(dx)
> testl $~0,(r(dx),r(ax),1)
> jnz .Lsvm_process_softirqs
>
> +#ifdef XEN_KDB_CONFIG
> +.Lkdb_skip_softirq:
> +#endif
Does gas complain about unused labels? IOW can you omit the ifdef here?
Why does kdb skip soft irqs? I presume the final submission will include
a commit log which will explain this sort of thing?
> testb $0, VCPU_nsvm_hap_enabled(r(bx))
> UNLIKELY_START(nz, nsvm_hap)
> mov VCPU_nhvm_p2m(r(bx)),r(ax)
> diff -r 32034d1914a6 xen/arch/x86/hvm/svm/svm.c
> --- a/xen/arch/x86/hvm/svm/svm.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/svm.c Wed Aug 29 14:39:57 2012 -0700
> @@ -2170,6 +2170,10 @@
> break;
>
> case VMEXIT_EXCEPTION_DB:
> +#ifdef XEN_KDB_CONFIG
> + if (kdb_handle_trap_entry(TRAP_debug, regs))
> + break;
defining kdb_handle_trap_entry as a nop when !KDB might make this and
the following similar examples tidier.
> +#endif
> if ( !v->domain->debugger_attached )
> goto exit_and_crash;
> domain_pause_for_debugger();
> @@ -2182,6 +2186,10 @@
> if ( (inst_len = __get_instruction_length(v, INSTR_INT3)) == 0 )
> break;
> __update_guest_eip(regs, inst_len);
> +#ifdef XEN_KDB_CONFIG
> + if (kdb_handle_trap_entry(TRAP_int3, regs))
> + break;
> +#endif
> current->arch.gdbsx_vcpu_event = TRAP_int3;
> domain_pause_for_debugger();
> break;
> diff -r 32034d1914a6 xen/arch/x86/hvm/svm/vmcb.c
> --- a/xen/arch/x86/hvm/svm/vmcb.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/svm/vmcb.c Wed Aug 29 14:39:57 2012 -0700
> @@ -315,6 +315,36 @@
> register_keyhandler('v', &vmcb_dump_keyhandler);
> }
>
> +#if defined(XEN_KDB_CONFIG)
> +/* did == 0 : display for all HVM domains. domid 0 is never HVM.
Ahem, I think you have a patch which kind of changes that ;-)
(I notice you have _or_hyb_domain below so I guess you've thought of
it ;-). Perhaps you culd use DOMID_INVALID or something as your flag?
> + * * vid == -1 : display for all HVM VCPUs
> + * */
> +void kdb_dump_vmcb(domid_t did, int vid)
> +{
> + struct domain *dp;
> + struct vcpu *vp;
> +
> + rcu_read_lock(&domlist_read_lock);
> + for_each_domain (dp) {
> + if (!is_hvm_or_hyb_domain(dp) || dp->is_dying)
> + continue;
> + if (did != 0 && did != dp->domain_id)
> + continue;
> +
> + for_each_vcpu (dp, vp) {
> + if (vid != -1 && vid != vp->vcpu_id)
> + continue;
> +
> + kdbp(" VMCB [domid: %d vcpu:%d]:\n", dp->domain_id,
> vp->vcpu_id);
> + svm_vmcb_dump("kdb", vp->arch.hvm_svm.vmcb);
> + kdbp("\n");
> + }
> + kdbp("\n");
> + }
> + rcu_read_unlock(&domlist_read_lock);
> +}
> +#endif
> +
> /*
> * Local variables:
> * mode: C
> diff -r 32034d1914a6 xen/arch/x86/hvm/vmx/entry.S
> --- a/xen/arch/x86/hvm/vmx/entry.S Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/vmx/entry.S Wed Aug 29 14:39:57 2012 -0700
> @@ -124,12 +124,23 @@
> get_current(bx)
> cli
>
> +#ifdef XEN_KDB_CONFIG
> +#if defined(__x86_64__)
> + testl $1, kdb_session_begun(%rip)
> +#else
> + testl $1, kdb_session_begun
> +#endif
> + jnz .Lkdb_skip_softirq
> +#endif
> mov VCPU_processor(r(bx)),%eax
> shl $IRQSTAT_shift,r(ax)
> lea addr_of(irq_stat),r(dx)
> cmpl $0,(r(dx),r(ax),1)
> jnz .Lvmx_process_softirqs
>
> +#ifdef XEN_KDB_CONFIG
> +.Lkdb_skip_softirq:
> +#endif
Same comments here as to the svm version.
> testb $0xff,VCPU_vmx_emulate(r(bx))
> jnz .Lvmx_goto_emulator
> testb $0xff,VCPU_vmx_realmode(r(bx))
> diff -r 32034d1914a6 xen/arch/x86/hvm/vmx/vmcs.c
> --- a/xen/arch/x86/hvm/vmx/vmcs.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c Wed Aug 29 14:39:57 2012 -0700
> @@ -1117,6 +1117,13 @@
> hvm_asid_flush_vcpu(v);
> }
>
> +#if defined(XEN_KDB_CONFIG)
> + if (kdb_dr7)
> + __vmwrite(GUEST_DR7, kdb_dr7);
> + else
> + __vmwrite(GUEST_DR7, 0);
Isn't this the same as
__vmwrite(GUEST_DR7, kdb_dr7) ?
Why does kdb maintain it's own global here instead of manipulating the
guest dr7 values in the state?
> +#endif
> +
> debug_state = v->domain->debugger_attached
> ||
> v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_INT3]
> ||
> v->domain->arch.hvm_domain.params[HVM_PARAM_MEMORY_EVENT_SINGLE_STEP];
> @@ -1326,6 +1333,220 @@
> register_keyhandler('v', &vmcs_dump_keyhandler);
> }
>
> +#if defined(XEN_KDB_CONFIG)
> +#define GUEST_EFER 0x2806 /* see page 23-20 */
> +#define GUEST_EFER_HIGH 0x2807 /* see page 23-20 */
23-20 of what?
These belong with the other GUEST_* VMCS offset definitions in vmcs.h
> +
> +/* it's a shame we can't use vmcs_dump_vcpu(), but it does vmx_vmcs_enter
> which
> + * will IPI other CPUs. also, print a subset relevant to software debugging
> */
> +static void noinline kdb_print_vmcs(struct vcpu *vp)
> +{
> + struct cpu_user_regs *regs = &vp->arch.user_regs;
> + unsigned long long x;
> +
> + kdbp("*** Guest State ***\n");
> + kdbp("CR0: actual=0x%016llx, shadow=0x%016llx, gh_mask=%016llx\n",
> + (unsigned long long)vmr(GUEST_CR0),
vmr returns an unsigned long, why the mismatched format string+ cast?
This seems to duplicate a fairly large chunk of vmcs_dump_vcpu. If kdbp
and printk can't co-exist then perhaps make a common function which
takes a printer as a function pointer?
Same goes for the SVM version I think I saw before.
> +/* Flush VMCS on this cpu if it needs to:
> + * - Upon leaving kdb, the HVM cpu will resume in vmx_vmexit_handler() and
> + * do __vmreads. So, the VMCS pointer can't be left cleared.
> + * - Doing __vmpclear will set the vmx state to 'clear', so to resume a
> + * vmlaunch must be done and not vmresume. This means, we must clear
> + * arch_vmx->launched.
> + */
> +void kdb_curr_cpu_flush_vmcs(void)
Is this function actually kdb specific? Other than the printing it looks
like a pretty generic help to me.
> +{
> + struct domain *dp;
> + struct vcpu *vp;
> + int ccpu = smp_processor_id();
> + struct vmcs_struct *cvp = this_cpu(current_vmcs);
> +
> + if (this_cpu(current_vmcs) == NULL)
> + return; /* no HVM active on this CPU */
> +
> + kdbp("KDB:[%d] curvmcs:%lx/%lx\n", ccpu, cvp, virt_to_maddr(cvp));
> +
> + /* looks like we got one. unfortunately, current_vmcs points to vmcs
> + * and not VCPU, so we gotta search the entire list... */
> + for_each_domain (dp) {
> + if ( !(is_hvm_or_hyb_domain(dp)) || dp->is_dying)
> + continue;
> + for_each_vcpu (dp, vp) {
> + if ( vp->arch.hvm_vmx.vmcs == cvp ) {
> + __vmpclear(virt_to_maddr(vp->arch.hvm_vmx.vmcs));
> + __vmptrld(virt_to_maddr(vp->arch.hvm_vmx.vmcs));
> + vp->arch.hvm_vmx.launched = 0;
> + this_cpu(current_vmcs) = NULL;
> + kdbp("KDB:[%d] %d:%d current_vmcs:%lx flushed\n",
> + ccpu, dp->domain_id, vp->vcpu_id, cvp,
> virt_to_maddr(cvp));
> + }
> + }
> + }
> +}
> +
> +/*
> + * domid == 0 : display for all HVM domains (dom0 is never an HVM domain)
Not any more...
> + * vcpu id == -1 : display all vcpuids
> + * PreCondition: all HVM cpus (including current cpu) have flushed VMCS
> + */
> +void kdb_dump_vmcs(domid_t did, int vid)
> +{
> + struct domain *dp;
> + struct vcpu *vp;
> + struct vmcs_struct *vmcsp;
> + u64 addr = -1;
> +
> + ASSERT(!local_irq_is_enabled()); /* kdb should always run disabled */
> + __vmptrst(&addr);
> +
> + for_each_domain (dp) {
> + if ( !(is_hvm_or_hyb_domain(dp)) || dp->is_dying)
> + continue;
> + if (did != 0 && did != dp->domain_id)
> + continue;
> +
> + for_each_vcpu (dp, vp) {
> + if (vid != -1 && vid != vp->vcpu_id)
> + continue;
A lot of this scaffolding is the same as for svm. Perhaps pull it up
into a common hvm function with only the inner arch specific bit in a
per-VMX/SVM function?
> +
> + vmcsp = vp->arch.hvm_vmx.vmcs;
> + kdbp("VMCS %lx/%lx [domid:%d (%p) vcpu:%d (%p)]:\n", vmcsp,
> + virt_to_maddr(vmcsp), dp->domain_id, dp, vp->vcpu_id, vp);
> + __vmptrld(virt_to_maddr(vmcsp));
> + kdb_print_vmcs(vp);
> + __vmpclear(virt_to_maddr(vmcsp));
> + vp->arch.hvm_vmx.launched = 0;
> + }
> + kdbp("\n");
> + }
> + /* restore orig vmcs pointer for __vmreads in vmx_vmexit_handler() */
> + if (addr && addr != (u64)-1)
> + __vmptrld(addr);
> +}
> +#endif
>
> /*
> * Local variables:
> diff -r 32034d1914a6 xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/hvm/vmx/vmx.c Wed Aug 29 14:39:57 2012 -0700
> @@ -2183,11 +2183,14 @@
> printk("reason not known yet!");
> break;
> }
> -
> +#if defined(XEN_KDB_CONFIG)
> + kdbp("\n************* VMCS Area **************\n");
> + kdb_dump_vmcs(curr->domain->domain_id, (curr)->vcpu_id);
> +#else
> printk("************* VMCS Area **************\n");
> vmcs_dump_vcpu(curr);
Is kdb_dump_vmcs better than/different to vmcs_dump_vcpu in this
context?
> printk("**************************************\n");
> -
> +#endif
> domain_crash(curr->domain);
> }
>
> @@ -2415,6 +2418,12 @@
> write_debugreg(6, exit_qualification | 0xffff0ff0);
> if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
> goto exit_and_crash;
> +
> +#if defined(XEN_KDB_CONFIG)
> + /* TRAP_debug: IP points correctly to next instr */
> + if (kdb_handle_trap_entry(vector, regs))
> + break;
> +#endif
> domain_pause_for_debugger();
> break;
> case TRAP_int3:
> @@ -2423,6 +2432,13 @@
> if ( v->domain->debugger_attached )
> {
> update_guest_eip(); /* Safe: INT3 */
> +#if defined(XEN_KDB_CONFIG)
> + /* vmcs.IP points to bp, kdb expects bp+1. Hence after the
> above
> + * update_guest_eip which updates to bp+1. works for gdbsx
> too
> + */
> + if (kdb_handle_trap_entry(vector, regs))
> + break;
> +#endif
> current->arch.gdbsx_vcpu_event = TRAP_int3;
> domain_pause_for_debugger();
> break;
> @@ -2707,6 +2723,10 @@
> case EXIT_REASON_MONITOR_TRAP_FLAG:
> v->arch.hvm_vmx.exec_control &= ~CPU_BASED_MONITOR_TRAP_FLAG;
> vmx_update_cpu_exec_control(v);
> +#if defined(XEN_KDB_CONFIG)
> + if (kdb_handle_trap_entry(TRAP_debug, regs))
> + break;
> +#endif
> if ( v->arch.hvm_vcpu.single_step ) {
> hvm_memory_event_single_step(regs->eip);
> if ( v->domain->debugger_attached )
> diff -r 32034d1914a6 xen/arch/x86/irq.c
> --- a/xen/arch/x86/irq.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/irq.c Wed Aug 29 14:39:57 2012 -0700
> @@ -2305,3 +2305,29 @@
> return is_hvm_domain(d) && pirq &&
> pirq->arch.hvm.emuirq != IRQ_UNBOUND;
> }
> +
> +#ifdef XEN_KDB_CONFIG
> +void kdb_prnt_guest_mapped_irqs(void)
> +{
> + int irq, j;
> + char affstr[NR_CPUS/4+NR_CPUS/32+2]; /* courtesy dump_irqs() */
Can this share some code with dump_irqs then?
I don't see this construct there though -- but that magic expression
could use some explanation.
> +
> + kdbp("irq vec aff type domid:mapped-pirq pairs (all in decimal)\n");
> + for (irq=0; irq < nr_irqs; irq++) {
> + irq_desc_t *dp = irq_to_desc(irq);
> + struct arch_irq_desc *archp = &dp->arch;
> + irq_guest_action_t *actp = (irq_guest_action_t *)dp->action;
> +
> + if (!dp->handler ||dp->handler==&no_irq_type ||
> !(dp->status&IRQ_GUEST))
> + continue;
> +
> + cpumask_scnprintf(affstr, sizeof(affstr), dp->affinity);
> + kdbp("[%3ld] %3d %3s %-13s ", irq, archp->vector, affstr,
> + dp->handler->typename);
> + for (j=0; j < actp->nr_guests; j++)
> + kdbp("%03d:%04d ", actp->guest[j]->domain_id,
> + domain_irq_to_pirq(actp->guest[j], irq));
> + kdbp("\n");
> + }
> +}
> +#endif
> diff -r 32034d1914a6 xen/arch/x86/smp.c
> --- a/xen/arch/x86/smp.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/arch/x86/smp.c Wed Aug 29 14:39:57 2012 -0700
> @@ -273,7 +273,7 @@
> * Structure and data for smp_call_function()/on_selected_cpus().
> */
>
> -static void __smp_call_function_interrupt(void);
> +static void __smp_call_function_interrupt(struct cpu_user_regs *regs);
I think you can just use get_irq_regs in the functions which need it
instead of adding this to every irq path?
> static DEFINE_SPINLOCK(call_lock);
> static struct call_data_struct {
> void (*func) (void *info);
> diff -r 32034d1914a6 xen/common/sched_credit.c
> --- a/xen/common/sched_credit.c Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/common/sched_credit.c Wed Aug 29 14:39:57 2012 -0700
> @@ -1475,6 +1475,33 @@
> printk("\n");
> }
>
> +#ifdef XEN_KDB_CONFIG
> +static void kdb_csched_dump(int cpu)
> +{
> + struct csched_pcpu *pcpup = CSCHED_PCPU(cpu);
> + struct vcpu *scurrvp = (CSCHED_VCPU(current))->vcpu;
> + struct list_head *tmp, *runq = RUNQ(cpu);
> +
> + kdbp(" csched_pcpu: %p\n", pcpup);
> + kdbp(" curr csched:%p {vcpu:%p id:%d domid:%d}\n",
> (current)->sched_priv,
> + scurrvp, scurrvp->vcpu_id, scurrvp->domain->domain_id);
> + kdbp(" runq:\n");
> +
> + /* next is top of struct, so screw stupid, ugly hard to follow macros */
> + if (offsetof(struct csched_vcpu, runq_elem.next) != 0) {
> + kdbp("next is not first in struct csched_vcpu. please fixme\n");
er, that's why the stupid macros are there!
at least this could be a build time check!
There seems to be a lot of code in this patch which adds a kdb_FOO_dump
function which duplicates the content (if not the exact layout) of an
exsiting FOO_dump function but using kdbp instead of printk.
I think that a recipe for one or the other getting out of sync or
bit-rotting and should be replaced either with making printk usable
while in kdb or if that isn't possible by abstracting out the printing
function.
> + return; /* otherwise for loop will crash */
> + }
> + for (tmp = runq->next; tmp != runq; tmp = tmp->next) {
> +
> + struct csched_vcpu *csp = (struct csched_vcpu *)tmp;
> + struct vcpu *vp = csp->vcpu;
> + kdbp(" csp:%p pri:%02d vcpu: {p:%p id:%d domid:%d}\n", csp,
> + csp->pri, vp, vp->vcpu_id, vp->domain->domain_id);
> + };
> +}
> +#endif
> +
> static void
> csched_dump_pcpu(const struct scheduler *ops, int cpu)
> {
> @@ -1484,6 +1511,10 @@
> int loop;
> #define cpustr keyhandler_scratch
>
> +#ifdef XEN_KDB_CONFIG
> + kdb_csched_dump(cpu);
> + return;
Return ?
> +#endif
> spc = CSCHED_PCPU(cpu);
> runq = &spc->runq;
>
> diff -r 32034d1914a6 xen/include/xen/sched.h
> --- a/xen/include/xen/sched.h Thu Jun 07 19:46:57 2012 +0100
> +++ b/xen/include/xen/sched.h Wed Aug 29 14:39:57 2012 -0700
> @@ -576,11 +576,14 @@
> unsigned long hypercall_create_continuation(
> unsigned int op, const char *format, ...);
> void hypercall_cancel_continuation(void);
> -
> +#ifdef XEN_KDB_CONFIG
> +#define hypercall_preempt_check() (0)
Erm, really?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |