Re: [PATCH 03/13] accel/tcg: Store some tlb flags in CPUTLBEntryFull

[Peter Maydell]

On Thu, 23 Feb 2023 at 20:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We have run out of bits we can use within the CPUTLBEntry comparators,
> as TLB_FLAGS_MASK cannot overlap alignment.
>
> Store slow_flags[] in CPUTLBEntryFull, and merge with the flags from
> the comparator.  A new TLB_FORCE_SLOW bit is set within the comparator
> as an indication that the slow path must be used.
>
> Move TLB_BSWAP to TLB_SLOW_FLAGS_MASK.  Since we are out of bits,
> we cannot create a new bit without moving an old one.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---


> @@ -1249,36 +1265,27 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
>       * vaddr we add back in io_readx()/io_writex()/get_page_addr_code().
>       */
>      desc->fulltlb[index] = *full;
> -    desc->fulltlb[index].xlat_section = iotlb - vaddr_page;
> -    desc->fulltlb[index].phys_addr = paddr_page;
> +    full = &desc->fulltlb[index];
> +    full->xlat_section = iotlb - vaddr_page;
> +    full->phys_addr = paddr_page;
>
>      /* Now calculate the new entry */
>      tn.addend = addend - vaddr_page;
> -    if (prot & PAGE_READ) {
> -        tn.addr_read = address;
> -        if (wp_flags & BP_MEM_READ) {
> -            tn.addr_read |= TLB_WATCHPOINT;
> -        }
> -    } else {
> -        tn.addr_read = -1;
> -    }
>
> -    if (prot & PAGE_EXEC) {
> -        tn.addr_code = address;
> -    } else {
> -        tn.addr_code = -1;
> -    }
> +    tlb_set_compare(full, &tn, vaddr_page, read_flags,
> +                    MMU_INST_FETCH, prot & PAGE_EXEC);
>
> -    tn.addr_write = -1;
> -    if (prot & PAGE_WRITE) {
> -        tn.addr_write = write_address;
> -        if (prot & PAGE_WRITE_INV) {
> -            tn.addr_write |= TLB_INVALID_MASK;
> -        }
> -        if (wp_flags & BP_MEM_WRITE) {
> -            tn.addr_write |= TLB_WATCHPOINT;
> -        }
> +    if (wp_flags & BP_MEM_READ) {
> +        read_flags |= TLB_WATCHPOINT;
>      }
> +    tlb_set_compare(full, &tn, vaddr_page, read_flags,
> +                    MMU_DATA_LOAD, prot & PAGE_READ);
> +
> +    if (wp_flags & BP_MEM_WRITE) {
> +        write_flags |= TLB_WATCHPOINT;
> +    }
> +    tlb_set_compare(full, &tn, vaddr_page, write_flags, MMU_DATA_STORE,
> +                    (prot & PAGE_WRITE) && !(prot & PAGE_WRITE_INV));

So in the old code, if PAGE_WRITE_INV then we set up the
addr_write field as normal, it just also has the TLB_INVALID_MASK bit
set. In the new code we won't do that, we'll set addr_write to -1.
I'm not fully familiar with the cputlb.c code, but doesn't this
break the code in probe_access_internal(), which assumes that
it can call tlb_fill (which will come through here) and then
fish out the TLB entry, clear out the TLB_INVALID_MASK bit and
use the TLB entry as a one-off ?

thanks
-- PMM