[PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
offset, causing the first byte of the adjacent PTE to be corrupted.
This caused a panic when booting FreeBSD, using the Hash MMU.

Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
 target/ppc/mmu-hash64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 19832c4b46..f165ac691a 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
 
 static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
 {
-    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
+    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
 
     if (cpu->vhyp) {
         PPCVirtualHypervisorClass *vhc =
-- 
2.25.1

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Daniel Henrique Barboza]

On 11/24/21 09:00, Leandro Lupori wrote:
> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
> offset, causing the first byte of the adjacent PTE to be corrupted.
> This caused a panic when booting FreeBSD, using the Hash MMU.

If you add a "Fixes:" tag with the commit that introduced the code you're
fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
break anything else, of course).

The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
Rework R and C bit updates")

One more comment below:

> 
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
>   target/ppc/mmu-hash64.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 19832c4b46..f165ac691a 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>   
>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>   {
> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;

Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
around the code and forcing us to go to the ISA every time we wonder what's
an apparently random number represents. There's also a "HPTE64_R_R" defined
there but I'm not sure if it's usable here, so feel free to create a new
macro if needed.

In that note, the original commit that added this code also added a lot of
hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
If you're feeling generous I believe that another patch replacing these hardcoded values
with bit shift macros is warranted as well.


Thanks,


Daniel

>   
>       if (cpu->vhyp) {
>           PPCVirtualHypervisorClass *vhc =
> 

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Cédric Le Goater]

On 11/24/21 14:40, Daniel Henrique Barboza wrote:
> 
> 
> On 11/24/21 09:00, Leandro Lupori wrote:
>> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>> offset, causing the first byte of the adjacent PTE to be corrupted.
>> This caused a panic when booting FreeBSD, using the Hash MMU.

I wonder how we never hit this issue before. Are you testing PowerNV
and/or pSeries  ?

Could you share a FreeBDS image with us ?

> If you add a "Fixes:" tag with the commit that introduced the code you're
> fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
> break anything else, of course).
>
> The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
> Rework R and C bit updates")

Indeed.

> One more comment below:
> 
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>>   target/ppc/mmu-hash64.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>> index 19832c4b46..f165ac691a 100644
>> --- a/target/ppc/mmu-hash64.c
>> +++ b/target/ppc/mmu-hash64.c
>> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>>   {
>> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
>> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
> 
> Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
> value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
> around the code and forcing us to go to the ISA every time we wonder what's
> an apparently random number represents. There's also a "HPTE64_R_R" defined
> there but I'm not sure if it's usable here, so feel free to create a new
> macro if needed.
> 
> In that note, the original commit that added this code also added a lot of
> hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
> ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
> If you're feeling generous I believe that another patch replacing these hardcoded values
> with bit shift macros is warranted as well.

May be for 7.0 though ?

Thanks,

C.

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Daniel Henrique Barboza]

On 11/24/21 15:42, Cédric Le Goater wrote:
> On 11/24/21 14:40, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/24/21 09:00, Leandro Lupori wrote:
>>> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>>> offset, causing the first byte of the adjacent PTE to be corrupted.
>>> This caused a panic when booting FreeBSD, using the Hash MMU.
> 
> I wonder how we never hit this issue before. Are you testing PowerNV
> and/or pSeries  ?
> 
> Could you share a FreeBDS image with us ?
> 
>> If you add a "Fixes:" tag with the commit that introduced the code you're
>> fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
>> break anything else, of course).
>>
>> The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
>> Rework R and C bit updates")
> 
> Indeed.
> 
>> One more comment below:
>>
>>>
>>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>>> ---
>>>   target/ppc/mmu-hash64.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>>> index 19832c4b46..f165ac691a 100644
>>> --- a/target/ppc/mmu-hash64.c
>>> +++ b/target/ppc/mmu-hash64.c
>>> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>>>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>>>   {
>>> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
>>> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
>>
>> Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
>> value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
>> around the code and forcing us to go to the ISA every time we wonder what's
>> an apparently random number represents. There's also a "HPTE64_R_R" defined
>> there but I'm not sure if it's usable here, so feel free to create a new
>> macro if needed.
>>
>> In that note, the original commit that added this code also added a lot of
>> hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
>> ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
>> If you're feeling generous I believe that another patch replacing these hardcoded values
>> with bit shift macros is warranted as well.
> 
> May be for 7.0 though ?


Yeah, this extra cleanup I proposed can be postponed to 7.0 in case someone wants
to give it a go.


Daniel



> 
> Thanks,
> 
> C.
> 

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

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

​​

On 11/24/21 14:40, Daniel Henrique Barboza wrote:
>
>
> On 11/24/21 09:00, Leandro Lupori wrote:
>> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>> offset, causing the first byte of the adjacent PTE to be corrupted.
>> This caused a panic when booting FreeBSD, using the Hash MMU.

I wonder how we never hit this issue before. Are you testing PowerNV
and/or pSeries  ?

Could you share a FreeBDS image with us ?
​I've hit this issue while testing PowerNV. With pSeries it doesn't happen.

It can be reproduced by trying to boot this iso: https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/FreeBSD-14.0-CURRENT-powerpc-powerpc64-20211028-4827bf76bce-250301-disc1.iso.xz

It is easier to reproduce it using power8/powernv8.
​
> If you add a "Fixes:" tag with the commit that introduced the code you're
> fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
> break anything else, of course).
>
> The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
> Rework R and C bit updates")

