[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control |
Date: |
Wed, 11 Mar 2015 17:02:00 +0000 |
On 10 March 2015 at 21:06, Andrew Jones <address@hidden> wrote:
> This patch makes the following changes to the determination of
> whether an address is executable, when translating addresses
> using LPAE.
>
> 1. No longer assumes that PL0 can't execute when it can't read.
> It can in AArch64, a difference from AArch32.
> 2. Use va_size == 64 to determine we're in AArch64, rather than
> arm_feature(env, ARM_FEATURE_V8), which is insufficient.
> 3. Add additional XN determinants
> - NS && is_secure && (SCR & SCR_SIF)
> - WXN && (prot & PAGE_WRITE)
> - AArch64: (prot_PL0 & PAGE_WRITE)
> - AArch32: UWXN && (prot_PL0 & PAGE_WRITE)
> - XN determination should also work in secure mode (untested)
> - XN may even work in EL2 (currently impossible to test)
> 4. Cleans up the bloated PAGE_EXEC condition - by removing it.
>
> The helper get_S1prot is introduced. It may even work in EL2,
> when support for that comes, but, as the function name implies,
> it only works for stage 1 translations.
>
> Signed-off-by: Andrew Jones <address@hidden>
I like the general shape of this patch. Minor comment below:
> ---
> target-arm/helper.c | 129
> ++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 100 insertions(+), 29 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index d996659652f8d..c457e9ab8c85a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4962,15 +4962,11 @@ static inline int ap_to_rw_prot(CPUARMState *env,
> ARMMMUIdx mmu_idx,
> /* Translate section/page access permissions to page
> * R/W protection flags.
> *
> - * @env: CPUARMState
> - * @mmu_idx: MMU index indicating required translation regime
> * @ap: The 2-bit simple AP (AP[2:1])
> + * @is_user: TRUE if accessing from PL0
> */
> -static inline int
> -simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> +static inline int simple_ap_to_rw_prot_is_user(int ap, bool is_user)
> {
> - bool is_user = regime_is_user(env, mmu_idx);
> -
> switch (ap) {
> case 0:
> return is_user ? 0 : PAGE_READ | PAGE_WRITE;
> @@ -4985,6 +4981,94 @@ simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx
> mmu_idx, int ap)
> }
> }
>
> +static inline int
> +simple_ap_to_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx, int ap)
> +{
> + return simple_ap_to_rw_prot_is_user(ap, regime_is_user(env, mmu_idx));
> +}
> +
> +/* Translate section/page access permissions to protection flags
> + *
> + * @env: CPUARMState
> + * @mmu_idx: MMU index indicating required translation regime
> + * @is_aa64: TRUE if AArch64
> + * @ap: The 2-bit simple AP (AP[2:1])
> + * @ns: NS (non-secure) bit
> + * @xn: XN (execute-never) bit
> + * @pxn: PXN (privileged execute-never) bit
> + */
> +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> + int ap, int ns, int xn, int pxn)
> +{
> + bool is_user = regime_is_user(env, mmu_idx);
> + int prot_rw, user_rw;
> + bool have_wxn;
> + int wxn = 0;
> +
> + assert(mmu_idx != ARMMMUIdx_S2NS);
> +
> + user_rw = simple_ap_to_rw_prot_is_user(ap, true);
> + if (is_user) {
> + prot_rw = user_rw;
> + } else {
> + prot_rw = simple_ap_to_rw_prot_is_user(ap, false);
> + }
> +
> + if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> + return prot_rw;
> + }
> +
> + /* TODO have_wxn should be replaced with
> + * ARM_FEATURE_V8 || (ARM_FEATURE_V7 && ARM_FEATURE_EL2)
> + * when ARM_FEATURE_EL2 starts getting set. For now we assume all LPAE
> + * compatible processors have EL2, which is required for [U]WXN.
> + */
> + have_wxn = arm_feature(env, ARM_FEATURE_LPAE);
> +
> + if (have_wxn) {
> + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> + }
> +
> + if (is_aa64) {
> + switch (regime_el(env, mmu_idx)) {
> + case 1:
> + if (is_user && !user_rw) {
> + wxn = 0;
I don't understand this. We ignore the WXN bit if this is
a user access and the page is not readable ?
I also find the naming of this variable "user_rw" (and
to a lesser extent "prot_rw") very confusing. I keep
misreading "if (user_rw)" as meaning "if this page is
read-write for the user", when in fact it only means
"if this page is readable for the user".
Maybe it would be less confusing if we always did tests
against a set of PAGE_* flags rather than doing an
is/is-not-zero test?
The rest looked OK to me.
-- PMM
- [Qemu-devel] [PATCH v2 0/3] tcg-arm: LPAE: fix and extend xn control, Andrew Jones, 2015/03/10
- [Qemu-devel] [PATCH v2 1/3] target-arm: convert check_ap to ap_to_rw_prot, Andrew Jones, 2015/03/10
- [Qemu-devel] [PATCH v2 2/3] target-arm: fix get_phys_addr_v6/SCTLR_AFE access check, Andrew Jones, 2015/03/10
- [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Andrew Jones, 2015/03/10
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control,
Peter Maydell <=
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Andrew Jones, 2015/03/11
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Peter Maydell, 2015/03/11
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Andrew Jones, 2015/03/11
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Peter Maydell, 2015/03/11
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Andrew Jones, 2015/03/11
- Re: [Qemu-devel] [PATCH v2 3/3] target-arm: get_phys_addr_lpae: more xn control, Peter Maydell, 2015/03/11