[PATCH 14/15] KVM: ARM: Handle I/O aborts

2012-09-15 Thread Christoffer Dall 
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

2012-09-27 Thread Will Deacon 
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

2012-09-30 Thread Christoffer Dall 
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

2012-10-01 Thread Dave Martin 
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

2012-10-01 Thread Dave Martin 
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

2012-10-01 Thread Jon Medhurst (Tixy) 
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

2012-10-05 Thread Russell King - ARM Linux 
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

2012-10-08 Thread Dave Martin 
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

2012-10-08 Thread Christoffer Dall 
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