Re: [Qemu-devel] [PATCH 17/20] target/arm: Implement SG instruction

[Richard Henderson]

On 09/22/2017 11:00 AM, Peter Maydell wrote:
> Implement the SG instruction, which we emulate 'by hand' in the
> exception handling code path.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/arm/helper.c | 129 
> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 124 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index b1ecb66..8df819d 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -41,6 +41,10 @@ typedef struct V8M_SAttributes {
>      bool irvalid;
>  } V8M_SAttributes;
>  
> +static void v8m_security_lookup(CPUARMState *env, uint32_t address,
> +                                MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                                V8M_SAttributes *sattrs);
> +
>  /* Definitions for the PMCCNTR and PMCR registers */
>  #define PMCRD   0x8
>  #define PMCRC   0x4
> @@ -6724,6 +6728,123 @@ static void arm_log_exception(int idx)
>      }
>  }
>  
> +static bool v7m_read_half_insn(ARMCPU *cpu, ARMMMUIdx mmu_idx, uint16_t 
> *insn)
> +{

This function doesn't take an address ...

> +    if (get_phys_addr(env, env->regs[15], MMU_INST_FETCH, mmu_idx,
> +                      &physaddr, &attrs, &prot, &page_size, &fsr, &fi)) {

... reading it directly from r15 ...

> +    if (insn != 0xe97f) {
> +        /* Not an SG instruction first half (we choose the IMPDEF
> +         * early-SG-check option).
> +         */
> +        goto gen_invep;
> +    }
> +
> +    if (!v7m_read_half_insn(cpu, mmu_idx, &insn)) {
> +        return false;
> +    }
> +
> +    if (insn != 0xe97f) {
> +        /* Not an SG instruction second half */
> +        goto gen_invep;
> +    }

... but somehow expects to get two different values read from the same address?

Certainly you'd get the wrong exception frame if you incremented r15 in between.

> +    env->regs[15] += 4;

... that make this right and the implicit address to the readers wrong.

I don't see anything else amiss in the patch.


r~