Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 10/13/26 12:07 PM, Baptiste Le Duc wrote:
> +static bool __init probe_hw_ad_update(void)
> +{
> +    bool updated = false;
> +    unsigned int slot = FIX_MISC;
> +    vaddr_t va = FIXMAP_ADDR(slot);
> +    struct page_info *pg;
> +    pte_t *entry, pte;
> +    mfn_t mfn;
> +    unsigned long tmp;
> +
> +    pg = alloc_domheap_page(NULL, 0);
> +    if ( !pg )
> +        return false;
> +
> +    mfn = page_to_mfn(pg);
> +
> +    arch_pmap_map(FIX_MISC, mfn);

Do we need the arch_pmap_map() call here? The PTE it writes is 
overwritten right below by write_pte() (and the TLB flush is repeated 
too), so effectively only its ASSERT(!pte_is_valid(*entry)) remains.

Also, IIUC, arch_pmap_{map,unmap}() are the arch backend of the common 
PMAP infrastructure and are meant for the FIX_PMAP_* slots handed out by
pmap_map(); FIX_MISC isn't one of them. Accessing xen_fixmap[] directly 
is fine here, as set_fixmap() can't be used (map_pages_to_xen() always 
sets A and D), but then I'd do the same for the unmapping ...:

> +    entry = &xen_fixmap[slot];
> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> +    write_pte(entry, pte);
> +
> +    flush_tlb_one_local(va);
> +
> +    asm_inline volatile (
> +        "1: ld %[tmp], 0(%[va])\n"
> +        "   li %[updated], 1\n"
> +        "2:\n"
> +        ASM_EXTABLE(1b, 2b)
> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> +        : [va] "r" (va)
> +        : "memory" );
> +
> +    arch_pmap_unmap(slot);

... I.e. write_pte(entry, (pte_t){}) followed by flush_tlb_one_local(va).

> +    free_domheap_page(pg);
> +
> +    return updated;
> +}


Also:

 > +    asm_inline volatile (
 > +        "1: ld %[tmp], 0(%[va])\n"
 > +        "   li %[updated], 1\n"
 > +        "2:\n"
 > +        ASM_EXTABLE(1b, 2b)
 > +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
 > +        : [va] "r" (va)
 > +        : "memory" );

Since the loaded value isn't needed, how about loading into the zero
register (the load still has to raise any exception) and using plain
asm goto? That needs no CONFIG_CC_HAS_ASM_GOTO_OUTPUT conditional (as I 
originally wanted to suggest), as there are no outputs, and both tmp and 
the LI go away. Together with accessing xen_fixmap[] directly and using 
stack page instead of allocation of new page (I assume something like 
this was suggested by Jan B.), something like this:

static bool __init probe_hw_ad_update(void)
{
     vaddr_t va = FIXMAP_ADDR(FIX_MISC);
     pte_t *entry = &xen_fixmap[FIX_MISC];
     bool updated = false;

     ASSERT(!pte_is_valid(*entry));

     /*
      * Any page in use will do, so alias the stack page this code runs on,
      * with A clear: the read below either faults or has A set by hardware.
      */
     write_pte(entry, pte_from_mfn(maddr_to_mfn(virt_to_maddr(&va)),
                                   PTE_VALID | PTE_READABLE));
     flush_tlb_one_local(va);

     asm_inline goto (
         "1: ld zero, 0(%[va])\n"
         ASM_EXTABLE(1b, %l[out])
         :
         : [va] "r" (va)
         : "memory"
         : out );

     updated = read_pte(entry).pte & PTE_ACCESSED;

  out:
     write_pte(entry, (pte_t){});
     flush_tlb_one_local(va);

     return updated;
}


One things which I also would like to note about:

 > +    asm_inline volatile (
 > +        "1: ld %[tmp], 0(%[va])\n"
 > +        "   li %[updated], 1\n"
 > +        "2:\n"
 > +        ASM_EXTABLE(1b, 2b)

Note that in staging do_trap() consults the exception table only for
CAUSE_ILLEGAL_INSTRUCTION, so on hardware which faults on a clear A bit
the load page fault ends up in do_unexpected_trap() and hence die(),
rather than in the fixup. This is addressed by "xen/riscv: look up the
exception table for any trap taken in Xen context" from the vINTC 
series, so this series depends on it; please mention that in the cover 
letter or at least something like that will be needed for now if i am 
not confusing something:

              break;
          }

+        fallthrough;
+    case CAUSE_LOAD_PAGE_FAULT:
          if ( fixup_exception(cpu_regs) )
              break;

~ Oleksii