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

[Peter Maydell]

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

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

> +
>  /* Minimum value which is a magic number for exception return */
>  #define EXC_RETURN_MIN_MAGIC 0xff000000
>  /* Minimum number which is a magic number for function or exception return
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 7d8a86b3c4..9de3099153 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -226,21 +226,31 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> size,
>                          &phys_addr, &attrs, &prot, &page_size,
>                          &fi, &cacheattrs);
>      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?

>          if (page_size >= TARGET_PAGE_SIZE) {
>              phys_addr &= TARGET_PAGE_MASK;
>              address &= TARGET_PAGE_MASK;
> +
> +            /* Record some particulars for later lookup. */
> +            extra.x = phys_addr;
> +            extra.x = FIELD_DP64(extra.x, PAGEENTRYEXTRA, ATTRS,
> +                                 cacheattrs.attrs);
> +            extra.x = FIELD_DP64(extra.x, PAGEENTRYEXTRA, SHAREABILITY,
> +                                 cacheattrs.shareability);
>          }
>          /* Notice and record tagged memory. */
>          if (cpu_isar_feature(aa64_mte, cpu) && cacheattrs.attrs == 0xf0) {
>              arm_tlb_mte_tagged(&attrs) = true;
>          }
>
> -        tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
> +        tlb_set_page_with_extra(cs, address, phys_addr, attrs, extra,
>                                  prot, mmu_idx, page_size);
>          return true;
>      } else if (probe) {
> --
> 2.34.1

thanks
-- PMM