Re: [PATCH v3 4/9] xen/riscv: setup fixmap mapping

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-30 at 09:49 +0200, Jan Beulich wrote:
> On 29.07.2024 18:11, oleksii.kurochko@xxxxxxxxx wrote:
> > On Mon, 2024-07-29 at 15:35 +0200, Jan Beulich wrote:
> > > On 24.07.2024 17:31, Oleksii Kurochko wrote:
> > > > @@ -81,6 +82,14 @@ 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)
> > > > +{
> > > > +    RISCV_FENCE(rw, rw);
> > > > +    *p = pte;
> > > > +    RISCV_FENCE(rw, rw);
> > > > +}
> > > 
> > > Why the first of the two fences? 
> > To ensure that writes have completed with the old mapping.
> 
> Wait: There can certainly be uncompleted writes, but those must have
> walked the page tables already, or else a (synchronous) fault could
> not be delivered on the originating store instruction. Or am I
> misunderstanding how paging (and associated faults) work on RISC-V?
I am not sure that I correctly understand the part regarding (
synchronous ) fault. Could you please come up with an example?

If something during page table walk will go wrong then a fault will be
raised.

My initial intension was to be sure if I will be writing to an actively
in-use page table that other cores can see at the time then fences
above are required. It is not the case for now as we have only one CPUs
but I assume that it will be a case when SMP will be enabled and more
then one CPU will be able to work with the same page table.

>>> And isn't having the 2nd here going
>> to badly affect batch updates of page tables?
>> By batch you mean update more then one pte?
>> It yes, then it will definitely affect. It could be dropped here but
>> could we do something to be sure that someone won't miss to add
>> fence/barrier?

> Well, imo you first want to spell out where the responsibilities for
> fencing lies. Then follow the spelled out rules in all new code you
> add.
It makes sense. It would we nice if someone can help me with that.

> 
> > > > +    }
> > > > +
> > > > +    BUG_ON(pte_is_valid(*pte));
> > > > +
> > > > +    tmp = paddr_to_pte(LINK_TO_LOAD((unsigned
> > > > long)&xen_fixmap),
> > > > PTE_TABLE);
> > > 
> > > I'm a little puzzled by the use of LINK_TO_LOAD() (and
> > > LOAD_TO_LINK()
> > > a
> > > little further up) here. Don't you have functioning __pa() and
> > > __va()?
> > No, they haven't been introduced yet.
> 
> So you're building up more technical debt, as the use of said two
> constructs really should be limited to very early setup. Aiui once
> you have functioning __va() / __pa() the code here would want
> changing?

Ideally yes, it would want to changed.

Would it be the better solution to define __va() and __pa() using
LOAD_TO_LINK()/LINK_TO_LOAD() so when "real" __va() and __pa() will be
ready so only definitions of __va() and __pa() should be changed.

~ Oleksii