Re: [RFC PATCH 1/1] target/arm: kvm: Handle DABT with no valid ISS

[Peter Maydell]

On Fri, 20 Dec 2019 at 20:27, Beata Michalska
<address@hidden> wrote:
>
> On ARMv7 & ARMv8 some load/store instructions might trigger a data abort
> exception with no valid ISS info to be decoded. The lack of decode info
> makes it at least tricky to emulate those instruction which is one of the
> (many) reasons why KVM will not even try to do so.
>
> Add suport for handling those by requesting KVM to inject external
> dabt into the quest.
>
> Signed-off-by: Beata Michalska <address@hidden>
> ---
>  accel/kvm/kvm-all.c    | 15 +++++++
>  accel/stubs/kvm-stub.c |  4 ++
>  include/sysemu/kvm.h   |  1 +
>  target/arm/cpu.h       |  3 +-
>  target/arm/kvm.c       | 95 ++++++++++++++++++++++++++++++++++++++++++
>  target/arm/kvm32.c     |  3 ++
>  target/arm/kvm64.c     |  3 ++
>  target/arm/kvm_arm.h   | 19 +++++++++
>  8 files changed, 142 insertions(+), 1 deletion(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index ca00daa2f5..a3ee038142 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2174,6 +2174,14 @@ static void do_kvm_cpu_synchronize_state(CPUState 
> *cpu, run_on_cpu_data arg)
>      }
>  }
>
> +static void do_kvm_cpu_synchronize_state_force(CPUState *cpu,
> +                                               run_on_cpu_data arg)
> +{
> +    kvm_arch_get_registers(cpu);
> +    cpu->vcpu_dirty = true;
> +}

Why is this functionality special such that it needs a non-standard
way of synchronizing state with KVM that nothing else does ?

> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 9fe233b9bf..0cacc61d8a 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -483,6 +483,7 @@ void kvm_cpu_synchronize_state(CPUState *cpu);
>  void kvm_cpu_synchronize_post_reset(CPUState *cpu);
>  void kvm_cpu_synchronize_post_init(CPUState *cpu);
>  void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu);
> +void kvm_cpu_synchronize_state_force(CPUState *cpu);
>
>  void kvm_init_cpu_signals(CPUState *cpu);
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5f70e9e043..e11b5e7438 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -558,7 +558,8 @@ typedef struct CPUARMState {
>          uint8_t has_esr;
>          uint64_t esr;
>      } serror;
> -
> +    /* Status field for pending extarnal dabt */

"external" (I think you have the same typo later in the patch too)

> +    uint8_t ext_dabt_pending;
>      /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>      uint32_t irq_line_state;


> @@ -701,6 +719,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
> *run)
>              ret = EXCP_DEBUG;
>          } /* otherwise return to guest */
>          break;
> +    case KVM_EXIT_ARM_NISV:
> +        /* External DAB with no valid iss to decode */

"DABT"

> +        ret = kvm_arm_handle_dabt_nisv(cs, run->arm_nisv.esr_iss,
> +                                 run->arm_nisv.fault_ipa);

(indentation looks odd here?)

> +        break;
>      default:
>          qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
>                        __func__, run->exit_reason);
> @@ -835,3 +858,75 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return (data - 32) & 0xffff;
>  }
> +
> +int kvm_arm_handle_dabt_nisv(CPUState *cs, uint64_t esr_iss,
> +                              uint64_t fault_ipa)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    hwaddr xlat, len;
> +    AddressSpace *as = cs->as ? cs->as : &address_space_memory;

I don't think you should need to test cs->as for NULL;
qemu_init_vcpu() ensures that one is always set up.

Probably the neatest way to get the AddressSpace* is to
call arm_addressspace(cs, MEMTXATTRS_UNSPECIFIED).


> +    int store_ins = ((esr_iss >> 6) & 1); /* WnR bit */

This should be a bool (matching the argument type for
address_space_translate), so you can just do
   bool is_write = esr_iss & (1 << 6);

