Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 10/14/22 13:22, Xenia Ragiadakou wrote:
> On 10/14/22 13:04, Julien Grall wrote:
> > Hi Xenia,
> >
> > On 14/10/2022 10:46, Xenia Ragiadakou wrote:
> > > On 10/13/22 15:39, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 13/10/2022 13:29, Henry Wang wrote:
> > > > > > -----Original Message-----
> > > > > > From: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > > > > Assuming you have 4 (N) page table levels, isn't it 7 (1 + 2 * 
> > > > > > > > (N - 1))?
> > > > > > > > Or is the root table not taken from the p2m pool?
> > > > > > > Correct, on arm the root is not taken from the pool.
> > > > > > Isn't that a (perhaps just minor) mistake?
> > > > > Not really, in the code review phase, the question of whether we 
> > > > > include
> > > > > the root in the p2m pool was discussed and the conclusion at that time
> > > > > was not including this page for now, as this is supposed to require 
> > > > > a lot
> > > > > of extra work/refactor. Probably there will be a series from my 
> > > > > side to
> > > > > add the root to the pool, but at least not now.
> > > > The root page tables can be one of multiple concatenated pages (up 
> > > > to 8 pages). The P2M pool is allocating page by page and therefore 
> > > > wouldn't allow us to allocate contiguous pages.
> > > Sorry that I 'm asking this so late (I was just going through the 
> > > thread) but why 8?
> > Rather than providing an extra level of page-tables, the architecture 
> > allows you to provide multiple pages at the root level.
> > The number of concatenated pages depend on the maximum physical 
> > address and the start level. You can look at the table in 
> > setup_virt_paging() for more details.
> I think you are referring to this:
> [6] = { 52,      12/*12*/,  3,          3 },
> Still I cannot understand why the maximum number of concatenated level 0 
> translation tables for t0sz 12 and 4KB granule is 8 and not 16?
Let me explain. Initially I got confused because according to the manual 
up to 16 translation tables can be concatenated. Then, looking at the 
code you have pointed out, I understood that you were referring to the 
maximum root order in the table below.
pa_range_info[] = {
        /* T0SZ minimum and SL0 maximum from ARM DDI 0487H.a Table D5-6 */
        /*      PA size, t0sz(min), root-order, sl0(max) */
        [0] = { 32,      32/*32*/,  0,          1 },
        [1] = { 36,      28/*28*/,  0,          1 },
        [2] = { 40,      24/*24*/,  1,          1 },
        [3] = { 42,      22/*22*/,  3,          1 },
        [4] = { 44,      20/*20*/,  0,          2 },
        [5] = { 48,      16/*16*/,  0,          2 },
        [6] = { 52,      12/*12*/,  3,          3 },
        [7] = { 0 }  /* Invalid */
    };

I think that the pa range info for the 52-bit pa range is wrong.
IMO, it could be either
[6] = { 52,      12/*12*/,  0,          3 } i.e single level -1 table
or
[6] = { 52,      12/*12*/,  4,          2 } i.e 16 concatenated level 0 
tables
Hope I am not totally out of context.

> > Cheers,
--
Xenia