Re: [PATCH v5 19/23] xen/riscv: add minimal stuff to mm.h to build full Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-03-05 at 09:17 +0100, Jan Beulich wrote:
> On 26.02.2024 18:39, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -3,11 +3,252 @@
> >  #ifndef _ASM_RISCV_MM_H
> >  #define _ASM_RISCV_MM_H
> >  
> > +#include <public/xen.h>
> > +#include <xen/bug.h>
> > +#include <xen/mm-frame.h>
> > +#include <xen/pdx.h>
> > +#include <xen/types.h>
> > +
> >  #include <asm/page-bits.h>
> >  
> >  #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
> >  #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
> >  
> > +#define paddr_to_pdx(pa)    mfn_to_pdx(maddr_to_mfn(pa))
> > +#define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
> > +#define gaddr_to_gfn(ga)    _gfn(paddr_to_pfn(ga))
> > +#define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> > +#define maddr_to_mfn(ma)    _mfn(paddr_to_pfn(ma))
> > +#define vmap_to_mfn(va)    
> > maddr_to_mfn(virt_to_maddr((vaddr_t)va))
> 
> va needs parenthesizing here. Also why vaddr_t here but ...
> 
> > +#define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
> > +
> > +static inline unsigned long __virt_to_maddr(unsigned long va)
> > +{
> > +    BUG_ON("unimplemented");
> > +    return 0;
> > +}
> > +
> > +static inline void *__maddr_to_virt(unsigned long ma)
> > +{
> > +    BUG_ON("unimplemented");
> > +    return NULL;
> > +}
> > +
> > +#define virt_to_maddr(va) __virt_to_maddr((unsigned long)(va))
> > +#define maddr_to_virt(pa) __maddr_to_virt((unsigned long)(pa))
> 
> ... unsigned long here? In fact for __maddr_to_virt() I think there
> better wouldn't be any cast, such that the compiler can spot if, by
> mistake, a pointer type value was passed in. Or, wait, we can go
> yet further (also on x86): There are no uses of __maddr_to_virt()
> except here. Hence the symbol isn't needed (anymore?) in the first
> place.
I used 'unsigned long' only because I declared __virt_to_maddr() with
unsigned long argument, but I think it should should be vaddr_t as in
input argument is expected to be VA and for a cast should be used
vaddr_t too.

~ Oleksii