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

[Oleksandr <olekstysh@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel