| Message ID | 20221017173209.236781-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: p2m: fix pa_range_info for 52-bit pa range | expand |
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. Funnily enough p2m_root_level is unsigned so it would lead to overflow (p2m_root_level would end up with (1 << 32) - 1 instead of -1). > In order to support the 52-bit pa size with 4KB granule, the p2m root > table needs to be configured either as a single table at level -1 or > as 16 concatenated tables at level 0. > Since, currently there is not support for level -1, set the > root_order and sl0 fields of the corresponding pa_range_info according > to the second approach. > > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
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. > Funnily enough p2m_root_level is unsigned so it would lead to 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). 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); > >> In order to support the 52-bit pa size with 4KB granule, the p2m root >> table needs to be configured either as a single table at level -1 or >> as 16 concatenated tables at level 0. >> Since, currently there is not support for level -1, set the >> root_order and sl0 fields of the corresponding pa_range_info according >> to the second approach. >> >> Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> > > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > ~Michal >
Hi Xenia, On 17/10/2022 18: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. > In order to support the 52-bit pa size with 4KB granule, the p2m root > table needs to be configured either as a single table at level -1 or > as 16 concatenated tables at level 0. > Since, currently there is not support for level -1, set the > root_order and sl0 fields of the corresponding pa_range_info according > to the second approach. > Please add a Fixes tag. Also, it would be good to provide a reference to the Arm Arm (paragraph + version) so it is easier to find where your values come from. Cheers, > Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> > --- > xen/arch/arm/p2m.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index f17500ddf3..c824d62806 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -2251,7 +2251,7 @@ void __init setup_virt_paging(void) > [3] = { 42, 22/*22*/, 3, 1 }, > [4] = { 44, 20/*20*/, 0, 2 }, > [5] = { 48, 16/*16*/, 0, 2 }, > - [6] = { 52, 12/*12*/, 3, 3 }, > + [6] = { 52, 12/*12*/, 4, 2 }, > [7] = { 0 } /* Invalid */ > }; >
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. > >> 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? > > 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. Cheers,
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, >
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index f17500ddf3..c824d62806 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2251,7 +2251,7 @@ void __init setup_virt_paging(void) [3] = { 42, 22/*22*/, 3, 1 }, [4] = { 44, 20/*20*/, 0, 2 }, [5] = { 48, 16/*16*/, 0, 2 }, - [6] = { 52, 12/*12*/, 3, 3 }, + [6] = { 52, 12/*12*/, 4, 2 }, [7] = { 0 } /* Invalid */ };
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. In order to support the 52-bit pa size with 4KB granule, the p2m root table needs to be configured either as a single table at level -1 or as 16 concatenated tables at level 0. Since, currently there is not support for level -1, set the root_order and sl0 fields of the corresponding pa_range_info according to the second approach. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- xen/arch/arm/p2m.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)