Re: [PATCH v4 09/10] ARM: KVM: Handle I/O aborts

[Avi Kivity <avi@redhat.com>]

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.

>   /******************************************************************************
> - * 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?

> +
> +/*
> + * 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.

> 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.

> +
> +	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