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

[Leandro Lupori]

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~