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

[Julien Grall <julien.grall@arm.com>]

On 8/22/19 6:06 PM, Oleksandr wrote:
> 
> On 22.08.19 15:46, Julien Grall wrote:
>> Hi Oleksandr,
> 
> Hi Julien.

Hi,

>>
>>
>> On 21/08/2019 19:17, 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 maximum supported by the IOMMU
>>> Stage-2 input size must be greater or equal to the P2M IPA size.
>>>
>>> So, first initialize the IOMMU and gather the requirements and
>>> then initialize the P2M. In the P2M code, take into the account
>>> the IOMMU requirements and restrict "pa_range" if necessary.
>>
>> 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.
> 
> Indeed, will guard with #ifdef CONFIG_ARM_64.

I would rather no guard any use in the IOMMU code with CONFIG_ARM_64. 
The problem is exactly the same if Arm32.

Rather than trying to limit it, I would just check that the IOMMU are 
able to support at least 40 bits. This can be done in setup_virt_paging().

> 
> 
>>
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> Why RFC?
>>>
>>> 1. Patch assumes that IPMMU support is already in.
>>> 2. Not sure for the SMMU.
>>>
>>> If there are no objections I will craft a proper patch.
>>> ---
>>>   xen/arch/arm/p2m.c                       | 19 +++++++++++++++++--
>>>   xen/arch/arm/setup.c                     |  4 ++--
>>>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 20 +++++---------------
>>>   xen/drivers/passthrough/arm/smmu.c       | 14 +++++++-------
>>>   4 files changed, 31 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index c171568..1262ae9 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -34,7 +34,7 @@ static unsigned int __read_mostly max_vmid = 
>>> MAX_VMID_8_BIT;
>>>     #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>>>   -unsigned int __read_mostly p2m_ipa_bits;
>>> +unsigned int __read_mostly p2m_ipa_bits = 0;
>>
>> Any uninitialized global variables are 0 by default. But I think 0 
>> will not be correct here. You are assuming all the IOMMUs can support 
>> the same number of IPA bits.
> 
> Yes, this was my assumption.
> 
> 
>>
>> I am not aware if such platform exists, but this is not prevented by 
>> the SMMU specification. So it would be possible to have one SMMU 
>> supporting only 42-bits while the other would support 48-bits.
> 
> What do you think would be the proper action at the moment?

We should compute the minimum of the maximum IPA bits that any IOMMU can 
support. In the example I gave, it would be 42-bits.

> 
> 
>>
>>>     /* Helpers to lookup the properties of each level */
>>>   static const paddr_t level_masks[] =
>>> @@ -1981,10 +1981,25 @@ 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 )
>>> +    {
>>> +        /* We expect "p2m_ipa_bits" to be updated by the IOMMU 
>>> driver */
>>
>> I am not entirely happy that the IOMMU driver is updating p2m_ipa_bits 
>> directly.
>>
>> It makes things fairly unclear when the value of p2m_ipa_bits will be 
>> stable.
>>
>> I would prefer if we provide an helper that allow restricting the 
>> size. Maybe p2m_restrict_ipa_bits(...).
> 
> Makes sense. Will implement.
> 
> 
>>
>>
>> The helper can then decide how to deal with it.
> 
> OK. I am wondering, do we have cases when we shouldn't restrict the size 
> (except cases when IOMMU wants wrong p2m_ipa_bits value)?

I don't think we should check in that helper whether the p2m_ipa_bits is 
wrong. This is up to the setup_virt_paging() to decide.

In this helper, we would only compute the minimum of the maximum. My 
point here is we can then decide whether we use p2m_ipa_bits or use a 
separate variable that is not exposed outside of p2m.c.

> 
> 
> 
> 
>>
>>> +        ASSERT(p2m_ipa_bits);
>>> +
>>> +        /* We need to restrict "pa_range" according to the IOMMU 
>>> requirements */
>>> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>>> +        {
>>> +            if ( p2m_ipa_bits >= 64 - pa_range_info[i].t0sz )
>>
>> While I know the compiler will do the right things, this is slightly 
>> confusing to read. Please add 64 - pa_range_info... between parenthesis.
> 
> Will do.
> 
> 
>>
>>
>> But I think you can just check against t0sz here. Also, it feels to me 
>> a strict equality would be better. If p2m_ipa_bits is neither of the 
>> value specified here, then most likely sharing the page tables was the 
>> wrong thing to do.
> 
> I am afraid I don't completely understand why we need to check against 
> t0sz. Or probably, you meant to check against pabits?

I meant pabits. Sorry.

Since commit, 896ebdfa3a "xen/arm: p2m: configure stage-2 page table to 
support upto 42-bit PA systems", stage-2 will always be configured with 
PA bits == IPA bits. So 64 - t0sz == pabits.

We should probably have a cleanup patch to remove t0sz and making 
clearer the correlation between the two.

>>
>>
>>> +                pa_range = i;
>>> +            else
>>> +                break;
>>> +        }
>>> +    }
>>> +
>>>       for_each_online_cpu ( cpu )
>>>       {
>>>           const struct cpuinfo_arm *info = &cpu_data[cpu];
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 51a6677..01cd83d 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -936,12 +936,12 @@ void __init start_xen(unsigned long 
>>> boot_phys_offset,
>>>       printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>>>       /* TODO: smp_cpus_done(); */
>>>   -    setup_virt_paging();
>>> -
>>>       rc = iommu_setup();
>>>       if ( !iommu_enabled && rc != -ENODEV )
>>>           panic("Couldn't configure correctly all the IOMMUs.");
>>>   +    setup_virt_paging();
>>
>> Please add a comment on top of either setup_virt_paging() (or 
>> iommu_setup()) to explain the dependency between the two.
> 
> Yes, will add.
> 
> 
>>
>>> +
>>>       do_initcalls();
>>>         /*
>>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
>>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> index ec543c3..0dc6351 100644
>>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>>> @@ -526,6 +526,7 @@ static int ipmmu_domain_init_context(struct 
>>> ipmmu_vmsa_domain *domain)
>>>        * to TTBR0. Use 4KB page granule. Start page table walks at 
>>> first level.
>>>        * Always bypass stage 1 translation.
>>>        */
>>> +    ASSERT(p2m_ipa_bits <= IPMMU_MAX_P2M_IPA_BITS);
>>
>> Why this ASSERT? I know that Xen is only supporting one kind of IOMMU, 
>> but you could imagine a platform with various of them. So this may 
>> trigger when it should not.
> I didn't take into account the platform where IOMMUs could support the 
> different number of IPA bits. What do you think would be the proper 
> action at the moment?
> I could bail out with error here (and for SMMU as well) to not allow 
> creating guest domain for now...

See my suggestion above.

Cheers,

-- 
Julien Grall

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