| Message ID | 1434969694-7432-9-git-send-email-zhichao.huang@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 22, 2015 at 06:41:31PM +0800, Zhichao Huang wrote: > The trapping code keeps track of the state of the debug registers, > allowing for the switch code to implement a lazy switching strategy. > > Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org> > --- > arch/arm/include/asm/kvm_asm.h | 3 +++ > arch/arm/include/asm/kvm_host.h | 3 +++ > arch/arm/kernel/asm-offsets.c | 1 + > arch/arm/kvm/coproc.c | 39 ++++++++++++++++++++++++++++++++++++-- > arch/arm/kvm/interrupts_head.S | 42 +++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 86 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h > index ba65e05..4fb64cf 100644 > --- a/arch/arm/include/asm/kvm_asm.h > +++ b/arch/arm/include/asm/kvm_asm.h > @@ -64,6 +64,9 @@ > #define cp14_DBGDSCRext 65 /* Debug Status and Control external */ > #define NR_CP14_REGS 66 /* Number of regs (incl. invalid) */ > > +#define KVM_ARM_DEBUG_DIRTY_SHIFT 0 > +#define KVM_ARM_DEBUG_DIRTY (1 << KVM_ARM_DEBUG_DIRTY_SHIFT) > + > #define ARM_EXCEPTION_RESET 0 > #define ARM_EXCEPTION_UNDEFINED 1 > #define ARM_EXCEPTION_SOFTWARE 2 > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 3d16820..09b54bf 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -127,6 +127,9 @@ struct kvm_vcpu_arch { > /* System control coprocessor (cp14) */ > u32 cp14[NR_CP14_REGS]; > > + /* Debug state */ > + u32 debug_flags; > + > /* > * Anything that is not used directly from assembly code goes > * here. > diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c > index 9158de0..e876109 100644 > --- a/arch/arm/kernel/asm-offsets.c > +++ b/arch/arm/kernel/asm-offsets.c > @@ -185,6 +185,7 @@ int main(void) > DEFINE(VCPU_FIQ_REGS, offsetof(struct kvm_vcpu, arch.regs.fiq_regs)); > DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_pc)); > DEFINE(VCPU_CPSR, offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr)); > + DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags)); > DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); > DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); > DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); > diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c > index eeee648..fc0c2ef 100644 > --- a/arch/arm/kvm/coproc.c > +++ b/arch/arm/kvm/coproc.c > @@ -220,14 +220,49 @@ bool access_vm_reg(struct kvm_vcpu *vcpu, > return true; > } > > +/* > + * We want to avoid world-switching all the DBG registers all the > + * time: > + * > + * - If we've touched any debug register, it is likely that we're > + * going to touch more of them. It then makes sense to disable the > + * traps and start doing the save/restore dance > + * - If debug is active (ARM_DSCR_MDBGEN set), it is then mandatory > + * to save/restore the registers, as the guest depends on them. > + * > + * For this, we use a DIRTY bit, indicating the guest has modified the > + * debug registers, used as follow: > + * > + * On guest entry: > + * - If the dirty bit is set (because we're coming back from trapping), > + * disable the traps, save host registers, restore guest registers. > + * - If debug is actively in use (ARM_DSCR_MDBGEN set), > + * set the dirty bit, disable the traps, save host registers, > + * restore guest registers. > + * - Otherwise, enable the traps > + * > + * On guest exit: > + * - If the dirty bit is set, save guest registers, restore host > + * registers and clear the dirty bit. This ensure that the host can > + * now use the debug registers. > + * > + * Notice: > + * - For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set in the guest, > + * debug is always actively in use (ARM_DSCR_MDBGEN set). > + * We have to do the save/restore dance in this case, because the > + * host and the guest might use their respective debug registers > + * at any moment. so doesn't this pretty much invalidate the whole saving/dirty effort? Guests configured from for example multi_v7_defconfig will then act like this and you will save/restore all these registers always. Wouldn't a better approach be to enable trapping to hyp mode most of the time, and simply clear the enabled bit of any host-used break- or wathcpoints upon guest entry, perhaps maintaining a bitmap of which ones must be re-set when exiting the guest, and thereby drastically reducing the amount of save/restore code you'd have to perform. Of course, you'd also have to keep track of whether the guest has any breakpoints or watchpoints enabled for when you do the full save/restore dance. That would also avoid all issues surrounding accesses to DBGDSCRext register I think. > + */ > static bool trap_debug32(struct kvm_vcpu *vcpu, > const struct coproc_params *p, > const struct coproc_reg *r) > { > - if (p->is_write) > + if (p->is_write) { > vcpu->arch.cp14[r->reg] = *vcpu_reg(vcpu, p->Rt1); > - else > + vcpu->arch.debug_flags |= KVM_ARM_DEBUG_DIRTY; > + } else { > *vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg]; > + } > > return true; > } > diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S > index a20b9ad..5662c39 100644 > --- a/arch/arm/kvm/interrupts_head.S > +++ b/arch/arm/kvm/interrupts_head.S > @@ -1,4 +1,6 @@ > #include <linux/irqchip/arm-gic.h> > +#include <asm/hw_breakpoint.h> > +#include <asm/kvm_asm.h> > #include <asm/assembler.h> > > #define VCPU_USR_REG(_reg_nr) (VCPU_USR_REGS + (_reg_nr * 4)) > @@ -407,6 +409,46 @@ vcpu .req r0 @ vcpu pointer always in r0 > mcr p15, 2, r12, c0, c0, 0 @ CSSELR > .endm > > +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ > +.macro skip_debug_state target > + ldr r5, [vcpu, #VCPU_DEBUG_FLAGS] > + cmp r5, #KVM_ARM_DEBUG_DIRTY > + bne \target > +1: > +.endm > + > +/* Compute debug state: If ARM_DSCR_MDBGEN or KVM_ARM_DEBUG_DIRTY > + * is set, we do a full save/restore cycle and disable trapping. > + * > + * Assumes vcpu pointer in vcpu reg > + * > + * Clobbers r5, r6 > + */ > +.macro compute_debug_state target > + // Check the state of MDSCR_EL1 > + ldr r5, [vcpu, #CP14_OFFSET(cp14_DBGDSCRext)] > + and r6, r5, #ARM_DSCR_MDBGEN > + cmp r6, #0 you can just do 'ands' here, or even tst and you don't have to touch r6. > + beq 9998f // Nothing to see there > + > + // If ARM_DSCR_MDBGEN bit was set, we must set the flag > + mov r5, #KVM_ARM_DEBUG_DIRTY > + str r5, [vcpu, #VCPU_DEBUG_FLAGS] > + b 9999f // Don't skip restore > + > +9998: > + // Otherwise load the flags from memory in case we recently > + // trapped > + skip_debug_state \target > +9999: > +.endm > + > +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ > +.macro clear_debug_dirty_bit > + mov r5, #0 > + str r5, [vcpu, #VCPU_DEBUG_FLAGS] > +.endm > + > /* > * Save the VGIC CPU state into memory > * > -- > 1.7.12.4 > Thanks, -Christoffer
On June 30, 2015 5:20:20 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote: >On Mon, Jun 22, 2015 at 06:41:31PM +0800, Zhichao Huang wrote: >> The trapping code keeps track of the state of the debug registers, >> allowing for the switch code to implement a lazy switching strategy. >> >> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org> >> --- >> arch/arm/include/asm/kvm_asm.h | 3 +++ >> arch/arm/include/asm/kvm_host.h | 3 +++ >> arch/arm/kernel/asm-offsets.c | 1 + >> arch/arm/kvm/coproc.c | 39 ++++++++++++++++++++++++++++++++++++-- >> arch/arm/kvm/interrupts_head.S | 42 >+++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 86 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h >> index ba65e05..4fb64cf 100644 >> --- a/arch/arm/include/asm/kvm_asm.h >> +++ b/arch/arm/include/asm/kvm_asm.h >> @@ -64,6 +64,9 @@ >> #define cp14_DBGDSCRext 65 /* Debug Status and Control external */ >> #define NR_CP14_REGS 66 /* Number of regs (incl. invalid) */ >> >> +#define KVM_ARM_DEBUG_DIRTY_SHIFT 0 >> +#define KVM_ARM_DEBUG_DIRTY (1 << KVM_ARM_DEBUG_DIRTY_SHIFT) >> + >> #define ARM_EXCEPTION_RESET 0 >> #define ARM_EXCEPTION_UNDEFINED 1 >> #define ARM_EXCEPTION_SOFTWARE 2 >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 3d16820..09b54bf 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -127,6 +127,9 @@ struct kvm_vcpu_arch { >> /* System control coprocessor (cp14) */ >> u32 cp14[NR_CP14_REGS]; >> >> + /* Debug state */ >> + u32 debug_flags; >> + >> /* >> * Anything that is not used directly from assembly code goes >> * here. >> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >> index 9158de0..e876109 100644 >> --- a/arch/arm/kernel/asm-offsets.c >> +++ b/arch/arm/kernel/asm-offsets.c >> @@ -185,6 +185,7 @@ int main(void) >> DEFINE(VCPU_FIQ_REGS, offsetof(struct kvm_vcpu, >arch.regs.fiq_regs)); >> DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, >arch.regs.usr_regs.ARM_pc)); >> DEFINE(VCPU_CPSR, offsetof(struct kvm_vcpu, >arch.regs.usr_regs.ARM_cpsr)); >> + DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, >arch.debug_flags)); >> DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); >> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); >> DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c >> index eeee648..fc0c2ef 100644 >> --- a/arch/arm/kvm/coproc.c >> +++ b/arch/arm/kvm/coproc.c >> @@ -220,14 +220,49 @@ bool access_vm_reg(struct kvm_vcpu *vcpu, >> return true; >> } >> >> +/* >> + * We want to avoid world-switching all the DBG registers all the >> + * time: >> + * >> + * - If we've touched any debug register, it is likely that we're >> + * going to touch more of them. It then makes sense to disable the >> + * traps and start doing the save/restore dance >> + * - If debug is active (ARM_DSCR_MDBGEN set), it is then mandatory >> + * to save/restore the registers, as the guest depends on them. >> + * >> + * For this, we use a DIRTY bit, indicating the guest has modified >the >> + * debug registers, used as follow: >> + * >> + * On guest entry: >> + * - If the dirty bit is set (because we're coming back from >trapping), >> + * disable the traps, save host registers, restore guest >registers. >> + * - If debug is actively in use (ARM_DSCR_MDBGEN set), >> + * set the dirty bit, disable the traps, save host registers, >> + * restore guest registers. >> + * - Otherwise, enable the traps >> + * >> + * On guest exit: >> + * - If the dirty bit is set, save guest registers, restore host >> + * registers and clear the dirty bit. This ensure that the host can >> + * now use the debug registers. >> + * >> + * Notice: >> + * - For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set in the guest, >> + * debug is always actively in use (ARM_DSCR_MDBGEN set). >> + * We have to do the save/restore dance in this case, because the >> + * host and the guest might use their respective debug registers >> + * at any moment. > >so doesn't this pretty much invalidate the whole saving/dirty effort? > >Guests configured from for example multi_v7_defconfig will then act >like >this and you will save/restore all these registers always. > >Wouldn't a better approach be to enable trapping to hyp mode most of >the >time, and simply clear the enabled bit of any host-used break- or >wathcpoints upon guest entry, perhaps maintaining a bitmap of which >ones >must be re-set when exiting the guest, and thereby drastically reducing >the amount of save/restore code you'd have to perform. > >Of course, you'd also have to keep track of whether the guest has any >breakpoints or watchpoints enabled for when you do the full >save/restore >dance. > >That would also avoid all issues surrounding accesses to DBGDSCRext >register I think. I have thought about it, which means to say, "Since we can't find whether the guest has any hwbrpts enabled from the DBGDSCR, why don't we find it from the DBGBCR and DBGWCR?". Case 1: The host and the guest enable all the hwbrpts. It's necessary to world switch the debug registers all the time. Case 2: The host and the guest enable some of the hwbrpts. It's necessary to world switch the debug registers which are enabled. But if we want skip thouse registers which aren't enabled, we have to keep track of all the debug states both in the host and the guest. We need to judge which debug registers we should switch, and which not. It may bring in a complex logic in the assembly code. And if the host or guest enabled almost all of the hwbrpts, this operation may bring in the loss outweights the grain. Is it acceptable and worthy? If yes, I will do it. Case 3: Neither the host nor the guest enable any hwbrpts. It's the case that we can skip the whole world switch thing. The only problem is that we have to read all the debug registers on each guest entry to find whether the host enable any hwbrpts or not. But this case is a normal case, which is worthy to do the efforts to reduce the saving/restoring overhead. > >> + */ >> static bool trap_debug32(struct kvm_vcpu *vcpu, >> const struct coproc_params *p, >> const struct coproc_reg *r) >> { >> - if (p->is_write) >> + if (p->is_write) { >> vcpu->arch.cp14[r->reg] = *vcpu_reg(vcpu, p->Rt1); >> - else >> + vcpu->arch.debug_flags |= KVM_ARM_DEBUG_DIRTY; >> + } else { >> *vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg]; >> + } >> >> return true; >> } >> diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S >> index a20b9ad..5662c39 100644 >> --- a/arch/arm/kvm/interrupts_head.S >> +++ b/arch/arm/kvm/interrupts_head.S >> @@ -1,4 +1,6 @@ >> #include <linux/irqchip/arm-gic.h> >> +#include <asm/hw_breakpoint.h> >> +#include <asm/kvm_asm.h> >> #include <asm/assembler.h> >> >> #define VCPU_USR_REG(_reg_nr) (VCPU_USR_REGS + (_reg_nr * 4)) >> @@ -407,6 +409,46 @@ vcpu .req r0 @ vcpu pointer always in r0 >> mcr p15, 2, r12, c0, c0, 0 @ CSSELR >> .endm >> >> +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ >> +.macro skip_debug_state target >> + ldr r5, [vcpu, #VCPU_DEBUG_FLAGS] >> + cmp r5, #KVM_ARM_DEBUG_DIRTY >> + bne \target >> +1: >> +.endm >> + >> +/* Compute debug state: If ARM_DSCR_MDBGEN or KVM_ARM_DEBUG_DIRTY >> + * is set, we do a full save/restore cycle and disable trapping. >> + * >> + * Assumes vcpu pointer in vcpu reg >> + * >> + * Clobbers r5, r6 >> + */ >> +.macro compute_debug_state target >> + // Check the state of MDSCR_EL1 >> + ldr r5, [vcpu, #CP14_OFFSET(cp14_DBGDSCRext)] >> + and r6, r5, #ARM_DSCR_MDBGEN >> + cmp r6, #0 > >you can just do 'ands' here, or even tst and you don't have to touch >r6. OK. Thanks for pointing out. > >> + beq 9998f // Nothing to see there >> + >> + // If ARM_DSCR_MDBGEN bit was set, we must set the flag >> + mov r5, #KVM_ARM_DEBUG_DIRTY >> + str r5, [vcpu, #VCPU_DEBUG_FLAGS] >> + b 9999f // Don't skip restore >> + >> +9998: >> + // Otherwise load the flags from memory in case we recently >> + // trapped >> + skip_debug_state \target >> +9999: >> +.endm >> + >> +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ >> +.macro clear_debug_dirty_bit >> + mov r5, #0 >> + str r5, [vcpu, #VCPU_DEBUG_FLAGS] >> +.endm >> + >> /* >> * Save the VGIC CPU state into memory >> * >> -- >> 1.7.12.4 >> >Thanks, >-Christoffer
On Fri, Jul 03, 2015 at 05:54:47PM +0800, Zhichao Huang wrote: > > > On June 30, 2015 5:20:20 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote: > >On Mon, Jun 22, 2015 at 06:41:31PM +0800, Zhichao Huang wrote: > >> The trapping code keeps track of the state of the debug registers, > >> allowing for the switch code to implement a lazy switching strategy. > >> > >> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org> > >> --- > >> arch/arm/include/asm/kvm_asm.h | 3 +++ > >> arch/arm/include/asm/kvm_host.h | 3 +++ > >> arch/arm/kernel/asm-offsets.c | 1 + > >> arch/arm/kvm/coproc.c | 39 ++++++++++++++++++++++++++++++++++++-- > >> arch/arm/kvm/interrupts_head.S | 42 > >+++++++++++++++++++++++++++++++++++++++++ > >> 5 files changed, 86 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h > >> index ba65e05..4fb64cf 100644 > >> --- a/arch/arm/include/asm/kvm_asm.h > >> +++ b/arch/arm/include/asm/kvm_asm.h > >> @@ -64,6 +64,9 @@ > >> #define cp14_DBGDSCRext 65 /* Debug Status and Control external */ > >> #define NR_CP14_REGS 66 /* Number of regs (incl. invalid) */ > >> > >> +#define KVM_ARM_DEBUG_DIRTY_SHIFT 0 > >> +#define KVM_ARM_DEBUG_DIRTY (1 << KVM_ARM_DEBUG_DIRTY_SHIFT) > >> + > >> #define ARM_EXCEPTION_RESET 0 > >> #define ARM_EXCEPTION_UNDEFINED 1 > >> #define ARM_EXCEPTION_SOFTWARE 2 > >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > >> index 3d16820..09b54bf 100644 > >> --- a/arch/arm/include/asm/kvm_host.h > >> +++ b/arch/arm/include/asm/kvm_host.h > >> @@ -127,6 +127,9 @@ struct kvm_vcpu_arch { > >> /* System control coprocessor (cp14) */ > >> u32 cp14[NR_CP14_REGS]; > >> > >> + /* Debug state */ > >> + u32 debug_flags; > >> + > >> /* > >> * Anything that is not used directly from assembly code goes > >> * here. > >> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c > >> index 9158de0..e876109 100644 > >> --- a/arch/arm/kernel/asm-offsets.c > >> +++ b/arch/arm/kernel/asm-offsets.c > >> @@ -185,6 +185,7 @@ int main(void) > >> DEFINE(VCPU_FIQ_REGS, offsetof(struct kvm_vcpu, > >arch.regs.fiq_regs)); > >> DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, > >arch.regs.usr_regs.ARM_pc)); > >> DEFINE(VCPU_CPSR, offsetof(struct kvm_vcpu, > >arch.regs.usr_regs.ARM_cpsr)); > >> + DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, > >arch.debug_flags)); > >> DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); > >> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); > >> DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); > >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c > >> index eeee648..fc0c2ef 100644 > >> --- a/arch/arm/kvm/coproc.c > >> +++ b/arch/arm/kvm/coproc.c > >> @@ -220,14 +220,49 @@ bool access_vm_reg(struct kvm_vcpu *vcpu, > >> return true; > >> } > >> > >> +/* > >> + * We want to avoid world-switching all the DBG registers all the > >> + * time: > >> + * > >> + * - If we've touched any debug register, it is likely that we're > >> + * going to touch more of them. It then makes sense to disable the > >> + * traps and start doing the save/restore dance > >> + * - If debug is active (ARM_DSCR_MDBGEN set), it is then mandatory > >> + * to save/restore the registers, as the guest depends on them. > >> + * > >> + * For this, we use a DIRTY bit, indicating the guest has modified > >the > >> + * debug registers, used as follow: > >> + * > >> + * On guest entry: > >> + * - If the dirty bit is set (because we're coming back from > >trapping), > >> + * disable the traps, save host registers, restore guest > >registers. > >> + * - If debug is actively in use (ARM_DSCR_MDBGEN set), > >> + * set the dirty bit, disable the traps, save host registers, > >> + * restore guest registers. > >> + * - Otherwise, enable the traps > >> + * > >> + * On guest exit: > >> + * - If the dirty bit is set, save guest registers, restore host > >> + * registers and clear the dirty bit. This ensure that the host can > >> + * now use the debug registers. > >> + * > >> + * Notice: > >> + * - For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set in the guest, > >> + * debug is always actively in use (ARM_DSCR_MDBGEN set). > >> + * We have to do the save/restore dance in this case, because the > >> + * host and the guest might use their respective debug registers > >> + * at any moment. > > > >so doesn't this pretty much invalidate the whole saving/dirty effort? > > > >Guests configured from for example multi_v7_defconfig will then act > >like > >this and you will save/restore all these registers always. > > > >Wouldn't a better approach be to enable trapping to hyp mode most of > >the > >time, and simply clear the enabled bit of any host-used break- or > >wathcpoints upon guest entry, perhaps maintaining a bitmap of which > >ones > >must be re-set when exiting the guest, and thereby drastically reducing > >the amount of save/restore code you'd have to perform. > > > >Of course, you'd also have to keep track of whether the guest has any > >breakpoints or watchpoints enabled for when you do the full > >save/restore > >dance. > > > >That would also avoid all issues surrounding accesses to DBGDSCRext > >register I think. > > I have thought about it, which means to say, "Since we can't find > whether the guest has any hwbrpts enabled from the DBGDSCR, why > don't we find it from the DBGBCR and DBGWCR?". > > Case 1: The host and the guest enable all the hwbrpts. > It's necessary to world switch the debug registers all the time. > > Case 2: The host and the guest enable some of the hwbrpts. > It's necessary to world switch the debug registers which are enabled. > But if we want skip thouse registers which aren't enabled, we have to > keep track of all the debug states both in the host and the guest. > We need to judge which debug registers we should switch, and which > not. It may bring in a complex logic in the assembly code. And if the > host or guest enabled almost all of the hwbrpts, this operation may > bring in the loss outweights the grain. > Is it acceptable and worthy? If yes, I will do it. > > Case 3: Neither the host nor the guest enable any hwbrpts. > It's the case that we can skip the whole world switch thing. > The only problem is that we have to read all the debug registers on each > guest entry to find whether the host enable any hwbrpts or not. > But this case is a normal case, which is worthy to do the efforts to > reduce the saving/restoring overhead. > I would never try to do a partial save/restore, just look at the control registers to see if anything is enabled as an indication of whether or not we need to do the save/restore of all the registers and disable trapping. Reading the guest control registers (from memory) only should be much faster than saving/restoring the whole lot. Perhaps there's even a hook in Linux to figure out if any of the registers are being used? Thanks, -Christoffer
Hi, Will, Chazy and me are talking about how to reduce the saving/restoring overhead for debug registers. We want to add a state in hw_breakpoint.c to indicate whether the host enable any hwbrpts or not (might export a fuction that kvm can call), then we can read this state from memory instead of reading from real hardware registers, and to decide whether we need a world switch or not. Does it acceptable? On July 3, 2015 7:56:11 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote: >On Fri, Jul 03, 2015 at 05:54:47PM +0800, Zhichao Huang wrote: >> >> >> On June 30, 2015 5:20:20 PM GMT+08:00, Christoffer Dall <christoffer.dall@linaro.org> wrote: >> >On Mon, Jun 22, 2015 at 06:41:31PM +0800, Zhichao Huang wrote: >> >> The trapping code keeps track of the state of the debug registers, >> >> allowing for the switch code to implement a lazy switching strategy. >> >> >> >> Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org> >> >> --- >> >> arch/arm/include/asm/kvm_asm.h | 3 +++ >> >> arch/arm/include/asm/kvm_host.h | 3 +++ >> >> arch/arm/kernel/asm-offsets.c | 1 + >> >> arch/arm/kvm/coproc.c | 39 ++++++++++++++++++++++++++++++++++++-- >> >> arch/arm/kvm/interrupts_head.S | 42 +++++++++++++++++++++++++++++++++++++++++ >> >> 5 files changed, 86 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h >> >> index ba65e05..4fb64cf 100644 >> >> --- a/arch/arm/include/asm/kvm_asm.h >> >> +++ b/arch/arm/include/asm/kvm_asm.h >> >> @@ -64,6 +64,9 @@ >> >> #define cp14_DBGDSCRext 65 /* Debug Status and Control external */ >> >> #define NR_CP14_REGS 66 /* Number of regs (incl. invalid) */ >> >> >> >> +#define KVM_ARM_DEBUG_DIRTY_SHIFT 0 >> >> +#define KVM_ARM_DEBUG_DIRTY (1 << KVM_ARM_DEBUG_DIRTY_SHIFT) >> >> + >> >> #define ARM_EXCEPTION_RESET 0 >> >> #define ARM_EXCEPTION_UNDEFINED 1 >> >> #define ARM_EXCEPTION_SOFTWARE 2 >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> >> index 3d16820..09b54bf 100644 >> >> --- a/arch/arm/include/asm/kvm_host.h >> >> +++ b/arch/arm/include/asm/kvm_host.h >> >> @@ -127,6 +127,9 @@ struct kvm_vcpu_arch { >> >> /* System control coprocessor (cp14) */ >> >> u32 cp14[NR_CP14_REGS]; >> >> >> >> + /* Debug state */ >> >> + u32 debug_flags; >> >> + >> >> /* >> >> * Anything that is not used directly from assembly code goes >> >> * here. >> >> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >> >> index 9158de0..e876109 100644 >> >> --- a/arch/arm/kernel/asm-offsets.c >> >> +++ b/arch/arm/kernel/asm-offsets.c >> >> @@ -185,6 +185,7 @@ int main(void) >> >> DEFINE(VCPU_FIQ_REGS, offsetof(struct kvm_vcpu, >> >arch.regs.fiq_regs)); >> >> DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, >> >arch.regs.usr_regs.ARM_pc)); >> >> DEFINE(VCPU_CPSR, offsetof(struct kvm_vcpu, >> >arch.regs.usr_regs.ARM_cpsr)); >> >> + DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, >> >arch.debug_flags)); >> >> DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); >> >> DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); >> >> DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); >> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c >> >> index eeee648..fc0c2ef 100644 >> >> --- a/arch/arm/kvm/coproc.c >> >> +++ b/arch/arm/kvm/coproc.c >> >> @@ -220,14 +220,49 @@ bool access_vm_reg(struct kvm_vcpu *vcpu, >> >> return true; >> >> } >> >> >> >> +/* >> >> + * We want to avoid world-switching all the DBG registers all the >> >> + * time: >> >> + * >> >> + * - If we've touched any debug register, it is likely that we're >> >> + * going to touch more of them. It then makes sense to disable the >> >> + * traps and start doing the save/restore dance >> >> + * - If debug is active (ARM_DSCR_MDBGEN set), it is then mandatory >> >> + * to save/restore the registers, as the guest depends on them. >> >> + * >> >> + * For this, we use a DIRTY bit, indicating the guest has modified the >> >> + * debug registers, used as follow: >> >> + * >> >> + * On guest entry: >> >> + * - If the dirty bit is set (because we're coming back from trapping), >> >> + * disable the traps, save host registers, restore guest registers. >> >> + * - If debug is actively in use (ARM_DSCR_MDBGEN set), >> >> + * set the dirty bit, disable the traps, save host registers, >> >> + * restore guest registers. >> >> + * - Otherwise, enable the traps >> >> + * >> >> + * On guest exit: >> >> + * - If the dirty bit is set, save guest registers, restore host >> >> + * registers and clear the dirty bit. This ensure that the host can >> >> + * now use the debug registers. >> >> + * >> >> + * Notice: >> >> + * - For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set in the guest, >> >> + * debug is always actively in use (ARM_DSCR_MDBGEN set). >> >> + * We have to do the save/restore dance in this case, because the >> >> + * host and the guest might use their respective debug registers >> >> + * at any moment. >> > >> >so doesn't this pretty much invalidate the whole saving/dirty effort? >> > >> >Guests configured from for example multi_v7_defconfig will then act >> >like >> >this and you will save/restore all these registers always. >> > >> >Wouldn't a better approach be to enable trapping to hyp mode most of >> >the >> >time, and simply clear the enabled bit of any host-used break- or >> >wathcpoints upon guest entry, perhaps maintaining a bitmap of which >> >ones >> >must be re-set when exiting the guest, and thereby drastically >reducing >> >the amount of save/restore code you'd have to perform. >> > >> >Of course, you'd also have to keep track of whether the guest has any >> >breakpoints or watchpoints enabled for when you do the full >> >save/restore >> >dance. >> > >> >That would also avoid all issues surrounding accesses to DBGDSCRext >> >register I think. >> >> I have thought about it, which means to say, "Since we can't find >> whether the guest has any hwbrpts enabled from the DBGDSCR, why >> don't we find it from the DBGBCR and DBGWCR?". >> >> Case 1: The host and the guest enable all the hwbrpts. >> It's necessary to world switch the debug registers all the time. >> >> Case 2: The host and the guest enable some of the hwbrpts. >> It's necessary to world switch the debug registers which are enabled. >> But if we want skip thouse registers which aren't enabled, we have to >> keep track of all the debug states both in the host and the guest. >> We need to judge which debug registers we should switch, and which >> not. It may bring in a complex logic in the assembly code. And if the >> host or guest enabled almost all of the hwbrpts, this operation may >> bring in the loss outweights the grain. >> Is it acceptable and worthy? If yes, I will do it. >> >> Case 3: Neither the host nor the guest enable any hwbrpts. >> It's the case that we can skip the whole world switch thing. >> The only problem is that we have to read all the debug registers on each >> guest entry to find whether the host enable any hwbrpts or not. >> But this case is a normal case, which is worthy to do the efforts to >> reduce the saving/restoring overhead. >> >I would never try to do a partial save/restore, just look at the >control registers to see if anything is enabled as an indication of >whether or not we need to do the save/restore of all the registers and >disable trapping. > >Reading the guest control registers (from memory) only should be much >faster than saving/restoring the whole lot. Perhaps there's even a >hook >in Linux to figure out if any of the registers are being used? > >Thanks, >-Christoffer
Hi, Will,
Are you happy with this?:
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
+bool hw_breakpoint_enabled(void)
+{
+ struct perf_event **slots;
+ int i;
+
+ slots = this_cpu_ptr(bp_on_reg);
+ for (i = 0; i < core_num_brps; i++) {
+ if (slots[i])
+ return true;
+ }
+
+ slots = this_cpu_ptr(wp_on_reg);
+ for (i = 0; i < core_num_wrps; i++) {
+ if (slots[i])
+ return true;
+ }
+
+ return false;
+}
It doesn't change any existing functions, and even doesn't add a new
variables, it just provide an indication for KVM, and it's low-overhead.
We will only call it upon guest entry, so there is also no race for it.
On July 7, 2015 6:24:06 PM GMT+08:00, Will Deacon <will.deacon@arm.com> wrote:
>On Tue, Jul 07, 2015 at 11:06:57AM +0100, Zhichao Huang wrote:
>> Chazy and me are talking about how to reduce the saving/restoring
>> overhead for debug registers.
>> We want to add a state in hw_breakpoint.c to indicate whether the
>host
>> enable any hwbrpts or not (might export a fuction that kvm can call),
>> then we can read this state from memory instead of reading from real
>> hardware registers, and to decide whether we need a world switch or
>> not.
>> Does it acceptable?
>
>Maybe, hard to tell without the code. There are obvious races to deal
>with
>if you use variables to indicate whether resources are in use -- why
>not
>just trap debug access from the host as well? Then you could keep track
>of
>the "owner" in kvm and trap accesses from everybody else.
>
>Will
On Tue, Jul 07, 2015 at 11:24:06AM +0100, Will Deacon wrote: > On Tue, Jul 07, 2015 at 11:06:57AM +0100, Zhichao Huang wrote: > > Chazy and me are talking about how to reduce the saving/restoring > > overhead for debug registers. > > We want to add a state in hw_breakpoint.c to indicate whether the host > > enable any hwbrpts or not (might export a fuction that kvm can call), > > then we can read this state from memory instead of reading from real > > hardware registers, and to decide whether we need a world switch or > > not. > > Does it acceptable? > > Maybe, hard to tell without the code. There are obvious races to deal with > if you use variables to indicate whether resources are in use -- why not > just trap debug access from the host as well? Then you could keep track of > the "owner" in kvm and trap accesses from everybody else. > The only information we're looking for here is whether the host has enabled some break/watch point so that we need to disable them before running the guest. Just to re-iterate, when we are about to run a guest, we have the following cases: 1) Neither the host nor the guest has configured any [WB]points 2) Only the host has configured any [WB]points 3) Only the guest has configured any [WB]points 4) Both the host and the guest have configured any [WB]points In case (1), KVM should enable trapping and swtich the register state on guest accesses. In cases (2), (3), and (4) we must switch the register state on each entry/exit. If we are to trap debug register accesses in KVM to set a flag to keep track of the owner (iow. has the host touched the registers) then don't we impose an ordering requirement of whether KVM or the breakpoint functionality gets initialized first, and we need to take special care when tearing down KVM to disable the traps? It sounds a little complex. I've previously suggested to simply look at the B/W control registers to figure out what to do. Caching the state in memory is an optimization, do we even have any idea how important such an optimization is? Thanks, -Christoffer
On July 9, 2015 1:08:55 AM GMT+08:00, Will Deacon <will.deacon@arm.com> wrote: >On Wed, Jul 08, 2015 at 11:50:22AM +0100, Zhichao Huang wrote: >> Are you happy with this?: > >You miss the reserved breakpoint, I think. Sorry, I can't quite understand. What is the reserved breakpoint? When arch_[iu]ninstall_hw_breakpoint is called, it only set/clear the brps and wrps slots. >I also still don't understand why this is preferable to trapping. > >Will
On 2015/7/9 19:50, Christoffer Dall wrote: > On Tue, Jul 07, 2015 at 11:24:06AM +0100, Will Deacon wrote: >> On Tue, Jul 07, 2015 at 11:06:57AM +0100, Zhichao Huang wrote: >>> Chazy and me are talking about how to reduce the saving/restoring >>> overhead for debug registers. >>> We want to add a state in hw_breakpoint.c to indicate whether the host >>> enable any hwbrpts or not (might export a fuction that kvm can call), >>> then we can read this state from memory instead of reading from real >>> hardware registers, and to decide whether we need a world switch or >>> not. >>> Does it acceptable? >> >> Maybe, hard to tell without the code. There are obvious races to deal with >> if you use variables to indicate whether resources are in use -- why not >> just trap debug access from the host as well? Then you could keep track of >> the "owner" in kvm and trap accesses from everybody else. >> > The only information we're looking for here is whether the host has > enabled some break/watch point so that we need to disable them before > running the guest. > > Just to re-iterate, when we are about to run a guest, we have the > following cases: > > 1) Neither the host nor the guest has configured any [WB]points > 2) Only the host has configured any [WB]points > 3) Only the guest has configured any [WB]points > 4) Both the host and the guest have configured any [WB]points > > In case (1), KVM should enable trapping and swtich the register state on > guest accesses. > > In cases (2), (3), and (4) we must switch the register state on each > entry/exit. > > If we are to trap debug register accesses in KVM to set a flag to keep > track of the owner (iow. has the host touched the registers) then don't > we impose an ordering requirement of whether KVM or the breakpoint > functionality gets initialized first, and we need to take special care > when tearing down KVM to disable the traps? It sounds a little complex. > > I've previously suggested to simply look at the B/W control registers to > figure out what to do. Caching the state in memory is an optimization, > do we even have any idea how important such an optimization is? > I have a test for the overhead both in el1 and el2 on D01 board(ARMv7). Each "MRC p14 ..." instruction cost 8 cycles, and Each "MCR p14 ..." cost 5 cycles. A15 has 6 breakpoints and 4 watchpoints, which gives us a total of 20 registers. and the overhead in each world switch is at least (20*8 + 20*5 = 260)cycles. > Thanks, > -Christoffer >
diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h index ba65e05..4fb64cf 100644 --- a/arch/arm/include/asm/kvm_asm.h +++ b/arch/arm/include/asm/kvm_asm.h @@ -64,6 +64,9 @@ #define cp14_DBGDSCRext 65 /* Debug Status and Control external */ #define NR_CP14_REGS 66 /* Number of regs (incl. invalid) */ +#define KVM_ARM_DEBUG_DIRTY_SHIFT 0 +#define KVM_ARM_DEBUG_DIRTY (1 << KVM_ARM_DEBUG_DIRTY_SHIFT) + #define ARM_EXCEPTION_RESET 0 #define ARM_EXCEPTION_UNDEFINED 1 #define ARM_EXCEPTION_SOFTWARE 2 diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 3d16820..09b54bf 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -127,6 +127,9 @@ struct kvm_vcpu_arch { /* System control coprocessor (cp14) */ u32 cp14[NR_CP14_REGS]; + /* Debug state */ + u32 debug_flags; + /* * Anything that is not used directly from assembly code goes * here. diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c index 9158de0..e876109 100644 --- a/arch/arm/kernel/asm-offsets.c +++ b/arch/arm/kernel/asm-offsets.c @@ -185,6 +185,7 @@ int main(void) DEFINE(VCPU_FIQ_REGS, offsetof(struct kvm_vcpu, arch.regs.fiq_regs)); DEFINE(VCPU_PC, offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_pc)); DEFINE(VCPU_CPSR, offsetof(struct kvm_vcpu, arch.regs.usr_regs.ARM_cpsr)); + DEFINE(VCPU_DEBUG_FLAGS, offsetof(struct kvm_vcpu, arch.debug_flags)); DEFINE(VCPU_HCR, offsetof(struct kvm_vcpu, arch.hcr)); DEFINE(VCPU_IRQ_LINES, offsetof(struct kvm_vcpu, arch.irq_lines)); DEFINE(VCPU_HSR, offsetof(struct kvm_vcpu, arch.fault.hsr)); diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c index eeee648..fc0c2ef 100644 --- a/arch/arm/kvm/coproc.c +++ b/arch/arm/kvm/coproc.c @@ -220,14 +220,49 @@ bool access_vm_reg(struct kvm_vcpu *vcpu, return true; } +/* + * We want to avoid world-switching all the DBG registers all the + * time: + * + * - If we've touched any debug register, it is likely that we're + * going to touch more of them. It then makes sense to disable the + * traps and start doing the save/restore dance + * - If debug is active (ARM_DSCR_MDBGEN set), it is then mandatory + * to save/restore the registers, as the guest depends on them. + * + * For this, we use a DIRTY bit, indicating the guest has modified the + * debug registers, used as follow: + * + * On guest entry: + * - If the dirty bit is set (because we're coming back from trapping), + * disable the traps, save host registers, restore guest registers. + * - If debug is actively in use (ARM_DSCR_MDBGEN set), + * set the dirty bit, disable the traps, save host registers, + * restore guest registers. + * - Otherwise, enable the traps + * + * On guest exit: + * - If the dirty bit is set, save guest registers, restore host + * registers and clear the dirty bit. This ensure that the host can + * now use the debug registers. + * + * Notice: + * - For ARMv7, if the CONFIG_HAVE_HW_BREAKPOINT is set in the guest, + * debug is always actively in use (ARM_DSCR_MDBGEN set). + * We have to do the save/restore dance in this case, because the + * host and the guest might use their respective debug registers + * at any moment. + */ static bool trap_debug32(struct kvm_vcpu *vcpu, const struct coproc_params *p, const struct coproc_reg *r) { - if (p->is_write) + if (p->is_write) { vcpu->arch.cp14[r->reg] = *vcpu_reg(vcpu, p->Rt1); - else + vcpu->arch.debug_flags |= KVM_ARM_DEBUG_DIRTY; + } else { *vcpu_reg(vcpu, p->Rt1) = vcpu->arch.cp14[r->reg]; + } return true; } diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S index a20b9ad..5662c39 100644 --- a/arch/arm/kvm/interrupts_head.S +++ b/arch/arm/kvm/interrupts_head.S @@ -1,4 +1,6 @@ #include <linux/irqchip/arm-gic.h> +#include <asm/hw_breakpoint.h> +#include <asm/kvm_asm.h> #include <asm/assembler.h> #define VCPU_USR_REG(_reg_nr) (VCPU_USR_REGS + (_reg_nr * 4)) @@ -407,6 +409,46 @@ vcpu .req r0 @ vcpu pointer always in r0 mcr p15, 2, r12, c0, c0, 0 @ CSSELR .endm +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ +.macro skip_debug_state target + ldr r5, [vcpu, #VCPU_DEBUG_FLAGS] + cmp r5, #KVM_ARM_DEBUG_DIRTY + bne \target +1: +.endm + +/* Compute debug state: If ARM_DSCR_MDBGEN or KVM_ARM_DEBUG_DIRTY + * is set, we do a full save/restore cycle and disable trapping. + * + * Assumes vcpu pointer in vcpu reg + * + * Clobbers r5, r6 + */ +.macro compute_debug_state target + // Check the state of MDSCR_EL1 + ldr r5, [vcpu, #CP14_OFFSET(cp14_DBGDSCRext)] + and r6, r5, #ARM_DSCR_MDBGEN + cmp r6, #0 + beq 9998f // Nothing to see there + + // If ARM_DSCR_MDBGEN bit was set, we must set the flag + mov r5, #KVM_ARM_DEBUG_DIRTY + str r5, [vcpu, #VCPU_DEBUG_FLAGS] + b 9999f // Don't skip restore + +9998: + // Otherwise load the flags from memory in case we recently + // trapped + skip_debug_state \target +9999: +.endm + +/* Assume vcpu pointer in vcpu reg, clobbers r5 */ +.macro clear_debug_dirty_bit + mov r5, #0 + str r5, [vcpu, #VCPU_DEBUG_FLAGS] +.endm + /* * Save the VGIC CPU state into memory *
The trapping code keeps track of the state of the debug registers, allowing for the switch code to implement a lazy switching strategy. Signed-off-by: Zhichao Huang <zhichao.huang@linaro.org> --- arch/arm/include/asm/kvm_asm.h | 3 +++ arch/arm/include/asm/kvm_host.h | 3 +++ arch/arm/kernel/asm-offsets.c | 1 + arch/arm/kvm/coproc.c | 39 ++++++++++++++++++++++++++++++++++++-- arch/arm/kvm/interrupts_head.S | 42 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 86 insertions(+), 2 deletions(-)