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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 03/12/2020 09:27, Jan Beulich wrote:
> 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.
I would be happy with 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)
I would go with 2).

> > 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.
Fair point!

> > > --- 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).
Thanks for the explanation!


> Jan
Cheers,

--
Julien Grall