Re: [PATCH 1/2] include: don't use asm/page.h from common headers

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 02/12/2020 14:49, Jan Beulich wrote:
> Doing so limits what can be done in (in particular included by) this per-
> arch header. Abstract out page shift/size related #define-s, which is all
> the repsecitve headers care about. Extend the replacement / removal to
s/repsecitve/respective/

> some x86 headers as well; some others now need to include page.h (and
> they really should have before).
>
> Arm's VADDR_BITS gets restricted to 32-bit, as its current value is
> clearly wrong for 64-bit, but the constant also isn't used anywhere
> right now (i.e. the #define could also be dropped altogether).
Whoops. Thankfully this is not used.

> I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I
> kept it and provided a way to override the #define in the common header.
vaddr_t is defined to 32-bit for arm32 or 64-bit for arm64. So I think 
it would be fine to use the generic PAGE_OFFSET() implementation.
> Also drop the dead PAGE_FLAG_MASK altogether at this occasion.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/arm/arm64/lib/clear_page.S
> +++ b/xen/arch/arm/arm64/lib/clear_page.S
> @@ -14,6 +14,8 @@
>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <xen/page-size.h>
> +
>   /*
>    * Clear page @dest
>    *
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -176,11 +176,6 @@
>   #define FIXMAP_ACPI_BEGIN  2  /* Start mappings of ACPI tables */
>   #define FIXMAP_ACPI_END    (FIXMAP_ACPI_BEGIN + NUM_FIXMAP_ACPI_PAGES - 1)  
> /* End mappings of ACPI tables */
>   
> -#define PAGE_SHIFT              12
> -#define PAGE_SIZE           (_AC(1,L) << PAGE_SHIFT)
> -#define PAGE_MASK           (~(PAGE_SIZE-1))
> -#define PAGE_FLAG_MASK      (~0)
> -
>   #define NR_hypercalls 64
>   
>   #define STACK_ORDER 3
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -1,6 +1,7 @@
>   #ifndef __ARM_CURRENT_H__
>   #define __ARM_CURRENT_H__
>   
> +#include <xen/page-size.h>
>   #include <xen/percpu.h>
>   
>   #include <asm/processor.h>
> --- /dev/null
> +++ b/xen/include/asm-arm/page-shift.h
The name of the file looks a bit odd given that *_BITS are also defined 
in it. So how about renaming to page-size.h?
> @@ -0,0 +1,15 @@
> +#ifndef __ARM_PAGE_SHIFT_H__
> +#define __ARM_PAGE_SHIFT_H__
> +
> +#define PAGE_SHIFT              12
> +
> +#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
> +
> +#ifdef CONFIG_ARM_64
> +#define PADDR_BITS              48
Shouldn't we define VADDR_BITS here? But I wonder whether VADDR_BITS 
should be defined as sizeof(vaddr_t) * 8.
This would require to include asm/types.h.

> +#else
> +#define PADDR_BITS              40
> +#define VADDR_BITS              32
> +#endif
> +
> +#endif /* __ARM_PAGE_SHIFT_H__ */
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -2,21 +2,11 @@
>   #define __ARM_PAGE_H__
>   
>   #include <public/xen.h>
> +#include <xen/page-size.h>
>   #include <asm/processor.h>
>   #include <asm/lpae.h>
>   #include <asm/sysregs.h>
>   
> -#ifdef CONFIG_ARM_64
> -#define PADDR_BITS              48
> -#else
> -#define PADDR_BITS              40
> -#endif
> -#define PADDR_MASK              ((1ULL << PADDR_BITS)-1)
> -#define PAGE_OFFSET(ptr)        ((vaddr_t)(ptr) & ~PAGE_MASK)
> -
> -#define VADDR_BITS              32
> -#define VADDR_MASK              (~0UL)
> -
>   /* Shareability values for the LPAE entries */
>   #define LPAE_SH_NON_SHAREABLE 0x0
>   #define LPAE_SH_UNPREDICTALE  0x1
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -8,8 +8,8 @@
>   #define __X86_CURRENT_H__
>   
>   #include <xen/percpu.h>
> +#include <xen/page-size.h>
>   #include <public/xen.h>
> -#include <asm/page.h>
>   
>   /*
>    * Xen's cpu stacks are 8 pages (8-page aligned), arranged as:
> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -1,6 +1,8 @@
>   #ifndef __ARCH_DESC_H
>   #define __ARCH_DESC_H
>   
> +#include <asm/page.h>
May I ask why you are including <asm/page.h> and not <xen/page-size.h> here?

Cheers,

--
Julien Grall