qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [PATCH 6/7] target/arm: Factor MPU lookup co


From: Philippe Mathieu-Daudé
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH 6/7] target/arm: Factor MPU lookup code out of get_phys_addr_pmsav8()
Date: Tue, 5 Dec 2017 18:27:49 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> For the TT instruction we're going to need to do an MPU lookup that
> also tells us which MPU region the access hit. This requires us
> to do the MPU lookup without first doing the SAU security access
> check, so pull the MPU lookup parts of get_phys_addr_pmsav8()
> out into their own function.
> 
> The TT instruction also needs to know the MPU region number which
> the lookup hit, so provide this information to the caller of the
> MPU lookup code, even though get_phys_addr_pmsav8() doesn't
> need to know it.
> 
> Signed-off-by: Peter Maydell <address@hidden>

Elegant refactor!

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  target/arm/helper.c | 130 
> +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 79 insertions(+), 51 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 14ab1f4..be8d797 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9351,67 +9351,28 @@ static void v8m_security_lookup(CPUARMState *env, 
> uint32_t address,
>      }
>  }
>  
> -static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
> -                                 MMUAccessType access_type, ARMMMUIdx 
> mmu_idx,
> -                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
> -                                 int *prot, uint32_t *fsr)
> +static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> +                              MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                              hwaddr *phys_ptr, MemTxAttrs *txattrs,
> +                              int *prot, uint32_t *fsr, uint32_t *mregion)
>  {
> +    /* Perform a PMSAv8 MPU lookup (without also doing the SAU check
> +     * that a full phys-to-virt translation does).
> +     * mregion is (if not NULL) set to the region number which matched,
> +     * or -1 if no region number is returned (MPU off, address did not
> +     * hit a region, address hit in multiple regions).
> +     */
>      ARMCPU *cpu = arm_env_get_cpu(env);
>      bool is_user = regime_is_user(env, mmu_idx);
>      uint32_t secure = regime_is_secure(env, mmu_idx);
>      int n;
>      int matchregion = -1;
>      bool hit = false;
> -    V8M_SAttributes sattrs = {};
>  
>      *phys_ptr = address;
>      *prot = 0;
> -
> -    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> -        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
> -        if (access_type == MMU_INST_FETCH) {
> -            /* Instruction fetches always use the MMU bank and the
> -             * transaction attribute determined by the fetch address,
> -             * regardless of CPU state. This is painful for QEMU
> -             * to handle, because it would mean we need to encode
> -             * into the mmu_idx not just the (user, negpri) information
> -             * for the current security state but also that for the
> -             * other security state, which would balloon the number
> -             * of mmu_idx values needed alarmingly.
> -             * Fortunately we can avoid this because it's not actually
> -             * possible to arbitrarily execute code from memory with
> -             * the wrong security attribute: it will always generate
> -             * an exception of some kind or another, apart from the
> -             * special case of an NS CPU executing an SG instruction
> -             * in S&NSC memory. So we always just fail the translation
> -             * here and sort things out in the exception handler
> -             * (including possibly emulating an SG instruction).
> -             */
> -            if (sattrs.ns != !secure) {
> -                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
> -                return true;
> -            }
> -        } else {
> -            /* For data accesses we always use the MMU bank indicated
> -             * by the current CPU state, but the security attributes
> -             * might downgrade a secure access to nonsecure.
> -             */
> -            if (sattrs.ns) {
> -                txattrs->secure = false;
> -            } else if (!secure) {
> -                /* NS access to S memory must fault.
> -                 * Architecturally we should first check whether the
> -                 * MPU information for this address indicates that we
> -                 * are doing an unaligned access to Device memory, which
> -                 * should generate a UsageFault instead. QEMU does not
> -                 * currently check for that kind of unaligned access though.
> -                 * If we added it we would need to do so as a special case
> -                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
> -                 */
> -                *fsr = M_FAKE_FSR_SFAULT;
> -                return true;
> -            }
> -        }
> +    if (mregion) {
> +        *mregion = -1;
>      }
>  
>      /* Unlike the ARM ARM pseudocode, we don't need to check whether this
> @@ -9500,12 +9461,79 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, 
> uint32_t address,
>          /* We don't need to look the attribute up in the MAIR0/MAIR1
>           * registers because that only tells us about cacheability.
>           */
> +        if (mregion) {
> +            *mregion = matchregion;
> +        }
>      }
>  
>      *fsr = 0x00d; /* Permission fault */
>      return !(*prot & (1 << access_type));
>  }
>  
> +
> +static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
> +                                 MMUAccessType access_type, ARMMMUIdx 
> mmu_idx,
> +                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
> +                                 int *prot, uint32_t *fsr)
> +{
> +    uint32_t secure = regime_is_secure(env, mmu_idx);
> +    V8M_SAttributes sattrs = {};
> +
> +    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> +        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
> +        if (access_type == MMU_INST_FETCH) {
> +            /* Instruction fetches always use the MMU bank and the
> +             * transaction attribute determined by the fetch address,
> +             * regardless of CPU state. This is painful for QEMU
> +             * to handle, because it would mean we need to encode
> +             * into the mmu_idx not just the (user, negpri) information
> +             * for the current security state but also that for the
> +             * other security state, which would balloon the number
> +             * of mmu_idx values needed alarmingly.
> +             * Fortunately we can avoid this because it's not actually
> +             * possible to arbitrarily execute code from memory with
> +             * the wrong security attribute: it will always generate
> +             * an exception of some kind or another, apart from the
> +             * special case of an NS CPU executing an SG instruction
> +             * in S&NSC memory. So we always just fail the translation
> +             * here and sort things out in the exception handler
> +             * (including possibly emulating an SG instruction).
> +             */
> +            if (sattrs.ns != !secure) {
> +                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
> +                *phys_ptr = address;
> +                *prot = 0;
> +                return true;
> +            }
> +        } else {
> +            /* For data accesses we always use the MMU bank indicated
> +             * by the current CPU state, but the security attributes
> +             * might downgrade a secure access to nonsecure.
> +             */
> +            if (sattrs.ns) {
> +                txattrs->secure = false;
> +            } else if (!secure) {
> +                /* NS access to S memory must fault.
> +                 * Architecturally we should first check whether the
> +                 * MPU information for this address indicates that we
> +                 * are doing an unaligned access to Device memory, which
> +                 * should generate a UsageFault instead. QEMU does not
> +                 * currently check for that kind of unaligned access though.
> +                 * If we added it we would need to do so as a special case
> +                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
> +                 */
> +                *fsr = M_FAKE_FSR_SFAULT;
> +                *phys_ptr = address;
> +                *prot = 0;
> +                return true;
> +            }
> +        }
> +    }
> +
> +    return pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr,
> +                             txattrs, prot, fsr, NULL);
> +}
> +
>  static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>                                   MMUAccessType access_type, ARMMMUIdx 
> mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]