[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Cédric Le Goater
Subject: Re: [PATCH] target/ppc: fix Hash64 MMU update of PTE bit R
Date: Wed, 24 Nov 2021 21:09:14 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

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: 

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")



    > 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 
    >>   {
    >> -    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 
    > around the code and forcing us to go to the ISA every time we wonder 
    > an apparently random number represents. There's also a "HPTE64_R_R" 
    > 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 
    > 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]