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 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?


>
>
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Still RFC:
>>
>> 1. Patch assumes that IPMMU support is already in.
>> 2. Not checked for the SMMU.
>>
>> Changes since RFC V1 [1]:
>>     - Don't update p2m_ipa_bits by the IOMMU drivers directly,
>>       introduce p2m_restrict_ipa_bits()
>>     - Clarify patch subject/description
>>     - Add more comments to code
>>     - Check for equivalent "pabits" in setup_virt_paging()
>>     - Remove ASSERTs from the SMMU and IPMMU drivers
>>
>> [1] 
>> https://lists.xenproject.org/archives/html/xen-devel/2019-08/msg02078.html
>> ---
>>   xen/arch/arm/p2m.c                       | 33 
>> ++++++++++++++++++++++++++++++--
>>   xen/arch/arm/setup.c                     | 11 +++++++++--
>>   xen/drivers/passthrough/arm/ipmmu-vmsa.c | 19 ++++--------------
>>   xen/drivers/passthrough/arm/smmu.c       | 16 ++++++++--------
>>   xen/include/asm-arm/p2m.h                |  8 ++++++++
>>   5 files changed, 60 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 2374e92..f742d9c 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -34,7 +34,8 @@ 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;
>> +/* Larger than any possible value */
>
> I think it would be worth explaining that this is required so the 
> number of P2M bits can be restricted by external entity (e.g IOMMU).

ok, will add explanation


>
>
>> +unsigned int __read_mostly p2m_ipa_bits = 64;
>>     /* Helpers to lookup the properties of each level */
>>   static const paddr_t level_masks[] =
>> @@ -1912,6 +1913,16 @@ struct page_info *get_page_from_gva(struct 
>> vcpu *v, vaddr_t va,
>>       return page;
>>   }
>>   +void __init p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits)
>
> The name of the function is quite generic as most of the code in it. 
> So can we avoid use the term IOMMU in it?

yes, will do


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


>> +    {
>> +        /*
>> +         * Choose suitable "pa_range" according to the IOMMU 
>> requirements
>> +         * (restricted "p2m_ipa_bits" value).
>> +         * As P2M table is always configured with IPA bits == PA bits,
>> +         * check for equivalent "pabits" and store it's index.
>> +         */
>> +        for ( i = 0; i < ARRAY_SIZE(pa_range_info); i++ )
>> +        {
>> +            if ( p2m_ipa_bits == pa_range_info[i].pabits )
>> +            {
>> +                pa_range = i;
>> +                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..413f3e6 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -936,12 +936,19 @@ 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();
>> -
>> +    /*
>> +     * The IOMMU subsystem must be initialized before P2M as we need 
>> to gather
>> +     * requirements regarding the maximum IPA bits supported by each 
>> IOMMU
>> +     * device to figure out the minimum value among them. The P2M 
>> code will
>> +     * choose suitable "pa_range" according to the restricted 
>> "p2m_ipa_bits"
>> +     * value.
>> +     */
>
> This is a bit too verbose, implementation details are not necessary 
> here and increasing the risk to have bit rot comment. So how about:
>
> "The IOMMU subsystem must be initialized before the P2M as we need to 
> gather requirements regarding the maximum IPA bit supported by each 
> IOMMU."?

you are right, will update a comment


>
>
>>       rc = iommu_setup();
>>       if ( !iommu_enabled && rc != -ENODEV )
>>           panic("Couldn't configure correctly all the IOMMUs.");
>>   +    setup_virt_paging();
>> +
>>       do_initcalls();
>>         /*
>> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
>> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> index ec543c3..d2e36a4 100644
>> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
>> @@ -1314,23 +1314,12 @@ static __init int ipmmu_init(struct 
>> dt_device_node *node, const void *data)
>>           return -ENODEV;
>>       }
>>       else
>> -    {
>>           /*
>> -         * As 4-level translation table is not supported in IPMMU, 
>> we need
>> -         * to check IPA size used for P2M table beforehand to be 
>> sure it is
>> -         * 3-level and the IPMMU will be able to use it.
>> -         *
>> -         * TODO: 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.
>> +         * Set maximum Stage-2 input size supported by the IPMMU. We 
>> expect
>> +         * the P2M code will take into the account the IOMMU 
>> requirements and
>> +         * choose suitable "pa_range".
>
> I would drop the last sentence.

ok. will do


>
>
>>            */
>> -        if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
>> -        {
>> -            printk_once(XENLOG_ERR "ipmmu: P2M IPA size is not 
>> supported (P2M=%u IPMMU=%u)!\n",
>> -                        p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
>> -            return -ENODEV;
>> -        }
>> -    }
>> +        p2m_restrict_ipa_bits(IPMMU_MAX_P2M_IPA_BITS);
>>         ret = ipmmu_probe(node);
>>       if ( ret )
>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>> b/xen/drivers/passthrough/arm/smmu.c
>> index 8ae986a..2b10c6e 100644
>> --- a/xen/drivers/passthrough/arm/smmu.c
>> +++ b/xen/drivers/passthrough/arm/smmu.c
>> @@ -2198,14 +2198,14 @@ static int arm_smmu_device_cfg_probe(struct 
>> arm_smmu_device *smmu)
>>       size = arm_smmu_id_size_to_bits((id >> ID2_IAS_SHIFT) & 
>> ID2_IAS_MASK);
>>       smmu->s1_output_size = min_t(unsigned long, PHYS_MASK_SHIFT, 
>> size);
>>   -    /* Xen: Stage-2 input size has to match p2m_ipa_bits.  */
>> -    if (size < p2m_ipa_bits) {
>> -        dev_err(smmu->dev,
>> -            "P2M IPA size not supported (P2M=%u SMMU=%lu)!\n",
>> -            p2m_ipa_bits, size);
>> -        return -ENODEV;
>> -    }
>> -    smmu->s2_input_size = p2m_ipa_bits;
>> +    /*
>> +     * Xen:
>> +     * Set maximum Stage-2 input size supported by the SMMU. We expect
>> +     * the P2M code will take into the account the IOMMU 
>> requirements and
>> +     * choose suitable "pa_range".
>
> Same here.

ok


-- 
Regards,

Oleksandr Tyshchenko


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