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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Oleksandr,

> On 19 Jan 2021, at 2:43 pm, Oleksandr <olekstysh@xxxxxxxxx> wrote:
> 
> 
> 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 am no sure but might be yes as I think if iommu is disabled we cannot 
passthrough the device.
> 
> 
>>  
>> 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.

Yes I agree with you and  I will add the code to free resources if probe failed 
and will send next version of the patch for review.

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