Re: [PATCH v1 1/2] xen/riscv: rework asm/mm.h and asm/page.h includes to match other architectures

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/06/2025 4:56 pm, Oleksii Kurochko wrote:
> To align with other architectures where <asm/page.h> is included from 
> <asm/mm.h>
> (and not the other way around), the following changes are made:
> - Since <asm/mm.h> is no longer included in <asm/page.h>:
>   - Move the definitions of paddr_to_pte() and pte_to_paddr() to <asm/mm.h>,
>     as paddr_to_pfn() and pte_to_paddr() are already defined there.
>   - Move _vmap_to_mfn() to <asm/mm.h> because mfn_from_pte() is defined there 
> and
>     open-code it inside macros vmap_to_mfn().
> - Drop the inclusion of <xen/domain_page.h> from <asm/page.h> to resolve a 
> compilation error:
>     ./include/xen/domain_page.h:63:12: error: implicit declaration of 
> function '__mfn_to_virt'; did you mean 'mfn_to_nid'? 
> [-Werror=implicit-function-declaration]
>        63 |     return __mfn_to_virt(mfn_x(mfn));
>   This happens because __mfn_to_virt() is defined in <asm/mm.h>, but due to
>   the current include chain:
>     <xen/domain.h>
>       <asm/domain.h>
>         <xen/mm.h>
>           <asm/mm.h>
>             <asm/page.h>
>               <xen/domain_page.h>
>                 static inline void *map_domain_page_global(mfn_t mfn)
>               {
>                   return __mfn_to_virt(mfn_x(mfn));
>                 }
>             ...
>           ...
>           #define __mfn_to_virt() ...
>
>   This leads to a circular dependency and the build error above.
>
>   As a result, since <xen/domain_page.h> is no longer included in
>   <asm/page.h>, the flush_page_to_ram() definition cannot remain there.
>   It is now moved to riscv/mm.c.
>
> Including <asm/page.h> from <asm/mm.h> does not cause issues with the
> declaration/definition of clear_page() when <xen/mm.h> is included, and
> also prevents build errors such as:
>   common/domain.c: In function 'alloc_domain_struct':
>   common/domain.c:797:5: error: implicit declaration of function 
> 'clear_page';did you mean 'steal_page'? 
> [-Werror=implicit-function-declaration]
>   797 |     clear_page(d);
>       |     ^~~~~~~~~~
>       |     steal_page
> caused by using clear_page() in common/domain.c.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This does want further cleaning, but is a step in the right direction.

~Andrew