Re: [Xen-devel] [PATCH V3 8/8] iommu/arm: Add Renesas IPMMU-VMSA support

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

Hi Oleksandr,

On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
> diff --git a/xen/arch/arm/platforms/Kconfig b/xen/arch/arm/platforms/Kconfig
> index bc0e9cd..c93a6b2 100644
> --- a/xen/arch/arm/platforms/Kconfig
> +++ b/xen/arch/arm/platforms/Kconfig
> @@ -25,6 +25,7 @@ config RCAR3
>   	bool "Renesas RCar3 support"
>   	depends on ARM_64
>   	select HAS_SCIF
> +	select IPMMU_VMSA

As discussed previously, I think the IPMMU driver should be merged as 
tech preview for a couple of release to allow more users to test before 
we mark it as supported.

Based on this, I would not advise to select IPMMU_VMSA by default as 
user may not want to use tech preview code by default. Instead I would 
only select if EXPERT is set.

>   	---help---
>   	Enable all the required drivers for Renesas RCar3
>   
> diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig
> index a3c0649..47eadb4 100644
> --- a/xen/drivers/passthrough/Kconfig
> +++ b/xen/drivers/passthrough/Kconfig
> @@ -12,4 +12,17 @@ config ARM_SMMU
>   
>   	  Say Y here if your SoC includes an IOMMU device implementing the
>   	  ARM SMMU architecture.
> +
> +config IPMMU_VMSA
> +	bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs"
> +	default n
> +	depends on ARM_64
> +	---help---
> +	  Support for implementations of the Renesas IPMMU-VMSA found
> +	  in R-Car Gen3 SoCs.
> +
> +	  Say Y here if you are using newest R-Car Gen3 SoCs revisions
> +	  (H3 ES3.0, M3-W+, etc) which IPMMU hardware supports stage 2
> +	  translation table format and is able to use CPU's P2M table as is.
> +
>   endif

[...]

> +    /* Wait until the Root device has been registered for sure. */
> +    if ( !mmu->root )
> +    {
> +        dev_err(&node->dev, "Root IPMMU hasn't been registered yet\n");

This is a bit odd to throw an error if we are going to defer the probe.

> +        ret = -EAGAIN;
> +        goto out;
> +    }

[...]

> +static __init bool ipmmu_stage2_supported(void)
> +{
> +    struct dt_device_node *np;
> +    uint64_t addr, size;
> +    void __iomem *base;
> +    uint32_t product, cut;
> +    static enum
> +    {
> +        UNKNOWN,
> +        SUPPORTED,
> +        NOTSUPPORTED
> +    } stage2_supported = UNKNOWN;
> +
> +    /* Use the flag to avoid checking for the compatibility more then once. */

There are only one IOMMU root that will always be initialized first. So 
can't you move this code in the root IOMMU path?

> +    switch ( stage2_supported )
> +    {
> +    case SUPPORTED:
> +        return true;
> +
> +    case NOTSUPPORTED:
> +        return false;
> +
> +    case UNKNOWN:
> +    default:
> +        stage2_supported = NOTSUPPORTED;
> +        break;
> +    }
> +
> +    np = dt_find_compatible_node(NULL, NULL, "renesas,prr");
> +    if ( !np )
> +    {
> +        printk(XENLOG_ERR "ipmmu: Failed to find PRR node\n");
> +        return false;
> +    }
> +
> +    if ( dt_device_get_address(np, 0, &addr, &size) )
> +    {
> +        printk(XENLOG_ERR "ipmmu: Failed to get PRR MMIO\n");
> +        return false;
> +    }
> +
> +    base = ioremap_nocache(addr, size);
> +    if ( !base )
> +    {
> +        printk(XENLOG_ERR "ipmmu: Failed to ioremap PRR MMIO\n");
> +        return false;
> +    }
> +
> +    product = readl(base);
> +    cut = product & RCAR_CUT_MASK;
> +    product &= RCAR_PRODUCT_MASK;
> +
> +    switch ( product )
> +    {
> +    case RCAR_PRODUCT_H3:
> +    case RCAR_PRODUCT_M3W:
> +        if ( cut >= RCAR_CUT_VER30 )
> +            stage2_supported = SUPPORTED;
> +        break;
> +
> +    case RCAR_PRODUCT_M3N:
> +        stage2_supported = SUPPORTED;
> +        break;
> +
> +    default:
> +        printk(XENLOG_ERR "ipmmu: Unsupported SoC version\n");
> +        break;
> +    }
> +
> +    iounmap(base);
> +
> +    return stage2_supported == SUPPORTED;
> +}
> +
> +static const struct dt_device_match ipmmu_dt_match[] __initconst =
> +{
> +    DT_MATCH_COMPATIBLE("renesas,ipmmu-r8a7795"),
> +    DT_MATCH_COMPATIBLE("renesas,ipmmu-r8a77965"),
> +    DT_MATCH_COMPATIBLE("renesas,ipmmu-r8a7796"),
> +    { /* sentinel */ },
> +};
> +
> +static __init int ipmmu_init(struct dt_device_node *node, const void *data)
> +{
> +    int ret;
> +
> +    /*
> +     * Even if the device can't be initialized, we don't want to give
> +     * the IPMMU device to dom0.
> +     */
> +    dt_device_set_used_by(node, DOMID_XEN);
> +
> +    if ( !iommu_hap_pt_share )

iommu_hap_pt_share will soon be hardcoded to true on Arm (see [1]). Even 
without the patch, the value of iommu_hap_pt_share should be ignored as 
done by the SMMU.

> +    {
> +        printk_once(XENLOG_ERR "ipmmu: P2M table must always be shared between the CPU and the IPMMU\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( !ipmmu_stage2_supported() )
> +    {
> +        printk_once(XENLOG_ERR "ipmmu: P2M sharing is not supported in current SoC revision\n");
> +        return -ENODEV;

The if part is returning so...

> +    }
> +    else

... the else part is not necessary removing one layer of indentation.

> +    {
> +        /*
> +         * 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.
> +         */
> +        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;
> +        }
> +    }
> +
> +    ret = ipmmu_probe(node);
> +    if ( ret )
> +    {
> +        dev_err(&node->dev, "Failed to init IPMMU (%d)\n", ret);
> +        return ret;
> +    }
> +
> +    iommu_set_ops(&ipmmu_iommu_ops);
> +
> +    return 0;
> +}
> +
> +DT_DEVICE_START(ipmmu, "Renesas IPMMU-VMSA", DEVICE_IOMMU)
> +    .dt_match = ipmmu_dt_match,
> +    .init = ipmmu_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

[1] <20190902145014.36442-6-paul.durrant@citrix.com>

-- 
Julien Grall

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