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

[Julien Grall <julien@xen.org>]

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@kernel.org> 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?

> 
> 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.

Cheers,

-- 
Julien Grall