[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn con
From: |
Andrew Jones |
Subject: |
Re: [Qemu-devel] [PATCH 4/5] target-arm: get_phys_addr_lpae: more xn control |
Date: |
Tue, 10 Mar 2015 17:48:00 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Mar 10, 2015 at 03:56:11PM +0000, Peter Maydell wrote:
> On 12 February 2015 at 15:05, 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, which also handles short-
> > descriptors and v6 XN determination. 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>
> > ---
> > target-arm/helper.c | 111
> > ++++++++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 86 insertions(+), 25 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index df78f481e92f4..20e5753bd216d 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -4695,8 +4695,8 @@ static inline bool regime_is_user(CPUARMState *env,
> > ARMMMUIdx mmu_idx)
> > /* Translate section/page access permissions to page
> > * R/W protection flags
> > */
> > -static inline int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> > - bool is_user, int ap, int domain_prot)
> > +static int get_rw_prot(CPUARMState *env, ARMMMUIdx mmu_idx,
> > + bool is_user, int ap, int domain_prot)
>
> ...why does this suddenly lose its 'inline' ?
Adding another caller, and thought it was a bit fat for explicit
inlining, but have no problem returning it.
>
> > {
> > bool simple_ap = regime_using_lpae_format(env, mmu_idx)
> > || (regime_sctlr(env, mmu_idx) & SCTLR_AFE);
> > @@ -4762,6 +4762,84 @@ static inline int get_rw_prot(CPUARMState *env,
> > ARMMMUIdx mmu_idx,
> > }
> > }
> >
> > +/* Translate section/page access permissions to protection flags */
>
> This is LPAE-format only so it would be nice to mention that in the comment
> and function name.
Not after the next patch. I probably should have made it completely
LPAE-only first, then added two more patches, one preparing it for
non-LPAE, and then the next patch, or just done a better job pointing
that out in the commit message.
>
> > +static int get_S1prot(CPUARMState *env, ARMMMUIdx mmu_idx, bool is_aa64,
> > + int ap, int domain_prot, int ns, int xn, int pxn)
> > +{
> > + bool domain_prot_valid = !regime_using_lpae_format(env, mmu_idx);
>
> But this is always false!
Same response as above - not after next patch.
>
> > + bool is_user = regime_is_user(env, mmu_idx);
> > + bool have_wxn;
> > + int prot_rw, user_rw;
> > + int wxn = 0;
> > +
> > + assert(mmu_idx != ARMMMUIdx_S2NS);
> > +
> > + if (domain_prot_valid && domain_prot == 3) {
> > + return PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > + }
> > +
> > + user_rw = get_rw_prot(env, mmu_idx, true, ap, domain_prot);
> > + if (is_user) {
> > + prot_rw = user_rw;
> > + } else {
> > + prot_rw = get_rw_prot(env, mmu_idx, false, ap, domain_prot);
> > + }
>
> I think it would be much better not to try to use the short-descriptor
> get_rw_prot function. For one thing, we know for definite that we
> won't be using the old-fashioned AP[2:0] access format, and that
> we don't have to worry about the domain protection stuff. So it's
> much simpler and better not to tangle it up with the legacy stuff.
> (Pull the simple-access-permissions check out into its own function
> if you like.)
legacy stuff is here for next patch too
>
> For instance, you're missing a shift here on the ap bits, because
> get_rw_prot needs AP[2:0] and 'ap' here is AP[2:1].
Don't need the shift because get_rw_prot supports the 2-bit format.
>
> > +
> > + if (ns && arm_is_secure(env) && (env->cp15.scr_el3 & SCR_SIF)) {
> > + return prot_rw;
> > + }
> > +
> > + /* TODO have_wxn should be replaced with arm_feature(env,
> > ARM_FEATURE_EL2),
> > + * when ARM_FEATURE_EL2 starts getting set. For now we assume all v7
> > + * compatible processors have EL2, which is required for [U]WXN.
> > + */
> > + have_wxn = arm_feature(env, ARM_FEATURE_V7);
>
> ARMv8 CPUs without EL2 still have WXN, I think.
I think so too. So V8 || (V7 && EL2) would be the most appropriate.
>
> > +
> > + if (have_wxn) {
> > + wxn = regime_sctlr(env, mmu_idx) & SCTLR_WXN;
> > + }
> > +
> > + if (is_aa64) {
> > + assert(arm_feature(env, ARM_FEATURE_V8));
>
> I wouldn't bother with this assert.
OK
>
> > + switch (regime_el(env, mmu_idx)) {
> > + case 1:
> > + if (is_user && !user_rw) {
> > + wxn = 0;
> > + } else if (!is_user) {
> > + xn = pxn || (user_rw & PAGE_WRITE);
> > + }
> > + break;
> > + case 2:
> > + case 3:
> > + break;
> > + }
> > + } else if (arm_feature(env, ARM_FEATURE_V6K)) {
>
> Always true, since you can't have long format descriptors
> unless this is at least v7.
Next patch again.
>
> > + switch (regime_el(env, mmu_idx)) {
> > + case 1:
> > + case 3:
> > + if (is_user) {
> > + xn = xn || !user_rw;
> > + } else {
> > + int uwxn = 0;
> > + if (have_wxn) {
> > + uwxn = regime_sctlr(env, mmu_idx) & SCTLR_UWXN;
> > + }
> > + xn = xn || !prot_rw || pxn || (uwxn && (user_rw &
> > PAGE_WRITE));
> > + }
> > + break;
>
> Doesn't this lose us the "you need read permission to execute"
> check (for 32-bit)? Something in here should be doing a
> PAGE_READ check to see if we can have PAGE_EXEC.
It's there. It's the '!user_rw' and the '!prot_rw'
>
> > + case 2:
> > + break;
> > + }
> > + } else {
> > + xn = wxn = 0;
> > + }
> > +
> > + if (xn || (wxn && (prot_rw & PAGE_WRITE))) {
> > + return prot_rw;
> > + }
> > + return prot_rw | PAGE_EXEC;
>
>
> > +}
> > +
> > static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
> > uint32_t *table, uint32_t address)
> > {
> > @@ -5047,7 +5125,6 @@ static int get_phys_addr_lpae(CPUARMState *env,
> > target_ulong address,
> > int32_t granule_sz = 9;
> > int32_t va_size = 32;
> > int32_t tbi = 0;
> > - bool is_user;
> > TCR *tcr = regime_tcr(env, mmu_idx);
> >
> > /* TODO:
> > @@ -5220,31 +5297,15 @@ static int get_phys_addr_lpae(CPUARMState *env,
> > target_ulong address,
> > /* Access flag */
> > goto do_fault;
> > }
> > +
> > + *prot = get_S1prot(env, mmu_idx, va_size == 64, extract32(attrs, 4,
> > 2), 0,
> > + extract32(attrs, 3, 1), extract32(attrs, 12, 1),
> > + extract32(attrs, 11, 1));
>
> Urgh. It would be easier to understand if you just passed attrs
> into get_S1prot and had it pick the fields apart, because then
> you can match them up with variable names without cross-referencing
> against the function definition.
I can pick them apart before passing into local well-named variables.
I'd prefer to keep the helper function independent of get_phys_addr_lpae's
internal bitmap.
>
> > +
> > fault_type = permission_fault;
> > - is_user = regime_is_user(env, mmu_idx);
> > - if (is_user && !(attrs & (1 << 4))) {
> > - /* Unprivileged access not enabled */
> > + if (!(*prot & (1 << access_type))) {
> > goto do_fault;
> > }
> > - *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> > - if ((arm_feature(env, ARM_FEATURE_V8) && is_user && (attrs & (1 <<
> > 12))) ||
> > - (!arm_feature(env, ARM_FEATURE_V8) && (attrs & (1 << 12))) ||
> > - (!is_user && (attrs & (1 << 11)))) {
> > - /* XN/UXN or PXN. Since we only implement EL0/EL1 we
> > unconditionally
> > - * treat XN/UXN as UXN for v8.
> > - */
> > - if (access_type == 2) {
> > - goto do_fault;
> > - }
> > - *prot &= ~PAGE_EXEC;
> > - }
> > - if (attrs & (1 << 5)) {
> > - /* Write access forbidden */
> > - if (access_type == 1) {
> > - goto do_fault;
> > - }
> > - *prot &= ~PAGE_WRITE;
> > - }
> >
> > *phys_ptr = descaddr;
> > *page_size_ptr = page_size;
> > --
> > 1.9.3
> >
>
>
> -- PMM