Indeed.
​​Right.

> One more comment below:
>
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>>   target/ppc/mmu-hash64.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>> index 19832c4b46..f165ac691a 100644
>> --- a/target/ppc/mmu-hash64.c
>> +++ b/target/ppc/mmu-hash64.c
>> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>>   {
>> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
>> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
>
> Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
> value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
> around the code and forcing us to go to the ISA every time we wonder what's
> an apparently random number represents. There's also a "HPTE64_R_R" defined
> there but I'm not sure if it's usable here, so feel free to create a new
> macro if needed.
>
> In that note, the original commit that added this code also added a lot of
> hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
> ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
> If you're feeling generous I believe that another patch replacing these hardcoded values
> with bit shift macros is warranted as well.
​What about creating HPTE64_R_R_BYTE and HPTE64_R_C_BYTE, with the values 14 and 15, respectively,
to make it clear that these are byte offsets within a PTE?

May be for 7.0 though ?

Thanks,

C.


[-- Attachment #2: Type: text/html, Size: 13225 bytes --]

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Daniel Henrique Barboza]

On 11/24/21 16:17, Leandro Lupori wrote:
> ​​
> 
> 
>     On 11/24/21 14:40, Daniel Henrique Barboza wrote:
>     >
>     >
>     > On 11/24/21 09:00, Leandro Lupori wrote:
>     >> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>     >> offset, causing the first byte of the adjacent PTE to be corrupted.
>     >> This caused a panic when booting FreeBSD, using the Hash MMU.
> 
>     I wonder how we never hit this issue before. Are you testing PowerNV
>     and/or pSeries  ?
> 
>     Could you share a FreeBDS image with us ?
> 
> ​I've hit this issue while testing PowerNV. With pSeries it doesn't happen.
> 
> It can be reproduced by trying to boot this iso: https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/FreeBSD-14.0-CURRENT-powerpc-powerpc64-20211028-4827bf76bce-250301-disc1.iso.xz
> 
> It is easier to reproduce it using power8/powernv8.
> ​
> 
>     > If you add a "Fixes:" tag with the commit that introduced the code you're
>     > fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
>     > break anything else, of course).
>     >
>     > The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
>     > Rework R and C bit  updates")
> 
>     Indeed.
> 
> ​​Right.
> 
>     > One more comment below:
>     >
>     >>
>     >> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>     >> ---
>     >>   target/ppc/mmu-hash64.c | 2 +-
>     >>   1 file changed, 1 insertion(+), 1 deletion(-)
>     >>
>     >> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>     >> index 19832c4b46..f165ac691a 100644
>     >> --- a/target/ppc/mmu-hash64.c
>     >> +++ b/target/ppc/mmu-hash64.c
>     >> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>     >>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>     >>   {
>     >> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
>     >> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
>     >
>     > Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
>     > value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
>     > around the code and forcing us to go to the ISA every time we wonder what's
>     > an apparently random number represents. There's also a "HPTE64_R_R" defined
>     > there but I'm not sure if it's usable here, so feel free to create a new
>     > macro if needed.
>     >
>     > In that note, the original commit that added this code also added a lot of
>     > hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
>     > ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
>     > If you're feeling generous I believe that another patch replacing these hardcoded values
>     > with bit shift macros  is warranted as well.
> 
> ​What about creating HPTE64_R_R_BYTEand HPTE64_R_C_BYTE, with the values 14 and 15, respectively,
> to make it clear that these are byte offsets within a PTE?

Looks good to me.


Daniel


> 
>     May be for 7.0 though ?
> 
>     Thanks,
> 
>     C.
> 

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Cédric Le Goater]

