Re: [PATCH v6 4/9] xen/riscv: set up fixmap mappings

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-09-10 at 12:01 +0200, Jan Beulich wrote:
> On 02.09.2024 19:01, Oleksii Kurochko wrote:
> > Set up fixmap mappings and the L0 page table for fixmap support.
> > 
> > Modify the Page Table Entries (PTEs) directly in arch_pmap_map()
> > instead of using set_fixmap() ( which relies on map_pages_to_xen()
> > ).
> 
> What do you derive this from? There's no set_fixmap() here, and hence
> it's unknown how it is going to be implemented.
I derived it from the my code where is set_fixmap() is implemented but
agree that in brackets it is better to write "will use
map_pages_to_xen()" instead of "which relies on map_pages_to_xen()".

>  The most you can claim
> is that it is expected that it will use map_pages_to_xen(), which in
> turn ...
> 
> > This change is necessary because PMAP is used when the domain map
> > page infrastructure is not yet initialized so map_pages_to_xen()
> > called by set_fixmap() needs to map pages on demand, which then
> > calls pmap() again, resulting in a loop.
> 
> ... is only expected to use arch_pmap_map().
it is what is written in the message, isn't it? But I am okay to change
this part of the commit message to:
   {set, clear}_fixmap() is expected to be implemented using
   map_pages_to_xen(), which, in turn, is only expected to use
   arch_pmap_map().

> 
> > @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned
> > long mfn, bool sync_icache)
> >      BUG_ON("unimplemented");
> >  }
> >  
> > +/* Write a pagetable entry. */
> > +static inline void write_pte(pte_t *p, pte_t pte)
> > +{
> > +    write_atomic(p, pte);
> > +}
> > +
> > +/* Read a pagetable entry. */
> > +static inline pte_t read_pte(pte_t *p)
> 
> const pte_t *?
It would be better to make it const. Thanks.

~ Oleksii