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

[Julien Grall <julien@xxxxxxx>]

Hi Xenia,

On 14/10/2022 16:05, Xenia Ragiadakou wrote:
> 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.
I was about to say the same and note that in Xen we don't support 
52-bits. So effectively those values are ignored for now.
> IMO, it could be either
> [6] = { 52,      12/*12*/,  0,          3 } i.e single level -1 table
I don't think Xen is ready to support -1 for the root level. So...

> or > [6] = { 52,      12/*12*/,  4,          2 } i.e 16 concatenated level 0
> tables

... this would be better. But this is not related to this patch 
directly. So any fix should be sent separately.
Cheers,

--
Julien Grall