Re: [PATCH v3 5/6] accel/tcg: Introduce tlb_set_page_full

[Peter Maydell]

On Mon, 5 Sept 2022 at 21:29, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Now that we have collected all of the page data into
> CPUTLBEntryFull, provide an interface to record that
> all in one go, instead of using 4 arguments.  This interface
> allows CPUTLBEntryFull to be extended without having to
> change the number of arguments.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/cpu-defs.h | 14 ++++++++++
>  include/exec/exec-all.h | 22 +++++++++++++++
>  accel/tcg/cputlb.c      | 62 ++++++++++++++++++++++++++++-------------
>  3 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index f70f54d850..5e12cc1854 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -148,7 +148,21 @@ typedef struct CPUTLBEntryFull {
>       *     + the offset within the target MemoryRegion (otherwise)
>       */
>      hwaddr xlat_section;
> +
> +    /*
> +     * @phys_addr contains the physical address in the address space
> +     * given by cpu_asidx_from_attrs(cpu, @attrs).
> +     */
> +    hwaddr phys_addr;
> +
> +    /* @attrs contains the memory transaction attributes for the page. */
>      MemTxAttrs attrs;
> +
> +    /* @prot contains the complete protections for the page. */
> +    uint8_t prot;
> +
> +    /* @lg_page_size contains the log2 of the page size. */
> +    uint8_t lg_page_size;
>  } CPUTLBEntryFull;
>
>  /*
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 758cf6bcc7..1a30c857f4 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -257,6 +257,28 @@ void tlb_flush_range_by_mmuidx_all_cpus_synced(CPUState 
> *cpu,
>                                                 uint16_t idxmap,
>                                                 unsigned bits);
>
> +/**
> + * tlb_set_page_full:
> + * @cpu: CPU context
> + * @mmu_idx: mmu index of the tlb to modify
> + * @vaddr: virtual address of the entry to add
> + * @full: the details of the tlb entry
> + *
> + * Add an entry to @cpu tlb index @mmu_idx.  All of the fields of
> + * @full must be filled, except for xlat_section, and constitute
> + * the complete description of the translated page.
> + *
> + * This is generally called by the target tlb_fill function after
> + * having performed a successful page table walk to find the physical
> + * address and attributes for the translation.
> + *
> + * At most one entry for a given virtual address is permitted. Only a
> + * single TARGET_PAGE_SIZE region is mapped; @full->ld_page_size is only

typo: lg_page_size

> + * used by tlb_flush_page.
> + */
> +void tlb_set_page_full(CPUState *cpu, int mmu_idx, target_ulong vaddr,
> +                       CPUTLBEntryFull *full);
> +
>  /**
>   * tlb_set_page_with_attrs:
>   * @cpu: CPU to add this TLB entry for

> @@ -1272,15 +1275,34 @@ void tlb_set_page_with_attrs(CPUState *cpu, 
> target_ulong vaddr,
>      qemu_spin_unlock(&tlb->c.lock);
>  }
>
> -/* Add a new TLB entry, but without specifying the memory
> - * transaction attributes to be used.
> - */
> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> +                             hwaddr paddr, MemTxAttrs attrs, int prot,
> +                             int mmu_idx, target_ulong size)
> +{
> +    CPUTLBEntryFull full = {
> +        .phys_addr = paddr,
> +        .attrs = attrs,
> +        .prot = prot,
> +        .lg_page_size = ctz64(size)
> +    };
> +
> +    assert(is_power_of_2(size));
> +    tlb_set_page_full(cpu, mmu_idx, vaddr, &full);
> +}
> +
>  void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size)
>  {
> -    tlb_set_page_with_attrs(cpu, vaddr, paddr, MEMTXATTRS_UNSPECIFIED,
> -                            prot, mmu_idx, size);
> +    CPUTLBEntryFull full = {
> +        .phys_addr = paddr,
> +        .attrs = MEMTXATTRS_UNSPECIFIED,
> +        .prot = prot,
> +        .lg_page_size = ctz64(size)
> +    };
> +
> +    assert(is_power_of_2(size));
> +    tlb_set_page_full(cpu, mmu_idx, vaddr, &full);
>  }

Why not just leave tlb_set_page() the way it was? Writing
it out results in this code duplication...

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM