Re: [XEN v6 11/12] xen/arm: p2m: Use the pa_range_info table to support ARM_32 and ARM_64

[Julien Grall <julien@xxxxxxx>]

On 15/05/2023 12:49, Ayan Kumar Halder wrote:
> Hi Julien
Hi Ayan,

> On 15/05/2023 11:43, Julien Grall wrote:
> > On 15/05/2023 11:30, Ayan Kumar Halder wrote:
> > > > AFAICT, this approach would be incorrect because we wouldn't take 
> > > > into account any restriction from the SMMU susbystem (it may support 
> > > > less than what the processor support).
> > > By the restriction from SMMU subsystem, I think you mean 
> > > p2m_restrict_ipa_bits().
> > Yes.
> >
> > > As I can see, p2m_restrict_ipa_bits() gets invoked much later than 
> > > setup_virt_paging().
> > I am afraid this is not correct. If you look at setup.c, you will 
> > notice that iommu_setup() is called before setup_virt_paging(). There 
> > is a comment on top of the former call explaining the ordering.
> Yes, you are correct.
>
>
> WRT to your comment
>
>  >> You are correct that the line is not correct for Arm32. But my point 
> was more for that fact you don't update p2m_ipa_bits based on the PA 
> range selected.
> Do you mean that I should update "p2m_ipa_bits" in setup_virt_paging() [ 
> similar to what is now being done for CONFIG_ARM_64 ] ?
Yes.

> This will then override "p2m_ipa_bits"value set by p2m_restrict_ipa_bits().
>
>
> But still it does not make sense to update "p2m_ipa_bits" in 
> setup_virt_paging() for ARM_32 as
>      /* Choose suitable "pa_range" according to the resulted 
> "p2m_ipa_bits". */
>      for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>      {
>          if ( p2m_ipa_bits == pa_range_info[i].pabits )
>          {
>              pa_range = i;
> break;
> }
>      }
>
> p2m_ipa_bits will be same as pa_range_info[i].pabits.
With the current logic yes. But, in the past, we had one case where we 
used a T0SZ that would not cover the full 2^pabits (see 896ebdfa3a85). I 
can't rule out this will not be again the case in the future.
Cheers,

--
Julien Grall