| Message ID | 59EA22FD0200007800188C19@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/10/17 15:23, Jan Beulich wrote: > Their state may be important to figure the reason for a crash. To not > further grow duplicate code, break out a helper function. > > I realize that (ab)using the control register array here may not be > considered the nicest solution, but it seems easier (and less overall > overhead) to do so compared to the alternative of introducing another > helper structure. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Printing this information is definitely a good idea. Reviewed-by Andrew Cooper <andrew.cooper3@citrix.com>, with two observations. > > --- a/xen/arch/x86/x86_64/traps.c > +++ b/xen/arch/x86/x86_64/traps.c > @@ -36,6 +36,21 @@ static void print_xen_info(void) > > enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; > /* (ab)use crs[5..7] for fs/gs bases. */ ? A comment to this effect at least makes it clear that this is intentional. > +static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8]) > +{ > + crs[0] = read_cr0(); > + crs[2] = read_cr2(); > + crs[3] = read_cr3(); > + crs[4] = read_cr4(); > + regs->ds = read_sreg(ds); > + regs->es = read_sreg(es); > + regs->fs = read_sreg(fs); > + regs->gs = read_sreg(gs); > + crs[5] = rdfsbase(); > + crs[6] = rdgsbase(); > + rdmsrl(MSR_SHADOW_GS_BASE, crs[7]); > +} > + > static void _show_registers( > const struct cpu_user_regs *regs, unsigned long crs[8], > enum context context, const struct vcpu *v) > @@ -146,6 +160,7 @@ void show_registers(const struct cpu_use > void vcpu_show_registers(const struct vcpu *v) > { > const struct cpu_user_regs *regs = &v->arch.user_regs; > + bool_t kernel = guest_kernel_mode(v, regs); Forward porting mishap? > unsigned long crs[8]; > > /* Only handle PV guests for now */ >
>>> On 20.10.17 at 16:56, <andrew.cooper3@citrix.com> wrote: > On 20/10/17 15:23, Jan Beulich wrote: >> Their state may be important to figure the reason for a crash. To not >> further grow duplicate code, break out a helper function. >> >> I realize that (ab)using the control register array here may not be >> considered the nicest solution, but it seems easier (and less overall >> overhead) to do so compared to the alternative of introducing another >> helper structure. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Printing this information is definitely a good idea. > > Reviewed-by Andrew Cooper <andrew.cooper3@citrix.com>, with two > observations. Thanks. >> --- a/xen/arch/x86/x86_64/traps.c >> +++ b/xen/arch/x86/x86_64/traps.c >> @@ -36,6 +36,21 @@ static void print_xen_info(void) >> >> enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; >> > > /* (ab)use crs[5..7] for fs/gs bases. */ > > ? A comment to this effect at least makes it clear that this is > intentional. Okay. >> +static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8]) >> +{ >> + crs[0] = read_cr0(); >> + crs[2] = read_cr2(); >> + crs[3] = read_cr3(); >> + crs[4] = read_cr4(); >> + regs->ds = read_sreg(ds); >> + regs->es = read_sreg(es); >> + regs->fs = read_sreg(fs); >> + regs->gs = read_sreg(gs); >> + crs[5] = rdfsbase(); >> + crs[6] = rdgsbase(); >> + rdmsrl(MSR_SHADOW_GS_BASE, crs[7]); >> +} >> + >> static void _show_registers( >> const struct cpu_user_regs *regs, unsigned long crs[8], >> enum context context, const struct vcpu *v) >> @@ -146,6 +160,7 @@ void show_registers(const struct cpu_use >> void vcpu_show_registers(const struct vcpu *v) >> { >> const struct cpu_user_regs *regs = &v->arch.user_regs; >> + bool_t kernel = guest_kernel_mode(v, regs); > > Forward porting mishap? Oops, indeed. Jan
--- a/xen/arch/x86/x86_64/traps.c +++ b/xen/arch/x86/x86_64/traps.c @@ -36,6 +36,21 @@ static void print_xen_info(void) enum context { CTXT_hypervisor, CTXT_pv_guest, CTXT_hvm_guest }; +static void read_registers(struct cpu_user_regs *regs, unsigned long crs[8]) +{ + crs[0] = read_cr0(); + crs[2] = read_cr2(); + crs[3] = read_cr3(); + crs[4] = read_cr4(); + regs->ds = read_sreg(ds); + regs->es = read_sreg(es); + regs->fs = read_sreg(fs); + regs->gs = read_sreg(gs); + crs[5] = rdfsbase(); + crs[6] = rdgsbase(); + rdmsrl(MSR_SHADOW_GS_BASE, crs[7]); +} + static void _show_registers( const struct cpu_user_regs *regs, unsigned long crs[8], enum context context, const struct vcpu *v) @@ -74,6 +89,8 @@ static void _show_registers( else printk("cr0: %016lx cr4: %016lx\n", crs[0], crs[4]); printk("cr3: %016lx cr2: %016lx\n", crs[3], crs[2]); + printk("fsb: %016lx gsb: %016lx gss: %016lx\n", + crs[5], crs[6], crs[7]); printk("ds: %04x es: %04x fs: %04x gs: %04x " "ss: %04x cs: %04x\n", regs->ds, regs->es, regs->fs, @@ -103,13 +120,18 @@ void show_registers(const struct cpu_use fault_regs.es = sreg.sel; hvm_get_segment_register(v, x86_seg_fs, &sreg); fault_regs.fs = sreg.sel; + fault_crs[5] = sreg.base; hvm_get_segment_register(v, x86_seg_gs, &sreg); fault_regs.gs = sreg.sel; + fault_crs[6] = sreg.base; hvm_get_segment_register(v, x86_seg_ss, &sreg); fault_regs.ss = sreg.sel; + fault_crs[7] = hvm_get_shadow_gs_base(v); } else { + read_registers(&fault_regs, fault_crs); + if ( guest_mode(regs) ) { context = CTXT_pv_guest; @@ -120,14 +142,6 @@ void show_registers(const struct cpu_use context = CTXT_hypervisor; fault_crs[2] = read_cr2(); } - - fault_crs[0] = read_cr0(); - fault_crs[3] = read_cr3(); - fault_crs[4] = read_cr4(); - fault_regs.ds = read_sreg(ds); - fault_regs.es = read_sreg(es); - fault_regs.fs = read_sreg(fs); - fault_regs.gs = read_sreg(gs); } print_xen_info(); @@ -146,6 +160,7 @@ void show_registers(const struct cpu_use void vcpu_show_registers(const struct vcpu *v) { const struct cpu_user_regs *regs = &v->arch.user_regs; + bool_t kernel = guest_kernel_mode(v, regs); unsigned long crs[8]; /* Only handle PV guests for now */ @@ -154,10 +169,13 @@ void vcpu_show_registers(const struct vc crs[0] = v->arch.pv_vcpu.ctrlreg[0]; crs[2] = arch_get_cr2(v); - crs[3] = pagetable_get_paddr(guest_kernel_mode(v, regs) ? + crs[3] = pagetable_get_paddr(kernel ? v->arch.guest_table : v->arch.guest_table_user); crs[4] = v->arch.pv_vcpu.ctrlreg[4]; + crs[5] = v->arch.pv_vcpu.fs_base; + crs[6 + !kernel] = v->arch.pv_vcpu.gs_base_kernel; + crs[7 - !kernel] = v->arch.pv_vcpu.gs_base_user; _show_registers(regs, crs, CTXT_pv_guest, v); } @@ -237,14 +255,7 @@ void do_double_fault(struct cpu_user_reg printk("*** DOUBLE FAULT ***\n"); print_xen_info(); - crs[0] = read_cr0(); - crs[2] = read_cr2(); - crs[3] = read_cr3(); - crs[4] = read_cr4(); - regs->ds = read_sreg(ds); - regs->es = read_sreg(es); - regs->fs = read_sreg(fs); - regs->gs = read_sreg(gs); + read_registers(regs, crs); printk("CPU: %d\n", cpu); _show_registers(regs, crs, CTXT_hypervisor, NULL);
Their state may be important to figure the reason for a crash. To not further grow duplicate code, break out a helper function. I realize that (ab)using the control register array here may not be considered the nicest solution, but it seems easier (and less overall overhead) to do so compared to the alternative of introducing another helper structure. Signed-off-by: Jan Beulich <jbeulich@suse.com>