target/ppc/mmu-hash64.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
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 =
>
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.
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.
>
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.
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.
>
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.
> 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.
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
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
© 2016 - 2026 Red Hat, Inc.