Re: [PATCH] xen/arm: p2m: fix pa_range_info for 52-bit pa range

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 10/18/22 13:56, Julien Grall wrote:

Hi Julien,

> Hi Xenia,
>
> On 18/10/2022 11:27, Xenia Ragiadakou wrote:
> > On 10/18/22 12:02, Michal Orzel wrote:
> >
> > Hi Michal,
> >
> > > Hi Xenia,
> > >
> > > On 17/10/2022 19:32, Xenia Ragiadakou wrote:
> > > > Currently the pa_range_info for the 52-bit pa range advertizes that the
> > > > p2m root table consists of 8 concatenated tables at level 3, which does
> > > > not make much sense.
> > > I think the current code advertises 8 concatenated tables at level -1 
> > > (sl0=3 -> root_level=-1)
> > > which is obviously incorrect, but the commit msg should be updated.
> > I did the same mistake in my email but I did not want to hijack the 
> > thread that 's why I did not come back to correct my error.
> > According to the manual, to support 52-bit pa range with 4KB granule 
> > with the root table at level -1, you need to set SL2=1 and SL0=0.
> > SL0=3 configures the root table at level 3.
> Which section are you reading? Looking at the definition of VTCR_EL2.SL0 
> (D17-6375, ARM DDI 0487I.a), the field has different meaning depending 
> on whether the feature TTST (Small translation table) is present.
> SL0 would be reserved when TTST is not present. That said, it looks like 
> LPA requires TTST.
I 'm referring to the table Table D8-12 "4KB granule, determining stage 
2 initial lookup level" (D8-5103, ARM DDI 0487I.a).
With 4KB granule, for having the root table at level 3, TTST is 
required, yes.
> > > Funnily enough p2m_root_level is unsigned so it would lead to overflow
> Did you mean underflow rather than overflow?
>
> > > (p2m_root_level would end up with (1 << 32) - 1 instead of -1).
> > Actually, currently, there is no support at all in XEN neither for LPA 
> > (LPA support for 4KB is not checked, VCTR DS and SL2 are not set etc) 
> > nor level -1 (the root table level is determined only based on sl0, 
> > the number of possible levels is hardcoded to 4 in many places etc). I 
> > don't think that there is even support for accessing other than the 
> > first table of concatenated root tables but I need to verify that (I 
> > assume this based on the way LPAE_TABLE_INDEX_GS is implemented).
> I am not sure I understand this. Are you saying that concatenation can 
> be used for non-root table?
No, the contrary. I cannot see how it can work out of the box given the 
current implementation. Because the mask applied to get the table index 
is limited to the size of a single table.
> > This entry is populated in the pa_range_info table just to prevent XEN 
> > from falling into this
> > if ( pa_range >= ARRAY_SIZE(pa_range_info) || 
> > !pa_range_info[pa_range].pabits )
> >          panic("Unknown encoding of ID_AA64MMFR0_EL1.PARange %x\n", 
> > pa_range);
> I think it would be worth to point out in the commit message that the 
> value is not used so far. So this is only update for correctness.
Sure.
Do I need a Fixes tag even though the previous code, effectively, was 
not breaking anything?
> Cheers,
--
Xenia