Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 21 Jan 2021, at 6:43 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> Please try to trim the e-mail when quoting, otherwise it is quite difficult 
> to find the only couple of answer you wrote.
> 
> On 21/01/2021 17:10, Rahul Singh wrote:
>>> On 20 Jan 2021, at 8:31 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>> wrote:
>>>> -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t 
>>>> start,
>>>> -                                resource_size_t size)
>>>> +
>>>> +static void arm_smmu_free_structures(struct arm_smmu_device *smmu)
>>>> {
>>>> -  struct resource res = {
>>>> -          .flags = IORESOURCE_MEM,
>>>> -          .start = start,
>>>> -          .end = start + size - 1,
>>>> -  };
>>>> +  if (smmu->cmdq.q.base)
>>>> +          xfree(smmu->cmdq.q.base);
>>>> +
>>>> +  if (smmu->evtq.q.base)
>>>> +          xfree(smmu->evtq.q.base);
>>>> 
>>>> -  return devm_ioremap_resource(dev, &res);
>>>> +  if (smmu->priq.q.base)
>>>> +          xfree(smmu->priq.q.base);
>>>> +
>>>> +  if (smmu->strtab_cfg.strtab)
>>>> +          xfree(smmu->strtab_cfg.strtab);
>>>> +
>>>> +  if (smmu->strtab_cfg.l1_desc)
>>>> +          xfree(smmu->strtab_cfg.l1_desc);
>>> 
>>> From what I can tell we also need to free somewhere
>>> smmu->strtab_cfg->l1_desc->l2ptr allocated by arm_smmu_init_l2_strtab
>> "l1_desc->l2ptr" is a pointer to the Level 1 Stream Table Descriptor if 
>> 2-level Stream Table supported.
>> If the device is protected by IOMMU, SMMUv3 driver will allocate the  
>> "l1_desc->l2ptr” when the device is added to the IOMMU via 
>> arm_smmu_add_device() function and device will be configured in bypass/abort 
>> mode.
>> Once we assign the device to the domain(arm_smmu_assign_dev() ) smmuv3 hw 
>> will be configured correctly to match the StreamID. If there is a failure in 
>> assigning the device, that case also XEN will not remove the device and 
>> master device still be in bypass/abort mode.
> 
> I am a bit confused with this answer. Wouldn't this mean that we are 
> "leaking" memory if we fail to assign the device?

No we are not leaking memory as "l1_desc->l2ptr” is still be valid and is 
required for correct SMMUv3 opeation if we fail to assign the device, as in 
that case device will operate in bypass or abort mode. 

For example if by any chance there is faulty hw behind SMMUv3 then if we fail 
to assign the device we can configure the device in abort mode in that case 
SMMUv3 hw will silently report abort to device, without any event recorded.

We can also configure the device in bypass mode if we fail to assign. Thats why 
I thought not to free the "l1_desc->l2ptr” if we fail to assign the device.

> 
>> As in XEN, there is no function to remove the master device from the IOMMU, 
>> because of that I feel there is no need to free the "l1_desc->l2ptr” in case 
>> of failure also.
> 
> Hmmm... Xen is able to remove device from the IOMMU. The reason this is not 
> implemented yet on Arm is because you can't hot-unplug "platform" device.
> 
> I expect the removal function to be useful for PCI.

Yes I agree for PCI hot plug devices we have to implement the IOMMU remove 
function. If we implement the remove function for PCI hot-plug we can free the 
the "l1_desc->l2ptr” in the remove function for that device. 

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall