| Message ID | 20210805080724.480-2-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI/IORT: Support for IORT RMR node | expand |
On 05/08/2021 09:07, 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 | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 32d448050bf7..bd0e4641c569 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -114,6 +114,13 @@ enum iommu_resv_type { > IOMMU_RESV_SW_MSI, > }; > > +struct iommu_iort_rmr_data { > +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) > + u32 flags; > + u32 sid; /* Stream Id associated with RMR entry */ > + void *smmu; /* Associated IORT SMMU node pointer */ > +}; > + > /** > * struct iommu_resv_region - descriptor for a reserved memory region > * @list: Linked list pointers > @@ -121,6 +128,7 @@ enum iommu_resv_type { > * @length: Length of the region in bytes > * @prot: IOMMU Protection flags (READ/WRITE/...) > * @type: Type of the reserved region > + * @rmr: ACPI IORT RMR specific data NIT: This will provoke a kernel-doc warning as the field name in the structure is 'fw_data' not 'rmr' ('rmr being a field of the anonymous union). I've also retested this series on my Juno setup, so feel free to add: Tested-by: Steven Price <steven.price@arm.com> Thanks, Steve > */ > struct iommu_resv_region { > struct list_head list; > @@ -128,6 +136,9 @@ struct iommu_resv_region { > size_t length; > int prot; > enum iommu_resv_type type; > + union { > + struct iommu_iort_rmr_data rmr; > + } fw_data; > }; > > /** >
On 2021-08-05 09:07, 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 | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 32d448050bf7..bd0e4641c569 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -114,6 +114,13 @@ enum iommu_resv_type { > IOMMU_RESV_SW_MSI, > }; > > +struct iommu_iort_rmr_data { > +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) > + u32 flags; > + u32 sid; /* Stream Id associated with RMR entry */ > + void *smmu; /* Associated IORT SMMU node pointer */ > +}; Do we really need to duplicate all this data? AFAICS we could just save the acpi_iort_rmr pointer in the iommu_resv_region (with a forward declaration here if necessary) and defer parsing its actual mappings until the point where we can directly consume the results. Robin. > + > /** > * struct iommu_resv_region - descriptor for a reserved memory region > * @list: Linked list pointers > @@ -121,6 +128,7 @@ enum iommu_resv_type { > * @length: Length of the region in bytes > * @prot: IOMMU Protection flags (READ/WRITE/...) > * @type: Type of the reserved region > + * @rmr: ACPI IORT RMR specific data > */ > struct iommu_resv_region { > struct list_head list; > @@ -128,6 +136,9 @@ struct iommu_resv_region { > size_t length; > int prot; > enum iommu_resv_type type; > + union { > + struct iommu_iort_rmr_data rmr; > + } fw_data; > }; > > /** >
On Fri, Oct 8, 2021 at 2:14 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-08-05 09:07, 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 | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 32d448050bf7..bd0e4641c569 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -114,6 +114,13 @@ enum iommu_resv_type { > > IOMMU_RESV_SW_MSI, > > }; > > > > +struct iommu_iort_rmr_data { > > +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) > > + u32 flags; > > + u32 sid; /* Stream Id associated with RMR entry */ > > + void *smmu; /* Associated IORT SMMU node pointer */ > > +}; > > Do we really need to duplicate all this data? AFAICS we could just save > the acpi_iort_rmr pointer in the iommu_resv_region (with a forward > declaration here if necessary) and defer parsing its actual mappings > until the point where we can directly consume the results. From earlier discussions on this patchset, the original goal was also for device-tree mechanisms to be able to hook into this code to support similar RMR's and SMMU initialization, just not through the ACPI / IORT path. > > Robin. > > > + > > /** > > * struct iommu_resv_region - descriptor for a reserved memory region > > * @list: Linked list pointers > > @@ -121,6 +128,7 @@ enum iommu_resv_type { > > * @length: Length of the region in bytes > > * @prot: IOMMU Protection flags (READ/WRITE/...) > > * @type: Type of the reserved region > > + * @rmr: ACPI IORT RMR specific data > > */ > > struct iommu_resv_region { > > struct list_head list; > > @@ -128,6 +136,9 @@ struct iommu_resv_region { > > size_t length; > > int prot; > > enum iommu_resv_type type; > > + union { > > + struct iommu_iort_rmr_data rmr; > > + } fw_data; > > }; > > > > /** > >
> -----Original Message----- > From: Jon Nettleton [mailto:jon@solid-run.com] > Sent: 09 October 2021 07:58 > To: Robin Murphy <robin.murphy@arm.com> > Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling > List <linux-acpi@vger.kernel.org>; Linux IOMMU > <iommu@lists.linux-foundation.org>; Linuxarm <linuxarm@huawei.com>; > Steven Price <steven.price@arm.com>; Guohanjun (Hanjun Guo) > <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>; Sami > Mujawar <Sami.Mujawar@arm.com>; Will Deacon <will@kernel.org>; > wanghuiqiang <wanghuiqiang@huawei.com> > Subject: Re: [PATCH v7 1/9] iommu: Introduce a union to struct > iommu_resv_region > > On Fri, Oct 8, 2021 at 2:14 PM Robin Murphy <robin.murphy@arm.com> > wrote: > > > > On 2021-08-05 09:07, 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 | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index > > > 32d448050bf7..bd0e4641c569 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -114,6 +114,13 @@ enum iommu_resv_type { > > > IOMMU_RESV_SW_MSI, > > > }; > > > > > > +struct iommu_iort_rmr_data { > > > +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) > > > + u32 flags; > > > + u32 sid; /* Stream Id associated with RMR entry */ > > > + void *smmu; /* Associated IORT SMMU node pointer */ > > > +}; > > > > Do we really need to duplicate all this data? AFAICS we could just > > save the acpi_iort_rmr pointer in the iommu_resv_region (with a > > forward declaration here if necessary) and defer parsing its actual > > mappings until the point where we can directly consume the results. > > From earlier discussions on this patchset, the original goal was also for > device-tree mechanisms to be able to hook into this code to support similar > RMR's and SMMU initialization, just not through the ACPI / IORT path. Yes. IIRC, there were some earlier attempts to have DT support for reserved regions and there was a suggestion to provide generic interfaces so that when DT solution comes up it is easier to add the support. Thanks, Shameer > > > > Robin. > > > > > + > > > /** > > > * struct iommu_resv_region - descriptor for a reserved memory region > > > * @list: Linked list pointers > > > @@ -121,6 +128,7 @@ enum iommu_resv_type { > > > * @length: Length of the region in bytes > > > * @prot: IOMMU Protection flags (READ/WRITE/...) > > > * @type: Type of the reserved region > > > + * @rmr: ACPI IORT RMR specific data > > > */ > > > struct iommu_resv_region { > > > struct list_head list; > > > @@ -128,6 +136,9 @@ struct iommu_resv_region { > > > size_t length; > > > int prot; > > > enum iommu_resv_type type; > > > + union { > > > + struct iommu_iort_rmr_data rmr; > > > + } fw_data; > > > }; > > > > > > /** > > >
On 2021-10-11 06:47, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Jon Nettleton [mailto:jon@solid-run.com] >> Sent: 09 October 2021 07:58 >> To: Robin Murphy <robin.murphy@arm.com> >> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; >> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling >> List <linux-acpi@vger.kernel.org>; Linux IOMMU >> <iommu@lists.linux-foundation.org>; Linuxarm <linuxarm@huawei.com>; >> Steven Price <steven.price@arm.com>; Guohanjun (Hanjun Guo) >> <guohanjun@huawei.com>; yangyicong <yangyicong@huawei.com>; Sami >> Mujawar <Sami.Mujawar@arm.com>; Will Deacon <will@kernel.org>; >> wanghuiqiang <wanghuiqiang@huawei.com> >> Subject: Re: [PATCH v7 1/9] iommu: Introduce a union to struct >> iommu_resv_region >> >> On Fri, Oct 8, 2021 at 2:14 PM Robin Murphy <robin.murphy@arm.com> >> wrote: >>> >>> On 2021-08-05 09:07, 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 | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h index >>>> 32d448050bf7..bd0e4641c569 100644 >>>> --- a/include/linux/iommu.h >>>> +++ b/include/linux/iommu.h >>>> @@ -114,6 +114,13 @@ enum iommu_resv_type { >>>> IOMMU_RESV_SW_MSI, >>>> }; >>>> >>>> +struct iommu_iort_rmr_data { >>>> +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) >>>> + u32 flags; >>>> + u32 sid; /* Stream Id associated with RMR entry */ >>>> + void *smmu; /* Associated IORT SMMU node pointer */ >>>> +}; >>> >>> Do we really need to duplicate all this data? AFAICS we could just >>> save the acpi_iort_rmr pointer in the iommu_resv_region (with a >>> forward declaration here if necessary) and defer parsing its actual >>> mappings until the point where we can directly consume the results. >> >> From earlier discussions on this patchset, the original goal was also for >> device-tree mechanisms to be able to hook into this code to support similar >> RMR's and SMMU initialization, just not through the ACPI / IORT path. > > Yes. IIRC, there were some earlier attempts to have DT support for reserved regions > and there was a suggestion to provide generic interfaces so that when DT solution > comes up it is easier to add the support. OK, but in that case why is every single part of it IORT-specific in either name, description or function? Regardless, s/acpi_iort_rmr/original firmware descriptor of whatever variety/ and my comment still stands. If a firmware-specific structure is still going to exist to begin with, then what do we gain from interpreting details earlier than needed and wasting memory storing copies of them? This isn't something we're looking up hundreds of times per second and need to cache in some more efficient format. Furthermore, it seems unlikely that the eventual DT solution would end up being semantically identical to IORT RMRs, so there's every possibility that the One True Abstract Structure would need changing to work for another firmware implementation anyway. Heck, it might not even fit future IORT if it becomes permissible for multiple StreamIDs to share a single RMR descriptor. Thanks, Robin.
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 32d448050bf7..bd0e4641c569 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -114,6 +114,13 @@ enum iommu_resv_type { IOMMU_RESV_SW_MSI, }; +struct iommu_iort_rmr_data { +#define IOMMU_RMR_REMAP_PERMITTED (1 << 0) + u32 flags; + u32 sid; /* Stream Id associated with RMR entry */ + void *smmu; /* Associated IORT SMMU node pointer */ +}; + /** * struct iommu_resv_region - descriptor for a reserved memory region * @list: Linked list pointers @@ -121,6 +128,7 @@ enum iommu_resv_type { * @length: Length of the region in bytes * @prot: IOMMU Protection flags (READ/WRITE/...) * @type: Type of the reserved region + * @rmr: ACPI IORT RMR specific data */ struct iommu_resv_region { struct list_head list; @@ -128,6 +136,9 @@ struct iommu_resv_region { size_t length; int prot; enum iommu_resv_type type; + union { + struct iommu_iort_rmr_data rmr; + } fw_data; }; /**
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 | 11 +++++++++++ 1 file changed, 11 insertions(+)