[PATCH 14/15] KVM: ARM: Handle I/O aborts
When the guest accesses I/O memory this will create data abort exceptions and they are handled by decoding the HSR information (physical address, read/write, length, register) and forwarding reads and writes to QEMU which performs the device emulation. Certain classes of load/store operations do not support the syndrome information provided in the HSR and we therefore must be able to fetch the offending instruction from guest memory and decode it manually. We only support instruction decoding for valid reasonable MMIO operations where trapping them do not provide sufficient information in the HSR (no 16-bit Thumb instructions provide register writeback that we care about). The following instruciton types are NOT supported for MMIO operations despite the HSR not containing decode info: - any Load/Store multiple - any load/store exclusive - any load/store dual - anything with the PC as the dest register This requires changing the general flow somewhat since new calls to run the VCPU must check if there's a pending MMIO load and perform the write after userspace has made the data available. Rusty Russell fixed a horrible race pointed out by Ben Herrenschmidt: (1) Guest complicated mmio instruction traps. (2) The hardware doesn't tell us enough, so we need to read the actual instruction which was being exectuted. (3) KVM maps the instruction virtual address to a physical address. (4) The guest (SMP) swaps out that page, and fills it with something else. (5) We read the physical address, but now that's the wrong thing. Signed-off-by: Rusty Russell Signed-off-by: Christoffer Dall --- arch/arm/include/asm/kvm_arm.h |3 arch/arm/include/asm/kvm_asm.h |2 arch/arm/include/asm/kvm_emulate.h | 23 ++ arch/arm/include/asm/kvm_host.h|3 arch/arm/include/asm/kvm_mmu.h |1 arch/arm/kvm/arm.c | 14 + arch/arm/kvm/emulate.c | 448 arch/arm/kvm/exports.c |1 arch/arm/kvm/interrupts.S | 40 +++ arch/arm/kvm/mmu.c | 266 + arch/arm/kvm/trace.h | 21 ++ 11 files changed, 819 insertions(+), 3 deletions(-) diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h index 4cff3b7..21cb240 100644 --- a/arch/arm/include/asm/kvm_arm.h +++ b/arch/arm/include/asm/kvm_arm.h @@ -158,8 +158,11 @@ #define HSR_ISS(HSR_IL - 1) #define HSR_ISV_SHIFT (24) #define HSR_ISV(1U << HSR_ISV_SHIFT) +#define HSR_SRT_SHIFT (16) +#define HSR_SRT_MASK (0xf << HSR_SRT_SHIFT) #define HSR_FSC(0x3f) #define HSR_FSC_TYPE (0x3c) +#define HSR_SSE(1 << 21) #define HSR_WNR(1 << 6) #define HSR_CV_SHIFT (24) #define HSR_CV (1U << HSR_CV_SHIFT) diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h index 40ee099..5315c69 100644 --- a/arch/arm/include/asm/kvm_asm.h +++ b/arch/arm/include/asm/kvm_asm.h @@ -49,6 +49,8 @@ extern void __kvm_flush_vm_context(void); extern void __kvm_tlb_flush_vmid(struct kvm *kvm); extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu); + +extern u64 __kvm_va_to_pa(struct kvm_vcpu *vcpu, u32 va, bool priv); #endif #endif /* __ARM_KVM_ASM_H__ */ diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h index 6ddfae2..68489f2 100644 --- a/arch/arm/include/asm/kvm_emulate.h +++ b/arch/arm/include/asm/kvm_emulate.h @@ -22,6 +22,27 @@ #include #include +/* + * The in-kernel MMIO emulation code wants to use a copy of run->mmio, + * which is an anonymous type. Use our own type instead. + */ +struct kvm_exit_mmio { + phys_addr_t phys_addr; + u8 data[8]; + u32 len; + boolis_write; +}; + +static inline void kvm_prepare_mmio(struct kvm_run *run, + struct kvm_exit_mmio *mmio) +{ + run->mmio.phys_addr = mmio->phys_addr; + run->mmio.len = mmio->len; + run->mmio.is_write = mmio->is_write; + memcpy(run->mmio.data, mmio->data, mmio->len); + run->exit_reason= KVM_EXIT_MMIO; +} + u32 *vcpu_reg_mode(struct kvm_vcpu *vcpu, u8 reg_num, enum vcpu_mode mode); static inline u8 __vcpu_mode(u32 cpsr) @@ -52,6 +73,8 @@ static inline enum vcpu_mode vcpu_mode(struct kvm_vcpu *vcpu) } int kvm_handle_wfi(struct kvm_vcpu *vcpu, struct kvm_run *run); +int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, + unsigned long instr, struct kvm_exit_mmio *mmio); void kvm_skip_instr(struct kvm_vcpu *vcpu, bool is_wide_instr); void kvm_inject_undefined(struct kvm_vcpu *vcpu); void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 97608d7..3fec9ad 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/inc
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Sat, Sep 15, 2012 at 04:35:59PM +0100, Christoffer Dall wrote: > When the guest accesses I/O memory this will create data abort > exceptions and they are handled by decoding the HSR information > (physical address, read/write, length, register) and forwarding reads > and writes to QEMU which performs the device emulation. > > Certain classes of load/store operations do not support the syndrome > information provided in the HSR and we therefore must be able to fetch > the offending instruction from guest memory and decode it manually. > > We only support instruction decoding for valid reasonable MMIO operations > where trapping them do not provide sufficient information in the HSR (no > 16-bit Thumb instructions provide register writeback that we care about). > > The following instruciton types are NOT supported for MMIO operations > despite the HSR not containing decode info: > - any Load/Store multiple > - any load/store exclusive > - any load/store dual > - anything with the PC as the dest register [...] > + > +/** > + * Load-Store instruction emulation > + > */ > + > +/* > + * Must be ordered with LOADS first and WRITES afterwards > + * for easy distinction when doing MMIO. > + */ > +#define NUM_LD_INSTR 9 > +enum INSTR_LS_INDEXES { > + INSTR_LS_LDRBT, INSTR_LS_LDRT, INSTR_LS_LDR, INSTR_LS_LDRB, > + INSTR_LS_LDRD, INSTR_LS_LDREX, INSTR_LS_LDRH, INSTR_LS_LDRSB, > + INSTR_LS_LDRSH, > + INSTR_LS_STRBT, INSTR_LS_STRT, INSTR_LS_STR, INSTR_LS_STRB, > + INSTR_LS_STRD, INSTR_LS_STREX, INSTR_LS_STRH, > + NUM_LS_INSTR > +}; > + > +static u32 ls_instr[NUM_LS_INSTR][2] = { > + {0x0470, 0x0d70}, /* LDRBT */ > + {0x0430, 0x0d70}, /* LDRT */ > + {0x0410, 0x0c50}, /* LDR */ > + {0x0450, 0x0c50}, /* LDRB */ > + {0x00d0, 0x0e1000f0}, /* LDRD */ > + {0x01900090, 0x0ff000f0}, /* LDREX */ > + {0x001000b0, 0x0e1000f0}, /* LDRH */ > + {0x001000d0, 0x0e1000f0}, /* LDRSB */ > + {0x001000f0, 0x0e1000f0}, /* LDRSH */ > + {0x0460, 0x0d70}, /* STRBT */ > + {0x0420, 0x0d70}, /* STRT */ > + {0x0400, 0x0c50}, /* STR */ > + {0x0440, 0x0c50}, /* STRB */ > + {0x00f0, 0x0e1000f0}, /* STRD */ > + {0x01800090, 0x0ff000f0}, /* STREX */ > + {0x00b0, 0x0e1000f0} /* STRH */ > +}; > + > +static inline int get_arm_ls_instr_index(u32 instr) > +{ > + return kvm_instr_index(instr, ls_instr, NUM_LS_INSTR); > +} > + > +/* > + * Load-Store instruction decoding > + */ > +#define INSTR_LS_TYPE_BIT 26 > +#define INSTR_LS_RD_MASK 0xf000 > +#define INSTR_LS_RD_SHIFT 12 > +#define INSTR_LS_RN_MASK 0x000f > +#define INSTR_LS_RN_SHIFT 16 > +#define INSTR_LS_RM_MASK 0x000f > +#define INSTR_LS_OFFSET12_MASK 0x0fff I'm afraid you're not going to thank me much for this, but it's high time we unified the various instruction decoding functions we have under arch/arm/ and this seems like a good opportunity for that. For example, look at the following snippets (there is much more in the files I list) in addition to what you have: asm/ptrace.h - #define PSR_T_BIT 0x0020 #define PSR_F_BIT 0x0040 #define PSR_I_BIT 0x0080 #define PSR_A_BIT 0x0100 #define PSR_E_BIT 0x0200 #define PSR_J_BIT 0x0100 #define PSR_Q_BIT 0x0800 #define PSR_V_BIT 0x1000 #define PSR_C_BIT 0x2000 #define PSR_Z_BIT 0x4000 #define PSR_N_BIT 0x8000 mm/alignment.c -- #define LDST_I_BIT(i) (i & (1 << 26)) /* Immediate constant */ #define LDST_P_BIT(i) (i & (1 << 24)) /* Preindex */ #define LDST_U_BIT(i) (i & (1 << 23)) /* Add offset */ #define LDST_W_BIT(i) (i & (1 << 21)) /* Writeback*/ #define LDST_L_BIT(i) (i & (1 << 20)) /* Load */ kernel/kprobes*.c - static void __kprobes emulate_ldr(struct kprobe *p, struct pt_regs *regs) { kprobe_opcode_t insn = p->opcode; unsigned long pc = (unsigned long)p->addr + 8; int rt = (insn >> 12) & 0xf; int rn = (insn >> 16) & 0xf; int rm = insn & 0xf; kernel/opcodes.c static const unsigned short cc_map[16] = { 0xF0F0, /* EQ == Z set*/ 0x0F0F, /* NE */ 0x, /* CS == C set*/ 0x, /* CC */ 0xFF00, /* MI == N set*/ 0x00FF, /* PL */
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Thu, Sep 27, 2012 at 11:11 AM, Will Deacon wrote: > On Sat, Sep 15, 2012 at 04:35:59PM +0100, Christoffer Dall wrote: >> When the guest accesses I/O memory this will create data abort >> exceptions and they are handled by decoding the HSR information >> (physical address, read/write, length, register) and forwarding reads >> and writes to QEMU which performs the device emulation. >> >> Certain classes of load/store operations do not support the syndrome >> information provided in the HSR and we therefore must be able to fetch >> the offending instruction from guest memory and decode it manually. >> >> We only support instruction decoding for valid reasonable MMIO operations >> where trapping them do not provide sufficient information in the HSR (no >> 16-bit Thumb instructions provide register writeback that we care about). >> >> The following instruciton types are NOT supported for MMIO operations >> despite the HSR not containing decode info: >> - any Load/Store multiple >> - any load/store exclusive >> - any load/store dual >> - anything with the PC as the dest register > > [...] > >> + >> +/** >> + * Load-Store instruction emulation >> + >> */ >> + >> +/* >> + * Must be ordered with LOADS first and WRITES afterwards >> + * for easy distinction when doing MMIO. >> + */ >> +#define NUM_LD_INSTR 9 >> +enum INSTR_LS_INDEXES { >> + INSTR_LS_LDRBT, INSTR_LS_LDRT, INSTR_LS_LDR, INSTR_LS_LDRB, >> + INSTR_LS_LDRD, INSTR_LS_LDREX, INSTR_LS_LDRH, INSTR_LS_LDRSB, >> + INSTR_LS_LDRSH, >> + INSTR_LS_STRBT, INSTR_LS_STRT, INSTR_LS_STR, INSTR_LS_STRB, >> + INSTR_LS_STRD, INSTR_LS_STREX, INSTR_LS_STRH, >> + NUM_LS_INSTR >> +}; >> + >> +static u32 ls_instr[NUM_LS_INSTR][2] = { >> + {0x0470, 0x0d70}, /* LDRBT */ >> + {0x0430, 0x0d70}, /* LDRT */ >> + {0x0410, 0x0c50}, /* LDR */ >> + {0x0450, 0x0c50}, /* LDRB */ >> + {0x00d0, 0x0e1000f0}, /* LDRD */ >> + {0x01900090, 0x0ff000f0}, /* LDREX */ >> + {0x001000b0, 0x0e1000f0}, /* LDRH */ >> + {0x001000d0, 0x0e1000f0}, /* LDRSB */ >> + {0x001000f0, 0x0e1000f0}, /* LDRSH */ >> + {0x0460, 0x0d70}, /* STRBT */ >> + {0x0420, 0x0d70}, /* STRT */ >> + {0x0400, 0x0c50}, /* STR */ >> + {0x0440, 0x0c50}, /* STRB */ >> + {0x00f0, 0x0e1000f0}, /* STRD */ >> + {0x01800090, 0x0ff000f0}, /* STREX */ >> + {0x00b0, 0x0e1000f0} /* STRH */ >> +}; >> + >> +static inline int get_arm_ls_instr_index(u32 instr) >> +{ >> + return kvm_instr_index(instr, ls_instr, NUM_LS_INSTR); >> +} >> + >> +/* >> + * Load-Store instruction decoding >> + */ >> +#define INSTR_LS_TYPE_BIT 26 >> +#define INSTR_LS_RD_MASK 0xf000 >> +#define INSTR_LS_RD_SHIFT 12 >> +#define INSTR_LS_RN_MASK 0x000f >> +#define INSTR_LS_RN_SHIFT 16 >> +#define INSTR_LS_RM_MASK 0x000f >> +#define INSTR_LS_OFFSET12_MASK 0x0fff > > I'm afraid you're not going to thank me much for this, but it's high time we > unified the various instruction decoding functions we have under arch/arm/ > and this seems like a good opportunity for that. For example, look at the > following snippets (there is much more in the files I list) in addition to > what you have: > I think it would be great if we had a set of unified decoding functions! However, I think it's a shame if we can't merge KVM because we want to clean up this code. There would be nothing in the way of breaking API or anything like that preventing us from cleaning this up after the code has been merged. Please consider reviewing the code for correctness and consider if the code could be merged as is. On the other hand, if you or Dave or anyone else wants to create a set of patches that cleans this up in a timely manner, I will be happy to merge them for my code as well ;) > > asm/ptrace.h > - > #define PSR_T_BIT 0x0020 > #define PSR_F_BIT 0x0040 > #define PSR_I_BIT 0x0080 > #define PSR_A_BIT 0x0100 > #define PSR_E_BIT 0x0200 > #define PSR_J_BIT 0x0100 > #define PSR_Q_BIT 0x0800 > #define PSR_V_BIT 0x1000 > #define PSR_C_BIT 0x2000 > #define PSR_Z_BIT 0x4000 > #define PSR_N_BIT 0x8000 > > mm/alignment.c > -- > #define LDST_I_BIT(i) (i & (1 << 26)) /* Immediate constant */ > #define LDST_P_BIT(i) (i & (1 << 24)) /* Preindex */ > #define LDST_U_BIT(i) (i & (1 << 23)) /* Add offset */ > #define LDST_W_BIT(i) (i & (1 << 21)) /* Writeback*/ > #define LDST_L_BIT(i) (i & (1 << 20)) /* Load
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Sun, Sep 30, 2012 at 05:49:21PM -0400, Christoffer Dall wrote: > On Thu, Sep 27, 2012 at 11:11 AM, Will Deacon wrote: > > On Sat, Sep 15, 2012 at 04:35:59PM +0100, Christoffer Dall wrote: > >> When the guest accesses I/O memory this will create data abort > >> exceptions and they are handled by decoding the HSR information > >> (physical address, read/write, length, register) and forwarding reads > >> and writes to QEMU which performs the device emulation. > >> > >> Certain classes of load/store operations do not support the syndrome > >> information provided in the HSR and we therefore must be able to fetch > >> the offending instruction from guest memory and decode it manually. > >> > >> We only support instruction decoding for valid reasonable MMIO operations > >> where trapping them do not provide sufficient information in the HSR (no > >> 16-bit Thumb instructions provide register writeback that we care about). > >> > >> The following instruciton types are NOT supported for MMIO operations > >> despite the HSR not containing decode info: > >> - any Load/Store multiple > >> - any load/store exclusive > >> - any load/store dual > >> - anything with the PC as the dest register > > > > [...] > > > >> + > >> +/** > >> + * Load-Store instruction emulation > >> + > >> */ > >> + > >> +/* > >> + * Must be ordered with LOADS first and WRITES afterwards > >> + * for easy distinction when doing MMIO. > >> + */ > >> +#define NUM_LD_INSTR 9 > >> +enum INSTR_LS_INDEXES { > >> + INSTR_LS_LDRBT, INSTR_LS_LDRT, INSTR_LS_LDR, INSTR_LS_LDRB, > >> + INSTR_LS_LDRD, INSTR_LS_LDREX, INSTR_LS_LDRH, INSTR_LS_LDRSB, > >> + INSTR_LS_LDRSH, > >> + INSTR_LS_STRBT, INSTR_LS_STRT, INSTR_LS_STR, INSTR_LS_STRB, > >> + INSTR_LS_STRD, INSTR_LS_STREX, INSTR_LS_STRH, > >> + NUM_LS_INSTR > >> +}; > >> + > >> +static u32 ls_instr[NUM_LS_INSTR][2] = { > >> + {0x0470, 0x0d70}, /* LDRBT */ > >> + {0x0430, 0x0d70}, /* LDRT */ > >> + {0x0410, 0x0c50}, /* LDR */ > >> + {0x0450, 0x0c50}, /* LDRB */ > >> + {0x00d0, 0x0e1000f0}, /* LDRD */ > >> + {0x01900090, 0x0ff000f0}, /* LDREX */ > >> + {0x001000b0, 0x0e1000f0}, /* LDRH */ > >> + {0x001000d0, 0x0e1000f0}, /* LDRSB */ > >> + {0x001000f0, 0x0e1000f0}, /* LDRSH */ > >> + {0x0460, 0x0d70}, /* STRBT */ > >> + {0x0420, 0x0d70}, /* STRT */ > >> + {0x0400, 0x0c50}, /* STR */ > >> + {0x0440, 0x0c50}, /* STRB */ > >> + {0x00f0, 0x0e1000f0}, /* STRD */ > >> + {0x01800090, 0x0ff000f0}, /* STREX */ > >> + {0x00b0, 0x0e1000f0} /* STRH */ > >> +}; > >> + > >> +static inline int get_arm_ls_instr_index(u32 instr) > >> +{ > >> + return kvm_instr_index(instr, ls_instr, NUM_LS_INSTR); > >> +} > >> + > >> +/* > >> + * Load-Store instruction decoding > >> + */ > >> +#define INSTR_LS_TYPE_BIT 26 > >> +#define INSTR_LS_RD_MASK 0xf000 > >> +#define INSTR_LS_RD_SHIFT 12 > >> +#define INSTR_LS_RN_MASK 0x000f > >> +#define INSTR_LS_RN_SHIFT 16 > >> +#define INSTR_LS_RM_MASK 0x000f > >> +#define INSTR_LS_OFFSET12_MASK 0x0fff > > > > I'm afraid you're not going to thank me much for this, but it's high time we > > unified the various instruction decoding functions we have under arch/arm/ > > and this seems like a good opportunity for that. For example, look at the > > following snippets (there is much more in the files I list) in addition to > > what you have: > > > > I think it would be great if we had a set of unified decoding functions! > > However, I think it's a shame if we can't merge KVM because we want to > clean up this code. There would be nothing in the way of breaking API > or anything like that preventing us from cleaning this up after the > code has been merged. > > Please consider reviewing the code for correctness and consider if the > code could be merged as is. > > On the other hand, if you or Dave or anyone else wants to create a set > of patches that cleans this up in a timely manner, I will be happy to > merge them for my code as well ;) The time I would have available to put into this is rather limited, but I have some initial ideas, as outlined below. Tixy (who did the kprobes implementation, which is probably the most sophisticated opcode handling we have in the kernel right now) may have ideas too. I would hope that any common framework could reuse a fair chunk of his implementation and test coverage. I think that a common framework would have to start out a lot more generic that the special-purpose code in current subsystems, at least initially. We should try to move all the knowledge about how instructio
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Mon, Oct 01, 2012 at 04:12:09PM +0100, Jon Medhurst (Tixy) wrote: > On Mon, 2012-10-01 at 13:53 +0100, Dave Martin wrote: > > On Sun, Sep 30, 2012 at 05:49:21PM -0400, Christoffer Dall wrote: > > > On Thu, Sep 27, 2012 at 11:11 AM, Will Deacon wrote: > > > > On Sat, Sep 15, 2012 at 04:35:59PM +0100, Christoffer Dall wrote: > > > > I'm afraid you're not going to thank me much for this, but it's high > > > > time we > > > > unified the various instruction decoding functions we have under > > > > arch/arm/ > > > > and this seems like a good opportunity for that. For example, look at > > > > the > > > > following snippets (there is much more in the files I list) in addition > > > > to > > > > what you have: > > > > > > > > > > I think it would be great if we had a set of unified decoding functions! > > > > > > However, I think it's a shame if we can't merge KVM because we want to > > > clean up this code. There would be nothing in the way of breaking API > > > or anything like that preventing us from cleaning this up after the > > > code has been merged. > > > > > > Please consider reviewing the code for correctness and consider if the > > > code could be merged as is. > > > > > > On the other hand, if you or Dave or anyone else wants to create a set > > > of patches that cleans this up in a timely manner, I will be happy to > > > merge them for my code as well ;) > > > > The time I would have available to put into this is rather limited, but > > I have some initial ideas, as outlined below. > > > > Tixy (who did the kprobes implementation, which is probably the most > > sophisticated opcode handling we have in the kernel right now) may have > > ideas too. I would hope that any common framework could reuse a fair > > chunk of his implementation and test coverage. > > To my thinking, the kprobes code is very tailored to the job it needs to > do and that turning it into something generic is just going to make > everything bigger and more complex - because a generic framework would > be bigger (as it's trying to be generic) and then things like kprobes > will probably end up having an additional framework layered over the top > to bend it to it's purposes. Perhaps I'm being too pessimistic. Perhaps kprobes is a bit of a double-edged example. It's an example of an implementation with some good features, but because it is larger the amount of adaptation required to convert to a common framework would necessarily be larger also. Yet, kprobes isn't trying to solve radically different problems from other subsystems in the kernel. It doesn't just want to descode and manipulate the properties of instructions, it is actually interested in many of the same properties (for example, whether an instruction is a load or store, whether it modifies the PC etc.) as some other subsystems. I worry that by default every implementation of this ends up rather deeply tailored to its correcponding subsystem -- so we gradually accumulate more incompatible partially-overlapping duplicates of this functionality over time. This doesn't feel like a good thing. > It would also requiring an inordinate amount of time to thrash out > requirements, design, prototype, and to implement. (I don't think I'm > being overly pessimistic about that ;-) > > So, unless some-one has serious quantities of spare time lying around... Well, I don't suggest that we should expect to get there in one go: such an effort won't ever the off the ground for sure. If we can consolidate a few simpler subsystems' opcode handling then that would still be a step in the right direction, even if integrating kprobes could not happen until much later. If we do nothing, the situation will just gradually get worse. Cheers ---Dave -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Mon, 2012-10-01 at 13:53 +0100, Dave Martin wrote: > On Sun, Sep 30, 2012 at 05:49:21PM -0400, Christoffer Dall wrote: > > On Thu, Sep 27, 2012 at 11:11 AM, Will Deacon wrote: > > > On Sat, Sep 15, 2012 at 04:35:59PM +0100, Christoffer Dall wrote: > > > I'm afraid you're not going to thank me much for this, but it's high time > > > we > > > unified the various instruction decoding functions we have under arch/arm/ > > > and this seems like a good opportunity for that. For example, look at the > > > following snippets (there is much more in the files I list) in addition to > > > what you have: > > > > > > > I think it would be great if we had a set of unified decoding functions! > > > > However, I think it's a shame if we can't merge KVM because we want to > > clean up this code. There would be nothing in the way of breaking API > > or anything like that preventing us from cleaning this up after the > > code has been merged. > > > > Please consider reviewing the code for correctness and consider if the > > code could be merged as is. > > > > On the other hand, if you or Dave or anyone else wants to create a set > > of patches that cleans this up in a timely manner, I will be happy to > > merge them for my code as well ;) > > The time I would have available to put into this is rather limited, but > I have some initial ideas, as outlined below. > > Tixy (who did the kprobes implementation, which is probably the most > sophisticated opcode handling we have in the kernel right now) may have > ideas too. I would hope that any common framework could reuse a fair > chunk of his implementation and test coverage. To my thinking, the kprobes code is very tailored to the job it needs to do and that turning it into something generic is just going to make everything bigger and more complex - because a generic framework would be bigger (as it's trying to be generic) and then things like kprobes will probably end up having an additional framework layered over the top to bend it to it's purposes. Perhaps I'm being too pessimistic. It would also requiring an inordinate amount of time to thrash out requirements, design, prototype, and to implement. (I don't think I'm being overly pessimistic about that ;-) So, unless some-one has serious quantities of spare time lying around... -- Tixy -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Mon, Oct 01, 2012 at 01:53:26PM +0100, Dave Martin wrote: > A good starting point would be load/store emulation as this seems to be a > common theme, and we would need a credible deployment for any new > framework so that we know it's fit for purpose. Probably not actually, that code is written to be fast, because things like IP stack throughput depend on it - particularly when your network card can only DMA packets to 32-bit aligned addresses (resulting in virtually all network data being misaligned.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Fri, Oct 05, 2012 at 10:00:25AM +0100, Russell King - ARM Linux wrote: > On Mon, Oct 01, 2012 at 01:53:26PM +0100, Dave Martin wrote: > > A good starting point would be load/store emulation as this seems to be a > > common theme, and we would need a credible deployment for any new > > framework so that we know it's fit for purpose. > > Probably not actually, that code is written to be fast, because things > like IP stack throughput depend on it - particularly when your network > card can only DMA packets to 32-bit aligned addresses (resulting in > virtually all network data being misaligned.) A fair point, but surely it would still be worth a try? We might decide that a few particular cases of instruction decode should not use the generic framework for performance reaons, but in most cases being critically dependent on fault-driven software emulation for performance would be a serious mistake in the first place (discussions about the network code notwithstanding). This is not an argument for being slower just for the sake of it, but it can make sense to factor code on paths where performance is not an issue. Cheers ---Dave -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Mon, Oct 8, 2012 at 6:04 AM, Dave Martin wrote: > On Fri, Oct 05, 2012 at 10:00:25AM +0100, Russell King - ARM Linux wrote: >> On Mon, Oct 01, 2012 at 01:53:26PM +0100, Dave Martin wrote: >> > A good starting point would be load/store emulation as this seems to be a >> > common theme, and we would need a credible deployment for any new >> > framework so that we know it's fit for purpose. >> >> Probably not actually, that code is written to be fast, because things >> like IP stack throughput depend on it - particularly when your network >> card can only DMA packets to 32-bit aligned addresses (resulting in >> virtually all network data being misaligned.) > > A fair point, but surely it would still be worth a try? > > We might decide that a few particular cases of instruction decode > should not use the generic framework for performance reaons, but in > most cases being critically dependent on fault-driven software > emulation for performance would be a serious mistake in the first place > (discussions about the network code notwithstanding). > > This is not an argument for being slower just for the sake of it, but > it can make sense to factor code on paths where performance is not an > issue. > I'm all for unifying this stuff, but I still think it doesn't qualify for holding back on merging KVM patches. The ARM mode instruction decoding can definitely be cleaned up though to look more like the Thumb2 mode decoding which will be a good step before refactoring to use a more common framework. Currently we decode too many types of instructions (not just the ones with cleared HSR.IV) in ARM mode, so the whole complexity of that code can be reduced. I'll give that a go before re-sending the KVM patch series. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html