[Qemu-devel] [RFC PATCH] booke206: fix MAS update on tlb miss

[Qemu-devel] [RFC PATCH] booke206: fix MAS update on tlb miss

[KONRAD Frederic]

When a tlb instruction miss happen, rw is set to 0 at the bottom
of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.

Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 target/ppc/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index b7b9088..f06b938 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address,
                     env->spr[SPR_40x_ESR] = 0x00000000;
                     break;
                 case POWERPC_MMU_BOOKE206:
-                    booke206_update_mas_tlb_miss(env, address, rw);
+                    booke206_update_mas_tlb_miss(env, address, 2);
                     /* fall through */
                 case POWERPC_MMU_BOOKE:
                     cs->exception_index = POWERPC_EXCP_ITLB;
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH] booke206: fix MAS update on tlb miss

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1445 bytes --]

On Tue, Aug 01, 2017 at 10:44:57AM +0200, KONRAD Frederic wrote:
> When a tlb instruction miss happen, rw is set to 0 at the bottom
> of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
> the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
> 
> Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>

Applied to ppc-for-2.10.

> ---
>  target/ppc/mmu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index b7b9088..f06b938 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address,
>                      env->spr[SPR_40x_ESR] = 0x00000000;
>                      break;
>                  case POWERPC_MMU_BOOKE206:
> -                    booke206_update_mas_tlb_miss(env, address, rw);
> +                    booke206_update_mas_tlb_miss(env, address, 2);
>                      /* fall through */
>                  case POWERPC_MMU_BOOKE:
>                      cs->exception_index = POWERPC_EXCP_ITLB;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] booke206: fix MAS update on tlb miss

[Thomas Huth]

On 01.08.2017 10:44, KONRAD Frederic wrote:
> When a tlb instruction miss happen, rw is set to 0 at the bottom
> of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
> the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
> 
> Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  target/ppc/mmu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index b7b9088..f06b938 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address,
>                      env->spr[SPR_40x_ESR] = 0x00000000;
>                      break;
>                  case POWERPC_MMU_BOOKE206:
> -                    booke206_update_mas_tlb_miss(env, address, rw);
> +                    booke206_update_mas_tlb_miss(env, address, 2);

Couldn't that code path be called for normal data read miss (instead of
instruction miss), too?

Anyway, could we please use MMU_INST_FETCH instead of magic values like
2 here?

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] booke206: fix MAS update on tlb miss

[KONRAD Frederic]

On 08/03/2017 01:37 PM, Thomas Huth wrote:
> On 01.08.2017 10:44, KONRAD Frederic wrote:
>> When a tlb instruction miss happen, rw is set to 0 at the bottom
>> of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
>> the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
>>
>> Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   target/ppc/mmu_helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>> index b7b9088..f06b938 100644
>> --- a/target/ppc/mmu_helper.c
>> +++ b/target/ppc/mmu_helper.c
>> @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address,
>>                       env->spr[SPR_40x_ESR] = 0x00000000;
>>                       break;
>>                   case POWERPC_MMU_BOOKE206:
>> -                    booke206_update_mas_tlb_miss(env, address, rw);
>> +                    booke206_update_mas_tlb_miss(env, address, 2);
> 
Hi Thomas,

> Couldn't that code path be called for normal data read miss (instead of
> instruction miss), too?
> 

I don't think so because we have access_type == ACCESS_CODE and
the code in cpu_ppc_handle_mmu_fault explicitely split the CODE
and DATA cases.

> Anyway, could we please use MMU_INST_FETCH instead of magic values like
> 2 here?

I agree it's not nice to have a magic value like this.. But it's
used all over the code there and david took the patch.

So I suggest I send a second patch to fix all the instances of
that magic value.

Fred

> 
>   Thomas
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] booke206: fix MAS update on tlb miss

[Thomas Huth]

On 03.08.2017 14:08, KONRAD Frederic wrote:
> 
> 
> On 08/03/2017 01:37 PM, Thomas Huth wrote:
>> On 01.08.2017 10:44, KONRAD Frederic wrote:
>>> When a tlb instruction miss happen, rw is set to 0 at the bottom
>>> of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
>>> the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
>>>
>>> Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
>>>
>>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>>> ---
>>>   target/ppc/mmu_helper.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>>> index b7b9088..f06b938 100644
>>> --- a/target/ppc/mmu_helper.c
>>> +++ b/target/ppc/mmu_helper.c
>>> @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState
>>> *env, target_ulong address,
>>>                       env->spr[SPR_40x_ESR] = 0x00000000;
>>>                       break;
>>>                   case POWERPC_MMU_BOOKE206:
>>> -                    booke206_update_mas_tlb_miss(env, address, rw);
>>> +                    booke206_update_mas_tlb_miss(env, address, 2);
>>
> Hi Thomas,
> 
>> Couldn't that code path be called for normal data read miss (instead of
>> instruction miss), too?
>>
> 
> I don't think so because we have access_type == ACCESS_CODE and
> the code in cpu_ppc_handle_mmu_fault explicitely split the CODE
> and DATA cases.

Ah, right, I missed that if-statement. So never mind about my comment!

>> Anyway, could we please use MMU_INST_FETCH instead of magic values like
>> 2 here?
> 
> I agree it's not nice to have a magic value like this.. But it's
> used all over the code there and david took the patch.
> 
> So I suggest I send a second patch to fix all the instances of
> that magic value.

Sounds like a good idea!

 Thanks,
  Thomas

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] booke206: fix MAS update on tlb miss

[KONRAD Frederic]

On 08/03/2017 03:13 PM, Thomas Huth wrote:
> On 03.08.2017 14:08, KONRAD Frederic wrote:
>>
>>
>> On 08/03/2017 01:37 PM, Thomas Huth wrote:
>>> On 01.08.2017 10:44, KONRAD Frederic wrote:
>>>> When a tlb instruction miss happen, rw is set to 0 at the bottom
>>>> of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
>>>> the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.
>>>>
>>>> Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.
>>>>
>>>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>>>> ---
>>>>    target/ppc/mmu_helper.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>>>> index b7b9088..f06b938 100644
>>>> --- a/target/ppc/mmu_helper.c
>>>> +++ b/target/ppc/mmu_helper.c
>>>> @@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState
>>>> *env, target_ulong address,
>>>>                        env->spr[SPR_40x_ESR] = 0x00000000;
>>>>                        break;
>>>>                    case POWERPC_MMU_BOOKE206:
>>>> -                    booke206_update_mas_tlb_miss(env, address, rw);
>>>> +                    booke206_update_mas_tlb_miss(env, address, 2);
>>>
>> Hi Thomas,
>>
>>> Couldn't that code path be called for normal data read miss (instead of
>>> instruction miss), too?
>>>
>>
>> I don't think so because we have access_type == ACCESS_CODE and
>> the code in cpu_ppc_handle_mmu_fault explicitely split the CODE
>> and DATA cases.
> 
> Ah, right, I missed that if-statement. So never mind about my comment!
> 
>>> Anyway, could we please use MMU_INST_FETCH instead of magic values like
>>> 2 here?
>>
>> I agree it's not nice to have a magic value like this.. But it's
>> used all over the code there and david took the patch.
>>
>> So I suggest I send a second patch to fix all the instances of
>> that magic value.
> 
> Sounds like a good idea!

Hi Thomas,

Looking more in details at this magic value, it seems rw is
not considered as an access type but more as an is_write boolean.

The whole mmu code use "int access_type" which is kind of
redundant / confusing if we change rw to be MMUAccessType.

Fred

> 
>   Thanks,
>    Thomas
>