Re: [PATCH v4 11/11] xen/arm: smmuv3: Add support for SMMUv3 driver

[Oleksandr <olekstysh@xxxxxxxxx>]

On 18.01.21 18:57, Rahul Singh wrote:
> Hello Oleksandr,
Hi Rahul


> > On 18 Jan 2021, at 4:20 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
> >
> >
> > On 18.01.21 17:33, Rahul Singh wrote:
> > > Hello Oleksandr,
> > >
> > > > On 11 Jan 2021, at 4:39 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
> > > >
> > > >
> > > > Hi Rahul
> > Hi Rahul
> >
> >
> > > > > > -
> > > > > >    static int arm_smmu_device_probe(struct platform_device *pdev)
> > > > > >    {
> > > > > >        int irq, ret;
> > > > > > -    struct resource *res;
> > > > > > -    resource_size_t ioaddr;
> > > > > > +    paddr_t ioaddr, iosize;
> > > > > >        struct arm_smmu_device *smmu;
> > > > > > -    struct device *dev = &pdev->dev;
> > > > > > -    bool bypass;
> > > > > >    -    smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> > > > > > +    smmu = xzalloc(struct arm_smmu_device);
> > > > > >        if (!smmu) {
> > > > > > -        dev_err(dev, "failed to allocate arm_smmu_device\n");
> > > > > > +        dev_err(pdev, "failed to allocate arm_smmu_device\n");
> > > > > >            return -ENOMEM;
> > > > > >        }
> > > > > > -    smmu->dev = dev;
> > > > > > +    smmu->dev = pdev;
> > > > > >    -    if (dev->of_node) {
> > > > > > +    if (pdev->of_node) {
> > > > > >            ret = arm_smmu_device_dt_probe(pdev, smmu);
> > > > > > +        if (ret)
> > > > > > +            return -EINVAL;
> > > > > >        } else {
> > > > > >            ret = arm_smmu_device_acpi_probe(pdev, smmu);
> > > > > >            if (ret == -ENODEV)
> > > > > >                return ret;
> > > > > >        }
> > > > > >    -    /* Set bypass mode according to firmware probing result */
> > > > > > -    bypass = !!ret;
> > > > > > -
> > > > > >        /* Base address */
> > > > > > -    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > > > -    if (resource_size(res) < arm_smmu_resource_size(smmu)) {
> > > > > > -        dev_err(dev, "MMIO region too small (%pr)\n", res);
> > > > > > +    ret = dt_device_get_address(dev_to_dt(pdev), 0, &ioaddr, &iosize);
> > > > > > +    if (ret)
> > > > > > +        return -ENODEV;
> > > > > > +
> > > > > > +    if (iosize < arm_smmu_resource_size(smmu)) {
> > > > > > +        dev_err(pdev, "MMIO region too small (%lx)\n", iosize);
> > > > > >            return -EINVAL;
> > > > > >        }
> > > > > > -    ioaddr = res->start;
> > > > > >          /*
> > > > > >         * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
> > > > > > -     * the PMCG registers which are reserved by the PMU driver.
> > > > > > +     * the PMCG registers which are optional and currently not supported.
> > > > > >         */
> > > > > > -    smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
> > > > > > +    smmu->base = ioremap_nocache(ioaddr, ARM_SMMU_REG_SZ);
> > > > > >        if (IS_ERR(smmu->base))
> > > > > >            return PTR_ERR(smmu->base);
> > > > > >    -    if (arm_smmu_resource_size(smmu) > SZ_64K) {
> > > > > > -        smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
> > > > > > +    if (iosize > SZ_64K) {
> > > > > > +        smmu->page1 = ioremap_nocache(ioaddr + SZ_64K,
> > > > > >                               ARM_SMMU_REG_SZ);
> > > > > >            if (IS_ERR(smmu->page1))
> > > > > >                return PTR_ERR(smmu->page1);
> > > > > > @@ -2765,14 +3101,262 @@ static int arm_smmu_device_probe(struct 
> > > > > > platform_device *pdev)
> > > > > >            return ret;
> > > > > >          /* Reset the device */
> > > > > > -    ret = arm_smmu_device_reset(smmu, bypass);
> > > > > > +    ret = arm_smmu_device_reset(smmu);
> > > > > >        if (ret)
> > > > > >            return ret;
> > > > > >    +    /*
> > > > > > +     * Keep a list of all probed devices. This will be used to query
> > > > > > +     * the smmu devices based on the fwnode.
> > > > > > +     */
> > > > > > +    INIT_LIST_HEAD(&smmu->devices);
> > > > > > +
> > > > > > +    spin_lock(&arm_smmu_devices_lock);
> > > > > > +    list_add(&smmu->devices, &arm_smmu_devices);
> > > > > > +    spin_unlock(&arm_smmu_devices_lock);
> > > > Looks like that we need some kind of manual roll-back logic here in case of 
> > > > error during probe (there is no real devm_*):
> > > >
> > > > iounmap, xfree, etc.
> > > I agree with you that manual roll-back logic is good to have clean code but in 
> > > this scenario what I have found out that if there is an error during probe 
> > > arm_smmu_device_probe() will return and XEN will not continue to boot (call 
> > > panic function) , in that case if we free the memory also there is no much 
> > > difference. That why I decided not to modify the code that we ported from Linux.
> > >
> > > XEN) I/O virtualisation disabled
> > > (XEN)
> > > (XEN) ****************************************
> > > (XEN) Panic on CPU 0:
> > > (XEN) Couldn't configure correctly all the IOMMUs.
> > > (XEN) ****************************************
> > > (XEN)
> > > (XEN) Manual reset required ('noreboot' specified)
> > >
> > > Do we have a requirement to continue to boot the XEN if there is an IOMMU 
> > > available in the system and IOMMU probe is failed? If yes then I will modify 
> > > the code to free all the resources if there is error during probe.
> > Xen won't call panic if IOMMU driver returns -ENODEV and will continue to boot. 
> > For example, if the IOMMU is present but cannot be used in Xen for some reason 
> > (doesn't support page table sharing, etc)
> Yes you are right in case of IOMMU driver probe failed and return -ENODEV XEN 
> will continue to boot.
>
> I am thinking of if there is a problem with configuring the IOMMU HW and return 
> -ENODEV or  for some reason if IOMMU is present cannot not be used in XEN why 
> we are silently allows XEN to boot and make the system insecure.
> As end user might miss the error logs during boot and will think IOMMU is 
> enabled and system is secure but IOMMU is either disable or is working in 
> bypass mode.
But, wouldn't end user notice that device passthrough is not functional 
then?

>   
>
> I might be wrong, in that case as per my understanding we should return error and call 
> panic and let user decide either to fix the issue on next boot or boot XEN with cmdline 
> option "iommu=no”
I got your point, but I am not sure I can answer precisely how Xen 
should behave in the situation above, I will let the maintainers comment 
on that. Just a note, the -ENODEV is also returned by the framework if 
the IOMMU is not present (please see iommu_hardware_setup() in 
drivers/passthrough/arm/iommu.c for the details), either Xen doesn't 
have a suitable driver for it or the IOMMU H/W is not available in the 
target SoC, etc. I am not quite sure we should call panic in such cases.

Regarding the cleanup my point is that driver should be responsible of 
doing it if there is an error during initialization (and it cannot 
continue) regardless on how the common code would handle that (returned 
by driver) error. Now it panics on some conditions, tomorrow it will act 
differently, etc. If driver called panic by itself, it could _probably_ 
be in a position to leave resources unreleased then... This is my 
viewpoint which might be wrong.

> Regards,
> Rahul
>
> > --
> > Regards,
> >
> > Oleksandr Tyshchenko
--
Regards,

Oleksandr Tyshchenko