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

[Oleksandr <olekstysh@gmail.com>]

On 18.01.21 17:33, Rahul Singh wrote:
> Hello Oleksandr,
>
>> On 11 Jan 2021, at 4:39 pm, Oleksandr <olekstysh@gmail.com> 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)


-- 
Regards,

Oleksandr Tyshchenko