Re: [PATCH v1 3/6] xen/riscv: add {set,clear}_fixmap() functions for managing fixmap entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> Introduce set_fixmap() and clear_fixmap() functions to manage mappings
> in the fixmap region. The set_fixmap() function maps a 4k page ( as only L0
> is expected to be updated; look at setup_fixmap_mappings() ) at a specified
> fixmap entry using map_pages_to_xen(), while clear_fixmap() removes the
> mapping from a fixmap entry by calling destroy_xen_mappings().
> 
> Both functions ensure that the operations succeed by asserting that their
> respective calls (map_pages_to_xen() and destroy_xen_mappings()) return 0.
> A `BUG_ON` check is used to trigger a failure if any issues occur during
> the mapping or unmapping process.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

However, ...

> @@ -433,3 +434,21 @@ int __init populate_pt_range(unsigned long virt, 
> unsigned long nr_mfns)
>  {
>      return pt_update(virt, INVALID_MFN, nr_mfns, PTE_POPULATE);
>  }
> +
> +/* Map a 4k page in a fixmap entry */
> +void set_fixmap(unsigned int map, mfn_t mfn, unsigned int flags)
> +{
> +    int res;
> +
> +    res = map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL);
> +    BUG_ON(res != 0);
> +}

... imo in such cases it is preferable to go without a local variable:

    if ( map_pages_to_xen(FIXMAP_ADDR(map), mfn, 1, flags | PTE_SMALL) != 0 )
        BUG();

Just to double check: Iirc this BUG would in particular trigger when trying
to set a fixmap slot that was already set, and not intermediately cleared?

Jan