diff mbox series

[17/22] target/arm: Use get_phys_addr_with_struct for stage2

Message ID 20230124000027.3565716-18-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Implement FEAT_RME | expand

Commit Message

Richard Henderson Jan. 24, 2023, midnight UTC
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(-)

Comments

Peter Maydell Feb. 10, 2023, 1:28 p.m. UTC | #1
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
Richard Henderson Feb. 20, 2023, 10:15 p.m. UTC | #2
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~
Peter Maydell Feb. 21, 2023, 11:11 a.m. UTC | #3
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 mbox series

Patch

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.  */