Re: [Xen-devel] [PATCH 3/3] xen/arm: fix mask calculation in init_pdx

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 5/7/19 8:40 AM, Jan Beulich wrote:
> > > > On 06.05.19 at 17:26, <julien.grall@xxxxxxx> wrote:
> > On 5/6/19 10:06 AM, Jan Beulich wrote:
> > > > > > On 03.05.19 at 22:50, <sstabellini@xxxxxxxxxx> wrote:
> > > > --- a/xen/arch/arm/setup.c
> > > > +++ b/xen/arch/arm/setup.c
> > > > @@ -481,10 +481,15 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
> > > >    static void __init init_pdx(void)
> > > >    {
> > > >        paddr_t bank_start, bank_size, bank_end;
> > > > -
> > > > -    u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start);
> > > > +    u64 mask;
> > > >        int bank;
> > > >    
> > > > +    /*
> > > > +     * We always map the first 1<<MAX_ORDER of RAM, hence, they are left
> > > "... pages of RAM." ?
> > >
> > > > +     * uncompressed.
> > > > +     */
> > > > +    mask = pdx_init_mask(1ULL << (MAX_ORDER + PAGE_SHIFT));
> > > PAGE_SIZE << MAX_ORDER?
> > Hmmm, I am not entirely convince this will give the correct value
> > because PAGE_SIZE is defined as (_AC(1, L) << PAGE_SHIFT.
> Oh, indeed, for an abstract 32-bit arch this would de-generate, due
> to MAX_ORDER being 20. Nevertheless I think the expression used
> invites for "cleaning up" (making the same mistake I've made), and
> since this is in Arm-specific code (where MAX_ORDER is 18) I think it
> would still be better to use the suggested alternative.
The comment on top of PAGE_SIZE in asm-x86/page.h leads to think that 
PAGE_SIZE should use signed quantities. So I am not sure whether it is 
safe to switch to UL here.
If we keep PAGE_SIZE as signed quantities, then I would rather not used 
your suggestion because this may introduce buggy code if MAX_ORDER is 
ever updated on Arm.
> > > I wonder whether pdx_init_mask() itself wouldn't better apply this
> > > (lower) cap
> > Do you mean always returning (PAGE_SIZE << MAX_ORDER) or capping the
> > init mask?
> >
> > Note that the later will not produce the same behavior as this patch.
> Since I'm not sure I understand what you mean with "capping the
> init mask", I'm also uncertain what alternative behavior you're
> thinking of. What I'm suggesting is
>
> u64 __init pdx_init_mask(u64 base_addr)
> {
>      return fill_mask(max(base_addr, (uint64_t)PAGE_SIZE << MAX_ORDER) - 1);
> }
As I pointed out in the original thread, there are a couple of issues 
with this suggestion:
	1) banks are not ordered on Arm, so the pdx mask may get initialized 
with a higher bank address preventing the PDX compression to work
	2) the PDX will not be able to compress any bits between 0 and the MSB 
1' in the base_addr. In other word, for a base address 0x200000000 
(8GB), the initial mask will be  0x1ffffffff. I am aware of some 
platforms with some interesting first bank base address (i.e above 4GB).
2) is not overly critical, but I think 1) should be addressed.

At least on Arm, I don't see any real value to initialize the PDX mask 
with a base address. I would be more keen to implement pdx_init_mask() as:
return fill_mask(((uint64_t)PAGE_SIZE << MAX_ORDER - 1);

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel