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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-08-05 at 18:14 +0200, Jan Beulich wrote:
> On 05.08.2024 18:02, oleksii.kurochko@xxxxxxxxx wrote:
> > On Mon, 2024-08-05 at 17:45 +0200, Jan Beulich wrote:
> > > On 05.08.2024 17:13, oleksii.kurochko@xxxxxxxxx wrote:
> > > > On Mon, 2024-07-29 at 15:35 +0200, Jan Beulich wrote:
> > > > > > +    }
> > > > > > +
> > > > > > +    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()?
> > > > Can __pa() and __va() be used in this case?
> > > > 
> > > > According to comments for other architectures, these macros are
> > > > used
> > > > for converting between Xen heap virtual addresses (VA) and
> > > > machine
> > > > addresses (MA). I may have misunderstood what is meant by the
> > > > Xen
> > > > heap
> > > > in this context, but I'm not sure if xen_fixmap[] and page
> > > > tables
> > > > are
> > > > considered part of the Xen heap.
> > > 
> > > I didn't check Arm, but on x86 virt_to_maddr() (underlying
> > > __pa())
> > > has
> > > special case code to also allow addresses within the Xen image
> > > (area).
> > 
> > Yes, it is true for __virt_to_maddr:
> >    static inline unsigned long __virt_to_maddr(unsigned long va)
> >    {
> >        ASSERT(va < DIRECTMAP_VIRT_END);
> >        if ( va >= DIRECTMAP_VIRT_START )
> >            va -= DIRECTMAP_VIRT_START;
> >        else
> >        {
> >            BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
> >            /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an
> > imm32. */
> >            ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
> >                   ((long)XEN_VIRT_START >> (PAGE_ORDER_1G +
> > PAGE_SHIFT)));
> >    
> >            va += xen_phys_start - XEN_VIRT_START;
> >        }
> >        return (va & ma_va_bottom_mask) |
> >               ((va << pfn_pdx_hole_shift) & ma_top_mask);
> >    }
> >    
> > But in case of __maddr_to_virt ( __va() ) it is using directmap
> > region:
> >    static inline void *__maddr_to_virt(unsigned long ma)
> >    {
> >        ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >>
> >    PAGE_SHIFT));
> >        return (void *)(DIRECTMAP_VIRT_START +
> >                        ((ma & ma_va_bottom_mask) |
> >                         ((ma & ma_top_mask) >>
> > pfn_pdx_hole_shift)));
> >    }
> > 
> > But I have to use both __va() and __pa().
> > __va() inside cycle to find L1 page table:
> > 
> >     for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; )
> >     {
> >         BUG_ON(!pte_is_valid(*pte));
> > 
> >         pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte));
> >         pte = &pte[pt_index(i, FIXMAP_ADDR(0))];
> >     }
> > 
> > __pa() to set a physical address of L0 page table:
> >     tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap),
> > PTE_TABLE);
> >     write_pte(pte, tmp);
> 
> Hmm, then using at least LINK_TO_LOAD() is going to be unavoidable
> for the
> time being, I guess. Yet midterm I think they should disappear from
> here.
I think that in this specific case both LINK_TO_LOAD() and
LOAD_TO_LINK() should be used.

__va() -> __maddr_to_virt() can't be used here as it's calculation is
based on DIRECTMAP_VIRT_START thereby we will receive incorrect virtual
address of page table.

__pa() -> __virt_to_maddr() can't be used too because virtual address
of xen_fixmap will be always bigger then DIRECTMAP_VIRT_START ( as
XEN_VIRT_START > DIRECTMAP_VIRT_START ) thereby physical address (PA)
will be calculated based on DIRECTMAP_VIRT_START and incorrect PA of
xen_fixmap will be received.

It seems to me that it is only one choice we have at the moment ( and
probably in the case of fixmap mapping ) is to use
LINK_TO_LOAD()/LOAD_TO_LINK().

~ Oleksii