Re: [PATCH v4 2/8] iommu/dma: Introduce generic helper to retrieve RMR info

[Robin Murphy <robin.murphy@arm.com>]

On 2021-05-19 10:30, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Joerg Roedel [mailto:joro@8bytes.org]
>> Sent: 18 May 2021 09:50
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
>> iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>;
>> lorenzo.pieralisi@arm.com; robin.murphy@arm.com; wanghuiqiang
>> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo)
>> <guohanjun@huawei.com>; steven.price@arm.com; Sami.Mujawar@arm.com;
>> jon@solid-run.com; eric.auger@redhat.com; yangyicong
>> <yangyicong@huawei.com>
>> Subject: Re: [PATCH v4 2/8] iommu/dma: Introduce generic helper to retrieve
>> RMR info
>>
>> On Thu, May 13, 2021 at 02:45:44PM +0100, Shameer Kolothum wrote:
>>> +/**
>>> + * struct iommu_rmr - Reserved Memory Region details per IOMMU
>>> + * @list: Linked list pointers to hold RMR region info
>>> + * @base_address: base address of Reserved Memory Region
>>> + * @length: length of memory region
>>> + * @sid: associated stream id
>>> + * @flags: flags that apply to the RMR node
>>> + */
>>> +struct iommu_rmr {
>>> +	struct list_head	list;
>>> +	phys_addr_t		base_address;
>>> +	u64			length;
>>> +	u32			sid;
>>> +	u32			flags;
>>> +};
>>> +
>>> +/* RMR Remap permitted */
>>> +#define IOMMU_RMR_REMAP_PERMITTED	(1 << 0)
>>> +
>>
>> This struct has lots of overlap with 'struct iommu_resv_region'. Any
>> reason the existing struct can't be used here?
>>
> 
> Hmm..main reason is "sid". RMRs are associated with stream ids and
> that is used to install bypass STEs/SMRs in SMMU drivers and also to check
> whether a dev has any RMR regions associated with it.
> 
> I think we could add sid/dev_id to 'struct iommu_resv_region', and modify
> iommu_alloc_resv_region() accordingly. That can get rid of the above struct
> and iommu_dma_alloc_rmr() fn. Not sure this will complicate things as
> the dev_id is only valid for RMR reservation region cases.
> 
> Please let me know your thoughts.

Maybe add a union for FW-specific data to struct resv_region, so that it 
could eventually subsume AMD's struct unity_map_entry and Intel's struct 
dmar_rmrr_unit as well? They're essentially doing the same dance.

We might still have to create copies of the firmware-allocated entries 
to actually assign to domains (certainly where one entry covers multiple 
devices), but kmemdup() is still a lot neater than various translations 
from private formats.

Robin.