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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> >>> +        "2:\n"
> >>> +        ASM_EXTABLE(1b, 2b)
> >>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> >>> +        : [va] "r" (va)
> >>> +        : "memory" );
> >>> +
> >>> +    arch_pmap_unmap(slot);
> >>> +    free_domheap_page(pg);
> >>> +
> >>> +    return updated;
> >>> +}
> >>
> >> Imo you'd better check that A also was actually set in the PTE.
> > 
> > If I check that A was set, can I drop the "updated" variable? The A bit
> > would tell us more than whether the load succeeded.
> 
> Hmm, I'm inclined to suggest to check both.
Then I'd rather probe D than A, since D, unlike A, cannot be set
speculatively (spec: "updates to the D bit, resulting from an explicit store,
must be exact (i.e., non-speculative)"). The result would
then be:

      return updated && d_set;

with a warning (or panic()?) on updated && !d_set, which the spec
doesn't allow (spec: "the access must not be performed before the PTE update is
globally visible").

Furthermore, if I understood your earlier suggestion correctly, you'd
like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
in use:

    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

However, when both Svade and Svadu are advertised, Svade is in effect
until Svadu is enabled via FWFT (perfectly legit), so this condition
would skip the preset and the first access would fault.

I see two ways to address this:

1) Keep checking Svade:

    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

    After resolve_ad_scheme(), the Svade bit is set in every case where
    software has to preset the A/D bits. The only imprecise case is both
    extensions advertised with hardware updating already enabled by
    firmware, where presetting A/D is unnecessary but harmless.

2) Record the probe result in a global, e.g.

    static bool __ro_after_init hw_ad_update;

    and use:

    if ( !hw_ad_update )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

    This is exact in all cases, at the cost of a new global that mostly
    duplicates what resolve_ad_scheme() already records in the ISA bitmap.

I discussed with Oleksii and we are fine with either. What do you prefer?

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>