Re: [PATCH v3 6/6] Convert x86_mmu_translate() to use common code.

[Richard Henderson <richard.henderson@linaro.org>]

On 6/6/24 07:02, Don Porter wrote:
> Signed-off-by: Don Porter <porter@cs.unc.edu>
> ---
>   target/i386/arch_memory_mapping.c    |  44 +++-
>   target/i386/cpu.h                    |   5 +-
>   target/i386/helper.c                 | 374 +++++++--------------------
>   target/i386/tcg/sysemu/excp_helper.c |   2 +-
>   4 files changed, 129 insertions(+), 296 deletions(-)
> 
> diff --git a/target/i386/arch_memory_mapping.c b/target/i386/arch_memory_mapping.c
> index b52e98133c..bccd290b9f 100644
> --- a/target/i386/arch_memory_mapping.c
> +++ b/target/i386/arch_memory_mapping.c
> @@ -228,9 +228,38 @@ static void _mmu_decode_va_parameters(CPUState *cs, int height,
>   }
>   
>   /**
> - * get_pte - Copy the contents of the page table entry at node[i] into pt_entry.
> - *           Optionally, add the relevant bits to the virtual address in
> - *           vaddr_pte.
> + * x86_virtual_to_pte_index - Given a virtual address and height in
> + *       the page table radix tree, return the index that should be
> + *       used to look up the next page table entry (pte) in
> + *       translating an address.
> + *
> + * @cs - CPU state
> + * @vaddr - The virtual address to translate
> + * @height - height of node within the tree (leaves are 1, not 0).
> + *
> + * Example: In 32-bit x86 page tables, the virtual address is split
> + * into 10 bits at height 2, 10 bits at height 1, and 12 offset bits.
> + * So a call with VA and height 2 would return the first 10 bits of va,
> + * right shifted by 22.
> + */
> +
> +int x86_virtual_to_pte_index(CPUState *cs, target_ulong vaddr, int height)
> +{
> +    int shift = 0;
> +    int width = 0;
> +    int mask = 0;
> +
> +    _mmu_decode_va_parameters(cs, height, &shift, &width);
> +
> +    mask = (1 << width) - 1;
> +
> +    return (vaddr >> shift) & mask;
> +}
> +
> +/**
> + * x86_get_pte - Copy the contents of the page table entry at node[i]
> + *               into pt_entry.  Optionally, add the relevant bits to
> + *               the virtual address in vaddr_pte.
>    *
>    * @cs - CPU state
>    * @node - physical address of the current page table node
> @@ -249,7 +278,6 @@ void
>   x86_get_pte(CPUState *cs, hwaddr node, int i, int height,
>               PTE_t *pt_entry, vaddr vaddr_parent, vaddr *vaddr_pte,
>               hwaddr *pte_paddr)
> -
>   {
>       X86CPU *cpu = X86_CPU(cs);
>       CPUX86State *env = &cpu->env;

Some fixes to be merged back into previous patches.


> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -308,7 +308,8 @@ static bool ptw_translate(X86PTETranslate *inout, hwaddr addr, uint64_t ra)
>   
>   static bool x86_mmu_translate(CPUX86State *env, const X86TranslateParams *in,
>                                 X86TranslateResult *out,
> -                              X86TranslateFault *err, uint64_t ra)
> +                              X86TranslateFault *err, uint64_t ra,
> +                              bool read_only)
>   {
>       const target_ulong addr = in->addr;
>       const int pg_mode = in->pg_mode;
> @@ -324,6 +325,10 @@ static bool x86_mmu_translate(CPUX86State *env, const X86TranslateParams *in,
>       uint32_t pkr;
>       int page_size;
>       int error_code;
> +    CPUState *cs = env_cpu(env);
> +    int height;
> +    bool pae_enabled = env->cr[4] & CR4_PAE_MASK;
> +    bool long_mode_enabled = env->hflags & HF_LMA_MASK;

Incorrect.  These bits are in pg_mode...

> -    if (pg_mode & PG_MODE_PAE) {
> -#ifdef TARGET_X86_64
> -        if (pg_mode & PG_MODE_LMA) {
> -            if (pg_mode & PG_MODE_LA57) {

... like so.

> +    /*
> +     * ptep is really an accumulator for the permission bits.
> +     * Thus, the xor-ing totally trashes the high bits, and that is
> +     * ok - we only care about the low ones.
> +     */
> +    ptep = PG_NX_MASK | PG_USER_MASK | PG_RW_MASK;
> +    hwaddr pt_node = x86_page_table_root(cs, &height);
>   
> +    /* Special case for PAE paging */
> +    if (height == 3 && pg_mode & PG_MODE_PAE) {
> +        rsvd_mask |= PG_HI_USER_MASK;
> +    }
>   
> +    int i = height;
> +    do {
> +        int index = x86_virtual_to_pte_index(cs, addr, i);
> +        PTE_t pt_entry;
> +        uint64_t my_rsvd_mask = rsvd_mask;
> +
> +        x86_get_pte(cs, pt_node, index, i, &pt_entry, 0, NULL, &pte_addr);
> +        /* Check that we can access the page table entry */
>           if (!ptw_translate(&pte_trans, pte_addr, ra)) {
>               return false;
>           }

You "get" the pte and only afterward you check that it is accessible.
I think you've missed the point of ptw_translate.


> +
> +    restart:
> +        if (!x86_pte_present(cs, &pt_entry)) {
>               goto do_fault;
>           }
>   
> +        /* For height > 3, check and reject PSE mask */
> +        if (i > 3) {
> +            my_rsvd_mask |= PG_PSE_MASK;
>           }
> +
> +        if (x86_pte_check_bits(cs, &pt_entry, my_rsvd_mask)) {
>               goto do_fault_rsvd;
>           }

Surely the reserved bit checking should be part of the generic walker.
Is there some reason those should be ignored for "info pg", for example?

> +        if (long_mode_enabled) {
> +            pte = pt_entry.pte64_t;
> +        } else {
> +            pte = pt_entry.pte32_t;
>           }

This is pretty ugly.  Ignoring 128-bit ptes for the moment, surely we should just 
zero-extend 32-bit ptes into the 64-bit entry data slot right from the start.

> +            break; /* goto do_check_protect; */

What's with the comment.

> +        if ((!read_only) &&
> +            (!ptw_setl(&pte_trans, pte, PG_ACCESSED_MASK))) {

Again with the extra ().


r~