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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

>> 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.

>>> +     * x is the highest page table level for currect  MMU mode (
>>> for example,
>>> +     * for Sv39 has 3 page tables so the x = 2 (L2 -> L1 -> L0) ).
>>> +     *
>>> +     * In this cycle we want to find L1 page table because as L0
>>> page table
>>> +     * xen_fixmap[] will be used.
>>> +     *
>>> +     * i is defined ( HYP_PT_ROOT_LEVEL - 1 ) becuase pte for L2 (
>>> in
>>> +     * case of Sv39 ) has been recieved above.
>>> +     */
>>> +    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )
>>
>> I think the subtracting of 1 is unhelpful here. Think of another 
>> case where
>> you want to walk down to L0. How would you express that with a
>> similar for()
>> construct. It would imo be more natural to use
>>
>>     for ( i = HYP_PT_ROOT_LEVEL; i > 1; i-- )
> Then the first one _i_ will be initialized as L2, not L1. As an option
> we then have to use ...
>>
>> here (and then in that hypothetical other case
>>
>>     for ( i = HYP_PT_ROOT_LEVEL; i > 0; i-- )
>>
>> ), and then the last part of the comment likely wouldn't be needed
>> either.
>> However, considering ...
>>
>>> +    {
>>> +        BUG_ON(!pte_is_valid(*pte));
>>> +
>>> +        pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte));
>>> +        pte = &pte[pt_index(i, FIXMAP_ADDR(0))];
>>
>> ... the use of i here, it may instead want to be
> ... should be ( i - 1 ).
> I am okay with such refactoring.

Well, because of this use of i I specifically suggested ...

>>     for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; )

... this.

>>> +    }
>>> +
>>> +    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?

Jan