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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]

On Thu, 21 Jan 2021, Rahul Singh wrote:
> > On 21 Jan 2021, at 6:43 pm, Julien Grall <julien@xen.org> 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?
> 
> 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.

Basically there are two places where l2ptr should be freed:

1) the error out path (err_free_master) in arm_smmu_add_device
2) arm_smmu_remove_device

However, arm_smmu_remove_device is not actually implementedi at all. In
regards to 1), it would be redundant because the memory allocation of
l2ptr is the last operation that could return an error in
arm_smmu_add_device. In other words, if it fails then the memory is not
allocated.

To avoid future memory-leaks in case we add function calls that could
fail after the arm_smmu_init_l2_strtab() call in arm_smmu_add_device, I
think it would be good to have an xfree(l2ptr) under err_free_master.
But given that it is not necessary today I am OK either way.