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

[Oleksandr <olekstysh@gmail.com>]

On 10.09.19 00:24, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


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

Yes


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

Agree. Will do.


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

Shall I add EXPERT here also?

bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs" if EXPERT = "y"


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

Agree. Will remove.


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

Actually, I can. Good point.


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

I got it. Will ignore.


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

ok


-- 
Regards,

Oleksandr Tyshchenko


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