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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.12.2020 18:14, Julien Grall wrote:
> 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.

Will switch.

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

I was initially meaning to use that name, but these headers
specifically don't define any sizes - *_BITS are still shift
values, at least in a way. If the current name isn't liked, my
next best suggestion would then be page-bits.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?

See the description - it's unused anyway. I'm fine any of the three
possible ways:
1) keep as is in v1
2) drop altogether
3) also #define for 64-bit (but then you need to tell me whether 64
   is the right value to use, or what the correct one would be)

> But I wonder whether VADDR_BITS 
> should be defined as sizeof(vaddr_t) * 8.
> 
> This would require to include asm/types.h.

Which I'd specifically like to avoid. Plus use of sizeof() also
precludes the use of respective #define-s in #if-s.

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

Because of

DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e);

and

DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);

at least (didn't check further).

Jan