From: Julien Grall <julien.grall@arm.com>
To: Oleksandr Tyshchenko <olekstysh@gmail.com>,
xen-devel@lists.xenproject.org
Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
sstabellini@kernel.org, Volodymyr_Babchuk@epam.com
Subject: Re: [Xen-devel] [PATCH] [RFC V2] xen/arm: Restrict "p2m_ipa_bits" according to the IOMMU requirements
Date: Tue, 10 Sep 2019 16:11:45 +0100 [thread overview]
Message-ID: <d844f8b1-380d-0aed-785c-d889050b62c5@arm.com> (raw)
In-Reply-To: <1566588892-5305-1-git-send-email-olekstysh@gmail.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
next prev parent reply other threads:[~2019-09-10 15:12 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-23 19:34 [Xen-devel] [PATCH] [RFC V2] xen/arm: Restrict "p2m_ipa_bits" according to the IOMMU requirements Oleksandr Tyshchenko
2019-09-10 15:11 ` Julien Grall [this message]
2019-09-10 16:24 ` Oleksandr
2019-09-10 18:55 ` Julien Grall
2019-09-11 16:34 ` Oleksandr
2019-09-11 16:44 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d844f8b1-380d-0aed-785c-d889050b62c5@arm.com \
--to=julien.grall@arm.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=olekstysh@gmail.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).