> +    int ret;
> +
> +    /*
> +     * Note: The ioctl for SET_EVENTS will be triggered before next
> +     * KVM_RUN call though the vcpu regs need to be updated before hand
> +     * so to not to overwrite changes done by KVM upon injecting the abort.
> +     * This sadly requires running sync twice - shame ...
> +     */
> +    kvm_cpu_synchronize_state(cs);
> +   /*
> +    * ISS [23:14] is invalid so there is a limited info
> +    * on what has just happened so the only *useful* thing that can
> +    * be retrieved from ISS is WnR & DFSC (though in some cases WnR
> +    * might be less of a value as well)
> +    */
> +    /* Verify whether the memory being accessed is even recognisable */
> +    if (!address_space_translate(as, fault_ipa, &xlat, &len,
> +                                 store_ins, MEMTXATTRS_UNSPECIFIED)) {
> +        error_report("An attemp to access memory outside of the boundries"

"attempt"; "boundaries". But I think what we should actually report here
is:

"Guest attempted to access memory/device at guest physical address
0x... using an instruction that KVM could not emulate (no valid ISS).
Injecting a data abort exception into guest."

It doesn't seem to me worth distinguishing whether the guest
physical address happens to have anything mapped there or not;
we're going to inject a somewhat bogus dabt anyway.

After the initial error_report(), supplemental information like
the offending instruction should be printed with error_printf(),
because it isn't a separate new error.

> +                     "at 0x" TARGET_FMT_lx, (target_ulong) fault_ipa);
> +    } else {
> +        uint32_t ins;
> +        uint8_t ins_fetched;
> +
> +        /*
> +         * Get current PC before it will get updated to except vector entry

"exception"

> +         */
> +        target_ulong ins_addr = is_a64(env) ? env->pc
> +                                /* AArch32 mode vs T32 aka Thumb mode */
> +                                : env->regs[15] - (env->thumb ? 4 : 8);
> +
> +        /*
> +         * Get the faulting instruction
> +         * Instructions have a fixed length of 32 bits
> +         * and are always little-endian

...unless they're 16-bit Thumb. In an ideal world you should handle
the Thumb case by doing a 16-bit load, and then loading another 16-bit
value if the top 3 bits of the first part are 0b111.

At least we don't have to consider the possibility ofold-style
really-big-endian-instruction-ordering :-)

> +         */
> +        ins_fetched = !cpu_memory_rw_debug(cs, ins_addr, (uint8_t *) &ins,
> +                                           sizeof(uint32_t), store_ins) ;

stray space before semicolon

> +
> +        error_report("Data abort exception with no valid ISS generated by "
> +                     "memory access at 0x" TARGET_FMT_lx,
> +                     (target_ulong)fault_ipa);
> +        error_report(ins_fetched ? "%s: 0x%" PRIx32 " (encoded)" : "%s",
> +                     "Unable to emulate memory instruction",
> +                     (!ins_fetched ? 0 : ldl_le_p(&ins)));
> +
> +        error_report("Faulting instruction at 0x" TARGET_FMT_lx, ins_addr);
> +    }
> +    /*
> +     * Set pending ext dabt amd trigger SET_EVENTS so that
> +     * KVM can inject the abort
> +     */
> +    env->ext_dabt_pending = 1;
> +
> +    ret = kvm_put_vcpu_events(cpu);
> +
> +    /* Get the most up-tp-date state */

"to"

> +    if (!ret) {
> +        /* Hacky but the sync needs to be forced */
> +        kvm_cpu_synchronize_state_force(cs);
> +    }
> +    return ret;
> +}
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 32bf8d6757..5539c3a3f2 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -240,6 +240,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      /* Check whether userspace can specify guest syndrome value */
>      kvm_arm_init_serror_injection(cs);
>
> +    /* Set status for supporting the extarnal dabt injection */
> +    kvm_arm_init_ext_dabt_injection(cs);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 876184b8fe..3da4e2e70e 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -792,6 +792,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      /* Check whether user space can specify guest syndrome value */
>      kvm_arm_init_serror_injection(cs);
>
> +    /* Set status for supporting the extarnal dabt injection */
> +    kvm_arm_init_ext_dabt_injection(cs);
> +
>      return kvm_arm_init_cpreg_list(cpu);
>  }

thanks
-- PMM