Re: [PATCH 3/3] target/ppc: Check page dir/table base alignment

[Richard Henderson]

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~