| Message ID | 20230124000027.3565716-18-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_RME | expand |
On Tue, 24 Jan 2023 at 00:01, Richard Henderson <richard.henderson@linaro.org> wrote: > > This fixes a bug in which we failed to initialize > the result attributes properly after the memset. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/ptw.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/target/arm/ptw.c b/target/arm/ptw.c > index eaa47f6b62..3205339957 100644 > --- a/target/arm/ptw.c > +++ b/target/arm/ptw.c > @@ -32,12 +32,6 @@ typedef struct S1Translate { > void *out_host; > } S1Translate; > > -static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > - uint64_t address, > - MMUAccessType access_type, > - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > - __attribute__((nonnull)); The definition of the function doesn't have the __attribute__, so if we drop this forward declaration we need to move the attribute. > - > static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, > target_ulong address, > MMUAccessType access_type, > @@ -2854,12 +2848,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, > cacheattrs1 = result->cacheattrs; > memset(result, 0, sizeof(*result)); > > - if (arm_feature(env, ARM_FEATURE_PMSA)) { > - ret = get_phys_addr_pmsav8(env, ipa, access_type, > - ptw->in_mmu_idx, s2walk_secure, result, fi); > - } else { > - ret = get_phys_addr_lpae(env, ptw, ipa, access_type, result, fi); > - } > + ret = get_phys_addr_with_struct(env, ptw, ipa, access_type, result, fi); > fi->s2addr = ipa; > > /* Combine the S1 and S2 perms. */ Does this do the right thing for PMSAv8 ? The code in get_phys_addr_twostage sets up various things in ptw based on s2walk_secure, which (per previous patch) is not really well defined for PMSA. thanks -- PMM
On 2/10/23 03:28, Peter Maydell wrote: > On Tue, 24 Jan 2023 at 00:01, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> This fixes a bug in which we failed to initialize >> the result attributes properly after the memset. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/ptw.c | 13 +------------ >> 1 file changed, 1 insertion(+), 12 deletions(-) >> >> diff --git a/target/arm/ptw.c b/target/arm/ptw.c >> index eaa47f6b62..3205339957 100644 >> --- a/target/arm/ptw.c >> +++ b/target/arm/ptw.c >> @@ -32,12 +32,6 @@ typedef struct S1Translate { >> void *out_host; >> } S1Translate; >> >> -static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, >> - uint64_t address, >> - MMUAccessType access_type, >> - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) >> - __attribute__((nonnull)); > > The definition of the function doesn't have the __attribute__, > so if we drop this forward declaration we need to move the attribute. Eh. It was useful as an intermediary during one of the ptw reorgs, but now that we've eliminated the use case in which NULL had been passed, it can go away. I assume you'd prefer that as a separate patch? >> @@ -2854,12 +2848,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, >> cacheattrs1 = result->cacheattrs; >> memset(result, 0, sizeof(*result)); >> >> - if (arm_feature(env, ARM_FEATURE_PMSA)) { >> - ret = get_phys_addr_pmsav8(env, ipa, access_type, >> - ptw->in_mmu_idx, s2walk_secure, result, fi); >> - } else { >> - ret = get_phys_addr_lpae(env, ptw, ipa, access_type, result, fi); >> - } >> + ret = get_phys_addr_with_struct(env, ptw, ipa, access_type, result, fi); >> fi->s2addr = ipa; >> >> /* Combine the S1 and S2 perms. */ > > Does this do the right thing for PMSAv8 ? The code in get_phys_addr_twostage > sets up various things in ptw based on s2walk_secure, which (per previous > patch) is not really well defined for PMSA. As far as I can tell, yes, since as you say current PMSAv8 is all NonSecure. r~
On Mon, 20 Feb 2023 at 22:15, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/10/23 03:28, Peter Maydell wrote: > > On Tue, 24 Jan 2023 at 00:01, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> This fixes a bug in which we failed to initialize > >> the result attributes properly after the memset. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/arm/ptw.c | 13 +------------ > >> 1 file changed, 1 insertion(+), 12 deletions(-) > >> > >> diff --git a/target/arm/ptw.c b/target/arm/ptw.c > >> index eaa47f6b62..3205339957 100644 > >> --- a/target/arm/ptw.c > >> +++ b/target/arm/ptw.c > >> @@ -32,12 +32,6 @@ typedef struct S1Translate { > >> void *out_host; > >> } S1Translate; > >> > >> -static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, > >> - uint64_t address, > >> - MMUAccessType access_type, > >> - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) > >> - __attribute__((nonnull)); > > > > The definition of the function doesn't have the __attribute__, > > so if we drop this forward declaration we need to move the attribute. > > Eh. It was useful as an intermediary during one of the ptw reorgs, but now that we've > eliminated the use case in which NULL had been passed, it can go away. I assume you'd > prefer that as a separate patch? Yes, if we want to deliberately drop the attribute we should do that separately with justification for why it's not needed. thanks -- PMM
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index eaa47f6b62..3205339957 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -32,12 +32,6 @@ typedef struct S1Translate { void *out_host; } S1Translate; -static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, - uint64_t address, - MMUAccessType access_type, - GetPhysAddrResult *result, ARMMMUFaultInfo *fi) - __attribute__((nonnull)); - static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw, target_ulong address, MMUAccessType access_type, @@ -2854,12 +2848,7 @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw, cacheattrs1 = result->cacheattrs; memset(result, 0, sizeof(*result)); - if (arm_feature(env, ARM_FEATURE_PMSA)) { - ret = get_phys_addr_pmsav8(env, ipa, access_type, - ptw->in_mmu_idx, s2walk_secure, result, fi); - } else { - ret = get_phys_addr_lpae(env, ptw, ipa, access_type, result, fi); - } + ret = get_phys_addr_with_struct(env, ptw, ipa, access_type, result, fi); fi->s2addr = ipa; /* Combine the S1 and S2 perms. */
This fixes a bug in which we failed to initialize the result attributes properly after the memset. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/ptw.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)