RE: [PATCH v8 02/11] iommu: Introduce a union to struct iommu_resv_region

Shameerali Kolothum Thodi
2022-03-23T15:55:48Z

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 22 March 2022 18:27
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; linux-acpi@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: Linuxarm <linuxarm@huawei.com>; lorenzo.pieralisi@arm.com;
> joro@8bytes.org; will@kernel.org; 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 v8 02/11] iommu: Introduce a union to struct
> iommu_resv_region
>
> On 2022-02-21 15:43, Shameer Kolothum wrote:
> > A union is introduced to struct iommu_resv_region to hold
> > any firmware specific data. This is in preparation to add
> > support for IORT RMR reserve regions and the union now holds
> > the RMR specific information.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > ---
> > include/linux/iommu.h | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index de0c57a567c8..b06952a75f95 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -126,6 +126,11 @@ enum iommu_resv_type {
> > IOMMU_RESV_SW_MSI,
> > };
> >
> > +struct iommu_iort_rmr_data {
> > + u32 *sids; /* Stream Ids associated with IORT RMR entry */
>
> Please make this const.
>
> Further nit: capitalisation of "IDs" in the comment, otherwise I might
> worry about the possibility of Stream Egos too :P

True :). Will do that.

Thanks,
Shameer

>
> > + u32 num_sids;
> > +};
> > +
> > /**
> > * struct iommu_resv_region - descriptor for a reserved memory region
> > * @list: Linked list pointers
> > @@ -133,6 +138,7 @@ enum iommu_resv_type {
> > * @length: Length of the region in bytes
> > * @prot: IOMMU Protection flags (READ/WRITE/...)
> > * @type: Type of the reserved region
> > + * @fw_data: FW specific reserved region data
>
> Nit: we've got plenty of room to spell out "Firmware-specific", and it
> never hurts to make documentation as easy to read as possible.
>
> Thanks,
> Robin.
>
> > */
> > struct iommu_resv_region {
> > struct list_head list;
> > @@ -140,6 +146,9 @@ struct iommu_resv_region {
> > size_t length;
> > int prot;
> > enum iommu_resv_type type;
> > + union {
> > + struct iommu_iort_rmr_data rmr;
> > + } fw_data;
> > };
> >
> > /**