Hello Oleksandr,
> On 19 Jan 2021, at 2:43 pm, Oleksandr <olekst...@gmail.com> wrote:
>
>
> On 18.01.21 18:57, Rahul Singh wrote:
>> Hello Oleksandr,
>
> Hi Rahul
>
>
>>
>>> On 18 Jan 2021, at 4:20 pm, Oleksandr <olekst...@gmail.com> wrote:
>>>
>>>
>>> On 18.01.21 17:33, Rahul Singh wrote:
>>>> Hello Oleksandr,
>>>>
>>>>> On 11 Jan 2021, at 4:39 pm, Oleksandr <olekst...@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)
>> 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