On 11/24/21 20:42, Daniel Henrique Barboza wrote:
> 
> 
> On 11/24/21 16:17, Leandro Lupori wrote:
>> ​​
>>
>>
>>     On 11/24/21 14:40, Daniel Henrique Barboza wrote:
>>     >
>>     >
>>     > On 11/24/21 09:00, Leandro Lupori wrote:
>>     >> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
>>     >> offset, causing the first byte of the adjacent PTE to be corrupted.
>>     >> This caused a panic when booting FreeBSD, using the Hash MMU.
>>
>>     I wonder how we never hit this issue before. Are you testing PowerNV
>>     and/or pSeries  ?
>>
>>     Could you share a FreeBDS image with us ?
>>
>> ​I've hit this issue while testing PowerNV. With pSeries it doesn't happen.
>>
>> It can be reproduced by trying to boot this iso: https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/FreeBSD-14.0-CURRENT-powerpc-powerpc64-20211028-4827bf76bce-250301-disc1.iso.xz
>>
>> It is easier to reproduce it using power8/powernv8.
>> ​
>>
>>     > If you add a "Fixes:" tag with the commit that introduced the code you're
>>     > fixing, we can push it right away as a bug fix in 6.2 (assuming it doesn't
>>     > break anything else, of course).
>>     >
>>     > The commit to be fixed in the case seems to be a2dd4e83e76b ("ppc/hash64:
>>     > Rework R and C bit  updates")
>>
>>     Indeed.
>>
>> ​​Right.
>>
>>     > One more comment below:
>>     >
>>     >>
>>     >> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>>     >> ---
>>     >>   target/ppc/mmu-hash64.c | 2 +-
>>     >>   1 file changed, 1 insertion(+), 1 deletion(-)
>>     >>
>>     >> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
>>     >> index 19832c4b46..f165ac691a 100644
>>     >> --- a/target/ppc/mmu-hash64.c
>>     >> +++ b/target/ppc/mmu-hash64.c
>>     >> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>>     >>   static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>>     >>   {
>>     >> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
>>     >> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
>>     >
>>     > Instead of adding a '14' you should add a new #define in mmu-hash64.h with this
>>     > value, something like "HPTE64_R_R_SHIFT". This will avoid hardcoding literals
>>     > around the code and forcing us to go to the ISA every time we wonder what's
>>     > an apparently random number represents. There's also a "HPTE64_R_R" defined
>>     > there but I'm not sure if it's usable here, so feel free to create a new
>>     > macro if needed.
>>     >
>>     > In that note, the original commit that added this code also added a lot of
>>     > hardcoded "15" values for the C bit update in spapr_hpte_set_c() and
>>     > ppc_hash64_set_c(), and a "14" value like you're changing here in spapr_hpte_set_r().
>>     > If you're feeling generous I believe that another patch replacing these hardcoded values
>>     > with bit shift macros  is warranted as well.
>>
>> ​What about creating HPTE64_R_R_BYTEand HPTE64_R_C_BYTE, with the values 14 and 15, respectively,
>> to make it clear that these are byte offsets within a PTE?

  _BYTE_OFFSET may be ?

> Looks good to me.

Please update pSeries while you are it. I think HASH_PTE_SIZE_64 / 2
deserves an offset.

Thanks,

C.

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Cédric Le Goater]

> It can be reproduced by trying to boot this iso: https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/FreeBSD-14.0-CURRENT-powerpc-powerpc64-20211028-4827bf76bce-250301-disc1.iso.xz
> 
> It is easier to reproduce it using power8/powernv8.

power8 only has Hash MMU. I understand that FreeBSD also supports power9
processor. with radix ? and XIVE ?

Thanks,

C.

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[Leandro Lupori]

On 24/11/2021 16:52, Cédric Le Goater wrote:
>> It can be reproduced by trying to boot this iso: 
>> https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/FreeBSD-14.0-CURRENT-powerpc-powerpc64-20211028-4827bf76bce-250301-disc1.iso.xz 
>>
>>
>> It is easier to reproduce it using power8/powernv8.
> 
> power8 only has Hash MMU. I understand that FreeBSD also supports power9
> processor. with radix ? and XIVE ?
> 

Right, FreeBSD supports POWER9 with Radix, that is now the default MMU 
choice. To select Hash MMU, you need to pass radix_mmu=0 to kernel 
command line.
FreeBSD supports XIVE too, but only for PowerNV.

BTW, when trying to boot with Radix instead of Hash, a different issue 
happens. Boot goes further, but then programs start to get SIGILL or 
SIGSEGV.

> Thanks,
> 
> C.

Thanks,
Leandro

Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R

[David Gibson]

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

On Wed, Nov 24, 2021 at 09:00:46AM -0300, Leandro Lupori wrote:
> When updating the R bit of a PTE, the Hash64 MMU was using a wrong byte
> offset, causing the first byte of the adjacent PTE to be corrupted.
> This caused a panic when booting FreeBSD, using the Hash MMU.
> 
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>

Ouch, that's an embarrassing error :/.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/mmu-hash64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 19832c4b46..f165ac691a 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -786,7 +786,7 @@ static void ppc_hash64_set_dsi(CPUState *cs, int mmu_idx, uint64_t dar, uint64_t
>  
>  static void ppc_hash64_set_r(PowerPCCPU *cpu, hwaddr ptex, uint64_t pte1)
>  {
> -    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 16;
> +    hwaddr base, offset = ptex * HASH_PTE_SIZE_64 + 14;
>  
>      if (cpu->vhyp) {
>          PPCVirtualHypervisorClass *vhc =

-- 
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 --]