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

[Oleksandr <olekstysh@gmail.com>]

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@epam.com>
>>>>
>>>> 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?


>>>> +{
>>>> +    /*
>>>> +     * 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?


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


-- 
Regards,

Oleksandr Tyshchenko


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