| Message ID | 20220620202704.78978-4-leandro.lupori@eldorado.org.br |
|---|---|
| State | New |
| Headers | show |
| Series | ppc: Check for bad Radix configs | expand |
Leandro Lupori <leandro.lupori@eldorado.org.br> writes: > Check if each page dir/table base address is properly aligned and > log a guest error if not, as real hardware behave incorrectly in > this case. > > These checks are only performed when DEBUG_MMU is defined, to avoid > hurting the performance. > > Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> > --- > target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c > index 2f0bcbfe2e..80d945a7c3 100644 > --- a/target/ppc/mmu-radix64.c > +++ b/target/ppc/mmu-radix64.c > @@ -28,6 +28,8 @@ > #include "mmu-radix64.h" > #include "mmu-book3s-v3.h" > > +/* #define DEBUG_MMU */ > + > static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env, > vaddr eaddr, > uint64_t *lpid, uint64_t *pid) > @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr, > if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */ > ++*level; > *nls = pde & R_PDE_NLS; > + > +#ifdef DEBUG_MMU > + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx > + " page dir size: 0x%"PRIx64" level: %d\n", > + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level); > + } > +#endif Maybe use qemu_log_enabled() instead of the define? I think it is a little more useful and has less chance to rot. > + > index = eaddr >> (*psize - *nls); /* Shift */ > index &= ((1UL << *nls) - 1); /* Mask */ > *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde)); > @@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr, > return 1; > } > > +#ifdef DEBUG_MMU > + if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: misaligned page dir base: 0x%"VADDR_PRIx > + " page dir size: 0x%"PRIx64"\n", > + __func__, base_addr, BIT(nls + 3)); > + } > +#endif > + > index = eaddr >> (*psize - nls); /* Shift */ > index &= ((1UL << nls) - 1); /* Mask */ > *pte_addr = base_addr + (index * sizeof(pde));
On 6/21/22 18:26, Fabiano Rosas wrote: > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI. > > Leandro Lupori <leandro.lupori@eldorado.org.br> writes: > >> Check if each page dir/table base address is properly aligned and >> log a guest error if not, as real hardware behave incorrectly in >> this case. >> >> These checks are only performed when DEBUG_MMU is defined, to avoid >> hurting the performance. >> >> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> >> --- >> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c >> index 2f0bcbfe2e..80d945a7c3 100644 >> --- a/target/ppc/mmu-radix64.c >> +++ b/target/ppc/mmu-radix64.c >> @@ -28,6 +28,8 @@ >> #include "mmu-radix64.h" >> #include "mmu-book3s-v3.h" >> >> +/* #define DEBUG_MMU */ >> + >> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env, >> vaddr eaddr, >> uint64_t *lpid, uint64_t *pid) >> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr, >> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */ >> ++*level; >> *nls = pde & R_PDE_NLS; >> + >> +#ifdef DEBUG_MMU >> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx >> + " page dir size: 0x%"PRIx64" level: %d\n", >> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level); >> + } >> +#endif > > Maybe use qemu_log_enabled() instead of the define? I think it is a > little more useful and has less chance to rot. > Ok. I wanted to avoid introducing any extra overhead, but I guess just checking qemu_log_enabled() should be ok. >> + >> index = eaddr >> (*psize - *nls); /* Shift */ >> index &= ((1UL << *nls) - 1); /* Mask */ >> *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde)); >> @@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr, >> return 1; >> } >> >> +#ifdef DEBUG_MMU >> + if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "%s: misaligned page dir base: 0x%"VADDR_PRIx >> + " page dir size: 0x%"PRIx64"\n", >> + __func__, base_addr, BIT(nls + 3)); >> + } >> +#endif >> + >> index = eaddr >> (*psize - nls); /* Shift */ >> index &= ((1UL << nls) - 1); /* Mask */ >> *pte_addr = base_addr + (index * sizeof(pde));
On 6/23/22 07:26, Leandro Lupori wrote: > On 6/21/22 18:26, Fabiano Rosas wrote: >> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o >> remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre >> imediatamente em contato com o DTI. >> >> Leandro Lupori <leandro.lupori@eldorado.org.br> writes: >> >>> Check if each page dir/table base address is properly aligned and >>> log a guest error if not, as real hardware behave incorrectly in >>> this case. >>> >>> These checks are only performed when DEBUG_MMU is defined, to avoid >>> hurting the performance. >>> >>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> >>> --- >>> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c >>> index 2f0bcbfe2e..80d945a7c3 100644 >>> --- a/target/ppc/mmu-radix64.c >>> +++ b/target/ppc/mmu-radix64.c >>> @@ -28,6 +28,8 @@ >>> #include "mmu-radix64.h" >>> #include "mmu-book3s-v3.h" >>> >>> +/* #define DEBUG_MMU */ >>> + >>> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env, >>> vaddr eaddr, >>> uint64_t *lpid, uint64_t *pid) >>> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr, >>> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */ >>> ++*level; >>> *nls = pde & R_PDE_NLS; >>> + >>> +#ifdef DEBUG_MMU >>> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) { >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx >>> + " page dir size: 0x%"PRIx64" level: %d\n", >>> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level); >>> + } >>> +#endif >> >> Maybe use qemu_log_enabled() instead of the define? I think it is a >> little more useful and has less chance to rot. >> > > Ok. I wanted to avoid introducing any extra overhead, but I guess just checking > qemu_log_enabled() should be ok. No, qemu_log_enabled is *already* taken into account by qemu_log_mask. Just drop the ifdefs. Do you in fact need to raise an exception here? r~
On 6/23/22 18:34, Richard Henderson wrote: > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você > possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de > e-mail suspeito entre imediatamente em contato com o DTI. > > On 6/23/22 07:26, Leandro Lupori wrote: >> On 6/21/22 18:26, Fabiano Rosas wrote: >>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você >>> possa confirmar o >>> remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito >>> entre >>> imediatamente em contato com o DTI. >>> >>> Leandro Lupori <leandro.lupori@eldorado.org.br> writes: >>> >>>> Check if each page dir/table base address is properly aligned and >>>> log a guest error if not, as real hardware behave incorrectly in >>>> this case. >>>> >>>> These checks are only performed when DEBUG_MMU is defined, to avoid >>>> hurting the performance. >>>> >>>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> >>>> --- >>>> target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c >>>> index 2f0bcbfe2e..80d945a7c3 100644 >>>> --- a/target/ppc/mmu-radix64.c >>>> +++ b/target/ppc/mmu-radix64.c >>>> @@ -28,6 +28,8 @@ >>>> #include "mmu-radix64.h" >>>> #include "mmu-book3s-v3.h" >>>> >>>> +/* #define DEBUG_MMU */ >>>> + >>>> static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState >>>> *env, >>>> vaddr eaddr, >>>> uint64_t *lpid, >>>> uint64_t *pid) >>>> @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace >>>> *as, vaddr eaddr, >>>> if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */ >>>> ++*level; >>>> *nls = pde & R_PDE_NLS; >>>> + >>>> +#ifdef DEBUG_MMU >>>> + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) { >>>> + qemu_log_mask(LOG_GUEST_ERROR, >>>> + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx >>>> + " page dir size: 0x%"PRIx64" level: %d\n", >>>> + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level); >>>> + } >>>> +#endif >>> >>> Maybe use qemu_log_enabled() instead of the define? I think it is a >>> little more useful and has less chance to rot. >>> >> >> Ok. I wanted to avoid introducing any extra overhead, but I guess just >> checking >> qemu_log_enabled() should be ok. > > No, qemu_log_enabled is *already* taken into account by qemu_log_mask. > Just drop the ifdefs. > > Do you in fact need to raise an exception here? > Not in this case. I've tested it with KVM and it doesn't raise an exception. It seems to just ignore PDE_NLB's bits lower than nls + 3. Thanks, Leandro > > r~
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c index 2f0bcbfe2e..80d945a7c3 100644 --- a/target/ppc/mmu-radix64.c +++ b/target/ppc/mmu-radix64.c @@ -28,6 +28,8 @@ #include "mmu-radix64.h" #include "mmu-book3s-v3.h" +/* #define DEBUG_MMU */ + static bool ppc_radix64_get_fully_qualified_addr(const CPUPPCState *env, vaddr eaddr, uint64_t *lpid, uint64_t *pid) @@ -277,6 +279,16 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr, if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */ ++*level; *nls = pde & R_PDE_NLS; + +#ifdef DEBUG_MMU + if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: misaligned page dir/table base: 0x%"VADDR_PRIx + " page dir size: 0x%"PRIx64" level: %d\n", + __func__, (pde & R_PDE_NLB), BIT(*nls + 3), *level); + } +#endif + index = eaddr >> (*psize - *nls); /* Shift */ index &= ((1UL << *nls) - 1); /* Mask */ *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde)); @@ -297,6 +309,15 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr, return 1; } +#ifdef DEBUG_MMU + if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: misaligned page dir base: 0x%"VADDR_PRIx + " page dir size: 0x%"PRIx64"\n", + __func__, base_addr, BIT(nls + 3)); + } +#endif + index = eaddr >> (*psize - nls); /* Shift */ index &= ((1UL << nls) - 1); /* Mask */ *pte_addr = base_addr + (index * sizeof(pde));
Check if each page dir/table base address is properly aligned and log a guest error if not, as real hardware behave incorrectly in this case. These checks are only performed when DEBUG_MMU is defined, to avoid hurting the performance. Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br> --- target/ppc/mmu-radix64.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)