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

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

Hi Oleksandr,

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.

> 
> 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).

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

> +{
> +    /*
> +     * 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. In all honesty, I think we can completely 
avoid pa_range but this is probably going to require more a bit more work.

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

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

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

> +	 */
> +	p2m_restrict_ipa_bits(size);
> +	smmu->s2_input_size = size;
>   #if 0
>   	/* Stage-2 input size limited due to pgd allocation (PTRS_PER_PGD) */
>   #ifdef CONFIG_64BIT
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index f970c53..cdcf83a 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -165,6 +165,14 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>       /* Not supported on ARM. */
>   }
>   
> +/*
> + * Helper to restrict "p2m_ipa_bits" according the IOMMU requirements.
> + *
> + * Each IOMMU driver should report the maximum IPA bits (Stage-2 input size)
> + * it can support.
> + */
> +void p2m_restrict_ipa_bits(unsigned int iommu_ipa_bits);
> +
>   /* Second stage paging setup, to be called on all CPUs */
>   void setup_virt_paging(void);
>   
> 

Cheers,

-- 
Julien Grall

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