Re: [PATCH 02/62] target/arm: Enable PageEntryExtra

[Richard Henderson]

On 7/4/22 20:52, Peter Maydell wrote:
> On Sun, 3 Jul 2022 at 09:25, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > Copy attrs, sharability, and the NS bit into the TLB.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/arm/cpu-param.h  |  8 ++++++++
> >   target/arm/internals.h  |  5 +++++
> >   target/arm/tlb_helper.c | 14 ++++++++++++--
> >   3 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> > index 68ffb12427..a14f167d11 100644
> > --- a/target/arm/cpu-param.h
> > +++ b/target/arm/cpu-param.h
> > @@ -30,6 +30,14 @@
> >    */
> >   # define TARGET_PAGE_BITS_VARY
> >   # define TARGET_PAGE_BITS_MIN  10
> > +/*
> > + * Extra information stored in softmmu page tables.
> > + */
> > +# define TARGET_PAGE_ENTRY_EXTRA
> > +struct PageEntryExtra {
> > +    /* See PAGEENTRYEXTRA fields in cpu.h */
> > +    uint64_t x;
> > +};
> >   #endif
> >
> >   #define NB_MMU_MODES 15
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index c66f74a0db..2b38a83574 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -74,6 +74,11 @@ FIELD(V7M_EXCRET, DCRS, 5, 1)
> >   FIELD(V7M_EXCRET, S, 6, 1)
> >   FIELD(V7M_EXCRET, RES1, 7, 25) /* including the must-be-1 prefix */
> >
> > +/* Bit definitions for PageEntryExtra */
> > +FIELD(PAGEENTRYEXTRA, ATTRS, 0, 8)
> > +FIELD(PAGEENTRYEXTRA, SHAREABILITY, 8, 2)
> > +FIELD(PAGEENTRYEXTRA, PA, 12, 52)
>
> So why do we want these things in particular? It would be
> helpful to describe the intended uses in the commit message
> to save the reader having to read the next 60 patches to
> find out :-)

Heh, yes.  Basically, it's what S1_ptw_translate requires (pa, attrs), so that we can 
report a stage1 ptw failure, and what do_ats_write requires (pa, sh, attrs) for filling in 
PAR_EL1.  Although within these 62 patches I didn't came back to finish converting 
do_ats_write to use probe_access_flags_extra instead of using get_phys_addr directly, it 
was a goal.

> Is wanting to cache the physaddr an Arm-specific thing, or is it
> something we should consider having in the core softmmu code?

I'm not sure what other targets require for their 2-stage page table walks.  I guess I 
should have a look (riscv, i386, ?).

It *is* possible to recover the phys addr from the iommutlb, because I was doing that in 
mte_helper.c (see code removed in patch 5), but it's certainly not simple.

> >       if (likely(!ret)) {
> > +        PageEntryExtra extra = {};
> > +
> >           /*
> >            * Map a single [sub]page. Regions smaller than our declared
> >            * target page size are handled specially, so for those we
> > -         * pass in the exact addresses.
> > +         * pass in the exact addresses.  This only happens for M-profile,
> > +         * which does not use or require PageEntryExtra.
> >            */
>
> Do we have to exclude M-profile here because the PageEntryExtra
> data is strictly-per-page, or because the way we've formatted
> our extra uint64_t requires the physaddr to be page-aligned, or both?

Because our extra uint64_t requires page alignment, and reuses those bits.


r~