Re: [Xen-devel] [PATCH] [RFC V2] xen/arm: Restrict "p2m_ipa_bits" according to the IOMMU requirements

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 9/11/19 5:34 PM, Oleksandr wrote:
> On 10.09.19 21:55, Julien Grall wrote:
> > Hi,
> Hi Julien
>
>
> > On 9/10/19 5:24 PM, Oleksandr wrote:
> > > On 10.09.19 18:11, Julien Grall wrote:
> > > > Hi Oleksandr,
> > > Hi, Julien
> > >
> > >
> > > > On 8/23/19 8:34 PM, Oleksandr Tyshchenko wrote:
> > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > >
> > > > > There is a strict requirement for the IOMMU which wants to share
> > > > > the P2M table with the CPU. The IOMMU's Stage-2 input size must be 
> > > > > equal
> > > > > to the P2M IPA size. It is not a problem when the IOMMU can support
> > > > > all values the CPU supports. In that case, the IOMMU driver would just
> > > > > use any "p2m_ipa_bits" value as is. But, there are cases when not.
> > > > >
> > > > > In order to make P2M sharing possible on the platforms which
> > > > > IPMMUs have a limitation in maximum Stage-2 input size introduce
> > > > > the following logic.
> > > > >
> > > > > First initialize the IOMMU subsystem and gather requirements regarding
> > > > > the maximum IPA bits supported by each IOMMU device to figure out
> > > > > the minimum value among them. In the P2M code, take into the account
> > > > > the IOMMU requirements and choose suitable "pa_range" according
> > > > > to the restricted "p2m_ipa_bits".
> > > > As I pointed in the previous version, all the code you modify is 
> > > > arm64 specific. For arm32, the number of IPA bits is
> > > > hardcoded. So if you modify p2m_ipa_bits, you would end up to 
> > > > misconfigure VTCR.
> > > > In other words, for Arm32, you need to check p2m_ipa_bits is at 
> > > > least 40-bits before overriding it.
> > > But, all modifications with p2m_ipa_bits are done before 
> > > setup_virt_paging(), where, actually, the p2m_ipa_bits is hard-coded 
> > > to 40 bits. How can we end up misconfiguring VTCR for ARM32? Or I 
> > > really missed something?
> > Sorry if I wasn't cleared, I meant the VTCR for the IOMMU. You would 
> > end up to configure with a value that is bigger than what it can support.
> > I am ok if you don't restrict the p2m_ipa_bits and just fail. The 
> > point is to notify the user ASAP rather than allowing to continue.
> > This would make the behavior similar to the current implementation 
> > (although the error would be different).
> So, in IOMMU driver we should check if IOMMU is able to support at least 
> 40-bit IPA before trying to restrict. If yes, then go ahead, but if no, 
> then just fail. Correct?
There are no need to do this in the IOMU drivers. And I actually don't 
such check in the drivers.
This is the similar problem to half initialized IOMMU. If the user 
requests IOMMU and doesn't work, then we don't want to continue an 
panic. Such check can be done directly in the function setup_virt_paging().
> > > > > +{
> > > > > +    /*
> > > > > +     * Calculate the minimum of the maximum IPA bits that any IOMMU
> > > > > +     * can support.
> > > > > +     */
> > > > > +    if ( iommu_ipa_bits < p2m_ipa_bits )
> > > > > +        p2m_ipa_bits = iommu_ipa_bits;
> > > > > +}
> > > > > +
> > > > >   /* VTCR value to be configured by all CPUs. Set only once by the 
> > > > > boot CPU */
> > > > >   static uint32_t __read_mostly vtcr;
> > > > >   @@ -1966,10 +1977,28 @@ void __init setup_virt_paging(void)
> > > > >           [7] = { 0 }  /* Invalid */
> > > > >       };
> > > > >   -    unsigned int cpu;
> > > > > +    unsigned int i, cpu;
> > > > >       unsigned int pa_range = 0x10; /* Larger than any possible 
> > > > > value */
> > > > >       bool vmid_8_bit = false;
> > > > >   +    if ( iommu_enabled )
> > > > Could we make this IOMMU-agnostic? The main reason to convert from 
> > > > p2m_ipa_bits to pa_range is to cater the rest of the code.
> > > > But we could rework the code to do the computation with p2m_ipa_bits 
> > > > and then look-up for the pa_range. 
> > > I am afraid, I don't completely understand your idea of making this 
> > > IOMMU-agnostic and what I should do...
> > Roughly what you are doing today is:
> >
> > if ( iommu_enabled )
> >    pa_range = find_pa_range_from_p2m_bits().
> >
> > for_each_cpu()
> >    if ( cpu.pa_range < pa_range )
> >      pa_range = cpu.pa_range
> >
> > ....
> >
> > What you could do is:
> Thank you for the clarification. I think I understand your idea.
>
> But ...
>
> > for_ech_cpu()
> >    if ( p2m_ipa_bits < pa_range_info[cpu.pa_range].pabits )
> Probably you meant ">" here?
Yes.


> > p2m_ipa_bits = pa_range_info[cpu.pa_range].pabits;
> >
> > pa_range = find_pa_range_from_p2m_bits();
> > /* Check validity */
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel