On Aug 9, 2011, at 1:34 PM, Avi Kivity wrote:
> On 08/06/2011 01:40 PM, 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.
>>
>> 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.
>
> We need to move this to arch independent code. Outside the scope of these patches, of course.
OK, let me know what I can do to make this fit with the ARM implementation nicely.
>
>> /******************************************************************************
>> - * Co-processor emulation
>> + * Utility functions common for all emulation code
>> + *****************************************************************************/
>> +
>> +/*
>> + * This one accepts a matrix where the first element is the
>> + * bits as they must be, and the second element is the bitmask.
>> */
>> +#define INSTR_NONE -1
>> +static int kvm_instr_index(u32 instr, u32 table[][2], int table_entries)
>> +{
>> + int i;
>> + u32 mask;
>> +
>> + for (i = 0; i< table_entries; i++) {
>> + mask = table[i][1];
>> + if ((table[i][0]& mask) == (instr& mask))
>> + return i;
>> + }
>> + return INSTR_NONE;
>> +}
>
> Seems somewhat inefficient to do this for insn emulation. Is there not a common prefix that can be used to determine the mask?
hehe, not so much.
>
>> +
>> +/*
>> + * 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] = {
>> + {0x04700000, 0x0d700000}, /* LDRBT */
>> + {0x04300000, 0x0d700000}, /* LDRT */
>> + {0x04100000, 0x0c500000}, /* LDR */
>> + {0x04500000, 0x0c500000}, /* LDRB */
>> + {0x000000d0, 0x0e1000f0}, /* LDRD */
>> + {0x01900090, 0x0ff000f0}, /* LDREX */
>> + {0x001000b0, 0x0e1000f0}, /* LDRH */
>> + {0x001000d0, 0x0e1000f0}, /* LDRSB */
>> + {0x001000f0, 0x0e1000f0}, /* LDRSH */
>> + {0x04600000, 0x0d700000}, /* STRBT */
>> + {0x04200000, 0x0d700000}, /* STRT */
>> + {0x04000000, 0x0c500000}, /* STR */
>> + {0x04400000, 0x0c500000}, /* STRB */
>> + {0x000000f0, 0x0e1000f0}, /* STRD */
>> + {0x01800090, 0x0ff000f0}, /* STREX */
>> + {0x000000b0, 0x0e1000f0} /* STRH */
>> +};
>> +
>
> Okay, maybe not. But surely there's some clever arithmetic the cpu uses to decode this.
Probably, but this is only used in the rare case when the virt. extensions doesn't support the fault information. I highly doubt that this is in any critical path for any sane guest OS, but surely one could write a VM that would run very slow. I would like not to spend time on this right now and perhaps get back to it when we have all sorts of other features in place. Or, what do you think?
>
>> diff --git a/arch/arm/kvm/trace.h b/arch/arm/kvm/trace.h
>> index 381ea4a..4f20d75 100644
>> --- a/arch/arm/kvm/trace.h
>> +++ b/arch/arm/kvm/trace.h
>> @@ -39,6 +39,21 @@ TRACE_EVENT(kvm_exit,
>> TP_printk("PC: 0x%08lx", __entry->vcpu_pc)
>> );
>>
>> +TRACE_EVENT(kvm_mmio_emulate,
>> + TP_PROTO(unsigned long vcpu_pc),
>> + TP_ARGS(vcpu_pc),
>
> Please add the instruction bytes and any other information needed to decode the opcode (e.g. thumb mode). Forx86 we have a trace-cmd plugin that disassembles guest instructions into the trace; it's very useful.
that's a good idea. I will look into it.
>
>> +
>> + TP_STRUCT__entry(
>> + __field( unsigned long, vcpu_pc )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->vcpu_pc = vcpu_pc;
>> + ),
>> +
>> + TP_printk("Emulate MMIO at: 0x%08lx", __entry->vcpu_pc)
>> +);
>> +
>> TRACE_EVENT(kvm_emulate_cp15_imp,
>> TP_PROTO(unsigned long Op1, unsigned long Rt1, unsigned long CRn,
>> unsigned long CRm, unsigned long Op2, bool is_write),
>>
>>
>
> --
> error compiling committee.c: too many arguments to function
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
