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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/9/24 3:29 PM, Jan Beulich wrote:

> 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();
>     
I will update that in the next patch version.

> 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?
>     
Yes, correct.

Thanks.

~ Oleksii