| Message ID | 20211124120046.6831-1-leandro.lupori@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: fix Hash64 MMU update of PTE bit R | expand |
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. >
> 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 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.
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 =
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 =
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(-)