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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-07-29 at 15:35 +0200, Jan Beulich wrote:
> On 24.07.2024 17:31, Oleksii Kurochko wrote:
> > Introduce a function to set up fixmap mappings and L0 page
> > table for fixmap.
> > 
> > Additionally, defines were introduced in riscv/config.h to
> > calculate the FIXMAP_BASE address.
> > This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
> > XEN_VIRT_SIZE, XEN_VIRT_END.
> > 
> > Also, the check of Xen size was updated in the riscv/lds.S
> > script to use XEN_VIRT_SIZE instead of a hardcoded constant.
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> Yet set_fixmap() isn't introduced here, so effectively it's all dead
> code. This could do with mentioning, as I at least would expect
> set_fixmap() to be usable once fixmaps are properly set up.
> 
> > --- a/xen/arch/riscv/include/asm/config.h
> > +++ b/xen/arch/riscv/include/asm/config.h
> > @@ -66,6 +66,14 @@
> >  #define SLOTN_ENTRY_BITS        (HYP_PT_ROOT_LEVEL * VPN_BITS +
> > PAGE_SHIFT)
> >  #define SLOTN(slot)             (_AT(vaddr_t, slot) <<
> > SLOTN_ENTRY_BITS)
> >  
> > +#define XEN_VIRT_SIZE           MB(2)
> > +
> > +#define BOOT_FDT_VIRT_START     (XEN_VIRT_START + XEN_VIRT_SIZE)
> > +#define BOOT_FDT_VIRT_SIZE      MB(4)
> > +
> > +#define FIXMAP_BASE             (BOOT_FDT_VIRT_START +
> > BOOT_FDT_VIRT_SIZE)
> > +#define FIXMAP_ADDR(n)          (FIXMAP_BASE + (n) * PAGE_SIZE)
> > +
> >  #if RV_STAGE1_MODE == SATP_MODE_SV39
> >  #define XEN_VIRT_START 0xFFFFFFFFC0000000
> >  #elif RV_STAGE1_MODE == SATP_MODE_SV48
> 
> Related to my earlier comment: Is there a particular reason that what
> you add cannot live _below_ the XEN_VIRT_START definitions, so things
> actually appear in order?
It can leave _below_ the XEN_VIRT_START definitions, I just wanted to
be in sync with table above.
But I'll move everything below the XEN_VIRT_START as it is used in
newly introduced definitions.
> 
> 
> 
> > @@ -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.

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

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

> 
>     for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; )
> 
> > +    }
> > +
> > +    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.

~ Oleksii