RE: [PATCH RFC 08/12] iommufd: IOCTLs for the io_pagetable
> From: David Gibson
> Sent: Thursday, March 31, 2022 12:36 PM
> > +
> > +/**
> > + * struct iommu_ioas_iova_ranges - ioctl(IOMMU_IOAS_IOVA_RANGES)
> > + * @size: sizeof(struct iommu_ioas_iova_ranges)
> > + * @ioas_id: IOAS ID to read ranges from
> > + * @out_num_iovas: Output total number of ranges in the IOAS
> > + * @__reserved: Must be 0
> > + * @out_valid_iovas: Array of valid IOVA ranges. The array length is the
> smaller
> > + * of out_num_iovas or the length implied by size.
> > + * @out_valid_iovas.start: First IOVA in the allowed range
> > + * @out_valid_iovas.last: Inclusive last IOVA in the allowed range
> > + *
> > + * Query an IOAS for ranges of allowed IOVAs. Operation outside these
> ranges is
> > + * not allowed. out_num_iovas will be set to the total number of iovas
> > + * and the out_valid_iovas[] will be filled in as space permits.
> > + * size should include the allocated flex array.
> > + */
> > +struct iommu_ioas_iova_ranges {
> > + __u32 size;
> > + __u32 ioas_id;
> > + __u32 out_num_iovas;
> > + __u32 __reserved;
> > + struct iommu_valid_iovas {
> > + __aligned_u64 start;
> > + __aligned_u64 last;
> > + } out_valid_iovas[];
> > +};
> > +#define IOMMU_IOAS_IOVA_RANGES _IO(IOMMUFD_TYPE,
> IOMMUFD_CMD_IOAS_IOVA_RANGES)
>
> Is the information returned by this valid for the lifeime of the IOAS,
> or can it change? If it can change, what events can change it?
>
It can change when a new device is attached to an ioas.
You can look at iopt_table_enforce_group_resv_regions() in patch7
which is called by iommufd_device_attach() in patch10. That function
will first check whether new reserved ranges from the attached device
have been used and if no conflict then add them to the list of reserved
ranges of this ioas.
Userspace can call this ioctl to retrieve updated IOVA range info after
attaching a device.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 08/12] iommufd: IOCTLs for the io_pagetable
On Fri, Mar 18, 2022 at 02:27:33PM -0300, Jason Gunthorpe wrote:
> Connect the IOAS to its IOCTL interface. This exposes most of the
> functionality in the io_pagetable to userspace.
>
> This is intended to be the core of the generic interface that IOMMUFD will
> provide. Every IOMMU driver should be able to implement an iommu_domain
> that is compatible with this generic mechanism.
>
> It is also designed to be easy to use for simple non virtual machine
> monitor users, like DPDK:
> - Universal simple support for all IOMMUs (no PPC special path)
> - An IOVA allocator that considerds the aperture and the reserved ranges
> - io_pagetable allows any number of iommu_domains to be connected to the
>IOAS
>
> Along with room in the design to add non-generic features to cater to
> specific HW functionality.
[snip]
> +/**
> + * struct iommu_ioas_alloc - ioctl(IOMMU_IOAS_ALLOC)
> + * @size: sizeof(struct iommu_ioas_alloc)
> + * @flags: Must be 0
> + * @out_ioas_id: Output IOAS ID for the allocated object
> + *
> + * Allocate an IO Address Space (IOAS) which holds an IO Virtual Address
> (IOVA)
> + * to memory mapping.
> + */
> +struct iommu_ioas_alloc {
> + __u32 size;
> + __u32 flags;
> + __u32 out_ioas_id;
> +};
> +#define IOMMU_IOAS_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_IOAS_ALLOC)
> +
> +/**
> + * struct iommu_ioas_iova_ranges - ioctl(IOMMU_IOAS_IOVA_RANGES)
> + * @size: sizeof(struct iommu_ioas_iova_ranges)
> + * @ioas_id: IOAS ID to read ranges from
> + * @out_num_iovas: Output total number of ranges in the IOAS
> + * @__reserved: Must be 0
> + * @out_valid_iovas: Array of valid IOVA ranges. The array length is the
> smaller
> + * of out_num_iovas or the length implied by size.
> + * @out_valid_iovas.start: First IOVA in the allowed range
> + * @out_valid_iovas.last: Inclusive last IOVA in the allowed range
> + *
> + * Query an IOAS for ranges of allowed IOVAs. Operation outside these ranges
> is
> + * not allowed. out_num_iovas will be set to the total number of iovas
> + * and the out_valid_iovas[] will be filled in as space permits.
> + * size should include the allocated flex array.
> + */
> +struct iommu_ioas_iova_ranges {
> + __u32 size;
> + __u32 ioas_id;
> + __u32 out_num_iovas;
> + __u32 __reserved;
> + struct iommu_valid_iovas {
> + __aligned_u64 start;
> + __aligned_u64 last;
> + } out_valid_iovas[];
> +};
> +#define IOMMU_IOAS_IOVA_RANGES _IO(IOMMUFD_TYPE,
> IOMMUFD_CMD_IOAS_IOVA_RANGES)
Is the information returned by this valid for the lifeime of the IOAS,
or can it change? If it can change, what events can change it?
If it *can't* change, then how do we have enough information to
determine this at ALLOC time, since we don't necessarily know which
(if any) hardware IOMMU will be attached to it.
> +/**
> + * enum iommufd_ioas_map_flags - Flags for map and copy
> + * @IOMMU_IOAS_MAP_FIXED_IOVA: If clear the kernel will compute an
> appropriate
> + * IOVA to place the mapping at
> + * @IOMMU_IOAS_MAP_WRITEABLE: DMA is allowed to write to this mapping
> + * @IOMMU_IOAS_MAP_READABLE: DMA is allowed to read from this mapping
> + */
> +enum iommufd_ioas_map_flags {
> + IOMMU_IOAS_MAP_FIXED_IOVA = 1 << 0,
> + IOMMU_IOAS_MAP_WRITEABLE = 1 << 1,
> + IOMMU_IOAS_MAP_READABLE = 1 << 2,
> +};
> +
> +/**
> + * struct iommu_ioas_map - ioctl(IOMMU_IOAS_MAP)
> + * @size: sizeof(struct iommu_ioas_map)
> + * @flags: Combination of enum iommufd_ioas_map_flags
> + * @ioas_id: IOAS ID to change the mapping of
> + * @__reserved: Must be 0
> + * @user_va: Userspace pointer to start mapping from
> + * @length: Number of bytes to map
> + * @iova: IOVA the mapping was placed at. If IOMMU_IOAS_MAP_FIXED_IOVA is set
> + *then this must be provided as input.
> + *
> + * Set an IOVA mapping from a user pointer. If FIXED_IOVA is specified then
> the
> + * mapping will be established at iova, otherwise a suitable location will be
> + * automatically selected and returned in iova.
> + */
> +struct iommu_ioas_map {
> + __u32 size;
> + __u32 flags;
> + __u32 ioas_id;
> + __u32 __reserved;
> + __aligned_u64 user_va;
> + __aligned_u64 length;
> + __aligned_u64 iova;
> +};
> +#define IOMMU_IOAS_MAP _IO(IOMMUFD_TYPE, IOMMUFD_CMD_IOAS_MAP)
> +
> +/**
> + * struct iommu_ioas_copy - ioctl(IOMMU_IOAS_COPY)
> + * @size: sizeof(struct iommu_ioas_copy)
> + * @flags: Combination of enum iommufd_ioas_map_flags
> + * @dst_ioas_id: IOAS ID to change the mapping of
> + * @src_ioas_id: IOAS ID to copy from
> + * @length: Number of bytes to copy and map
> + * @dst_iova: IOVA the mapping was placed at. If IOMMU_IOAS_MAP_FIXED_IOVA is
> + *set then this must be provided as input.
> + * @src_iova: IOVA to start the copy
> + *
> + * Copy an already existing mapping from src_ioas_id and establish it in
> + * dst_ioas_id. The src iova/length must exactly match a range used
Re: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail
On Wed, Mar 30, 2022 at 03:17:20PM -0400, Alex Xu (Hello71) wrote:
> Excerpts from Christoph Hellwig's message of March 30, 2022 2:01 pm:
> > Can you try this patch, which is a bit of a hack?
> >
> > diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> > index 50d209939c66c..61997c2ee0a17 100644
> > --- a/arch/x86/mm/mem_encrypt.c
> > +++ b/arch/x86/mm/mem_encrypt.c
> > @@ -28,7 +28,8 @@ bool force_dma_unencrypted(struct device *dev)
> > * device does not support DMA to addresses that include the
> > * encryption mask.
> > */
> > - if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> > + if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) &&
> > + !get_dma_ops(dev)) {
> > u64 dma_enc_mask = DMA_BIT_MASK(__ffs64(sme_me_mask));
> > u64 dma_dev_mask = min_not_zero(dev->coherent_dma_mask,
> > dev->bus_dma_limit);
> >
>
> This seems to work for me.
Ok, I'll try to come up with a less hacky version and will start a
discussion with the AMD folks that know memory encryption better.
Thanks for the report and testing already!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail
Excerpts from Christoph Hellwig's message of March 30, 2022 2:01 pm:
> Can you try this patch, which is a bit of a hack?
>
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 50d209939c66c..61997c2ee0a17 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -28,7 +28,8 @@ bool force_dma_unencrypted(struct device *dev)
>* device does not support DMA to addresses that include the
>* encryption mask.
>*/
> - if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
> + if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) &&
> + !get_dma_ops(dev)) {
> u64 dma_enc_mask = DMA_BIT_MASK(__ffs64(sme_me_mask));
> u64 dma_dev_mask = min_not_zero(dev->coherent_dma_mask,
> dev->bus_dma_limit);
>
This seems to work for me.
Cheers,
Alex.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 06/11] iommu/vt-d: Add SVA domain support
On Tue, Mar 29, 2022 at 01:37:55PM +0800, Lu Baolu wrote:
> Add support for SVA domain allocation and provide an SVA-specific
> iommu_domain_ops.
>
> Signed-off-by: Lu Baolu
> include/linux/intel-iommu.h | 1 +
> drivers/iommu/intel/iommu.c | 10 ++
> drivers/iommu/intel/svm.c | 37 +
> 3 files changed, 48 insertions(+)
>
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 2f9891cb3d00..c14283137fb5 100644
> +++ b/include/linux/intel-iommu.h
> @@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
> u32 intel_svm_get_pasid(struct iommu_sva *handle);
> int intel_svm_page_response(struct device *dev, struct iommu_fault_event
> *evt,
> struct iommu_page_response *msg);
> +extern const struct iommu_domain_ops intel_svm_domain_ops;
>
> struct intel_svm_dev {
> struct list_head list;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c1b91bce1530..5eae7cf9bee5 100644
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4318,6 +4318,16 @@ static struct iommu_domain
> *intel_iommu_domain_alloc(unsigned type)
> return domain;
> case IOMMU_DOMAIN_IDENTITY:
> return _domain->domain;
> +#ifdef CONFIG_INTEL_IOMMU_SVM
> + case IOMMU_DOMAIN_SVA:
> + dmar_domain = alloc_domain(type);
> + if (!dmar_domain)
> + return NULL;
> + domain = _domain->domain;
> + domain->ops = _svm_domain_ops;
> +
> + return domain;
> +#endif /* CONFIG_INTEL_IOMMU_SVM */
If this is the usual pattern for drivers I would prefer to see an
alloc_sva op instead of more and more types.
Multiplexing functions is often not a great idea...
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 04/11] iommu: Add attach/detach_dev_pasid domain ops
On Tue, Mar 29, 2022 at 01:37:53PM +0800, Lu Baolu wrote:
> Attaching an IOMMU domain to a PASID of a device is a generic operation
> for modern IOMMU drivers which support PASID-granular DMA address
> translation. Currently visible usage scenarios include (but not limited):
>
> - SVA (Shared Virtual Address)
> - kernel DMA with PASID
> - hardware-assist mediated device
>
> This adds a pair of common domain ops for this purpose and adds some
> common helpers to attach/detach a domain to/from a {device, PASID} and
> get/put the domain attached to {device, PASID}.
>
> Signed-off-by: Lu Baolu
> include/linux/iommu.h | 36 ++
> drivers/iommu/iommu.c | 88 +++
> 2 files changed, 124 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 29c4c2edd706..a46285488a57 100644
> +++ b/include/linux/iommu.h
> @@ -269,6 +269,8 @@ struct iommu_ops {
> * struct iommu_domain_ops - domain specific operations
> * @attach_dev: attach an iommu domain to a device
> * @detach_dev: detach an iommu domain from a device
> + * @attach_dev_pasid: attach an iommu domain to a pasid of device
> + * @detach_dev_pasid: detach an iommu domain from a pasid of device
> * @map: map a physically contiguous memory region to an iommu domain
> * @map_pages: map a physically contiguous set of pages of the same size to
> * an iommu domain.
> @@ -286,6 +288,10 @@ struct iommu_ops {
> struct iommu_domain_ops {
> int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
> void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
> + int (*attach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
> + void (*detach_dev_pasid)(struct iommu_domain *domain,
> + struct device *dev, ioasid_t id);
ID should be pasid for consistency
> +int iommu_attach_device_pasid(struct iommu_domain *domain,
> + struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group;
> + int ret = -EINVAL;
> + void *curr;
> +
> + if (!domain->ops->attach_dev_pasid)
> + return -EINVAL;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return -ENODEV;
> +
> + mutex_lock(>mutex);
> + /*
> + * To keep things simple, we currently don't support IOMMU groups
> + * with more than one device. Existing SVA-capable systems are not
> + * affected by the problems that required IOMMU groups (lack of ACS
> + * isolation, device ID aliasing and other hardware issues).
> + */
> + if (!iommu_group_singleton_lockdown(group))
> + goto out_unlock;
> +
> + xa_lock(>pasid_array);
> + curr = __xa_cmpxchg(>pasid_array, pasid, NULL,
> + domain, GFP_KERNEL);
> + xa_unlock(>pasid_array);
Why the xa_lock/unlock? Just call the normal xa_cmpxchg?
> +void iommu_detach_device_pasid(struct iommu_domain *domain,
> +struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_group *group;
> +
> + group = iommu_group_get(dev);
> + if (WARN_ON(!group))
> + return;
This group_get stuff really needs some cleaning, this makes no sense
at all.
If the kref to group can go to zero within this function then the
initial access of the kref is already buggy:
if (group)
kobject_get(group->devices_kobj);
Because it will crash or WARN_ON.
We don't hit this because it is required that a group cannot be
destroyed while a struct device has a driver bound, and all these
paths are driver bound paths.
So none of these group_get/put patterns have any purpose at all, and
now we are adding impossible WARN_ONs to..
> +struct iommu_domain *
> +iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid)
> +{
> + struct iommu_domain *domain;
> + struct iommu_group *group;
> +
> + group = iommu_group_get(dev);
> + if (!group)
> + return NULL;
And now we are doing useless things on a performance path!
> + mutex_lock(>mutex);
> + domain = xa_load(>pasid_array, pasid);
> + if (domain && domain->type == IOMMU_DOMAIN_SVA)
> + iommu_sva_domain_get_user(domain);
> + mutex_unlock(>mutex);
> + iommu_group_put(group);
Why do we need so much locking on a performance path? RCU out of the
xarray..
Not sure I see how this get_user refcounting can work ?
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 03/11] iommu/sva: Add iommu_domain type for SVA
On Tue, Mar 29, 2022 at 01:37:52PM +0800, Lu Baolu wrote:
> @@ -95,6 +101,7 @@ struct iommu_domain {
> void *handler_token;
> struct iommu_domain_geometry geometry;
> struct iommu_dma_cookie *iova_cookie;
> + struct iommu_sva_cookie *sva_cookie;
Cookie is still the wrong word to use here
> +struct iommu_sva_cookie {
> + struct mm_struct *mm;
> + ioasid_t pasid;
> + refcount_t users;
Really surprised to see a refcount buried inside the iommu_domain..
This design seems inside out, the SVA struct should 'enclose' the domain, not
be a pointer inside it.
struct iommu_sva_domain {
struct kref_t kref;
struct mm_struct *mm;
ioasid_t pasid;
/* All the domains that are linked to this */
struct xarray domain_list;
};
And then you could have a pointer to that inside the mm_struct instead
of just the naked pasid.
> +static __maybe_unused struct iommu_domain *
Why maybe unused?
> +iommu_sva_get_domain(struct device *dev, struct mm_struct *mm)
> +{
> + struct iommu_domain *domain;
> + ioasid_t pasid = mm->pasid;
> +
> + if (pasid == INVALID_IOASID)
> + return NULL;
> +
> + domain = xa_load(_domain_array, pasid);
> + if (!domain)
> + return iommu_sva_alloc_domain(dev, mm);
> + iommu_sva_domain_get_user(domain);
This assumes any domain is interchangeable with any device, which is
not the iommu model. We need a domain op to check if a device is
compatiable with the domain for vfio an iommufd, this should do the
same.
It means each mm can have a list of domains associated with it and a
new domain is auto-created if the device doesn't work with any of the
existing domains.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
"dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail
Hi,
After a recent kernel update, booting one of my machines causes it to
hang on a black screen. Pressing Lock keys on the USB keyboard does not
turn on the indicators, and the machine does not appear on the Ethernet
network. I don't have a serial port on this machine. I didn't try
netconsole, but I suspect it won't work.
Setting mem_encrypt=0 seems to resolve the issue. Reverting f5ff79fddf0e
("dma-mapping: remove CONFIG_DMA_REMAP") also appears to resolve the
issue.
The machine in question has an AMD Ryzen 5 1600 and ASRock B450 Pro4.
Cheers,
Alex.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail
Can you try this patch, which is a bit of a hack?
diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
index 50d209939c66c..61997c2ee0a17 100644
--- a/arch/x86/mm/mem_encrypt.c
+++ b/arch/x86/mm/mem_encrypt.c
@@ -28,7 +28,8 @@ bool force_dma_unencrypted(struct device *dev)
* device does not support DMA to addresses that include the
* encryption mask.
*/
- if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) {
+ if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) &&
+ !get_dma_ops(dev)) {
u64 dma_enc_mask = DMA_BIT_MASK(__ffs64(sme_me_mask));
u64 dma_dev_mask = min_not_zero(dev->coherent_dma_mask,
dev->bus_dma_limit);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: "dma-mapping: remove CONFIG_DMA_REMAP" causes AMD SME boot fail
On Wed, Mar 30, 2022 at 01:51:07PM -0400, Alex Xu (Hello71) wrote:
> Hi,
>
> After a recent kernel update, booting one of my machines causes it to
> hang on a black screen. Pressing Lock keys on the USB keyboard does not
> turn on the indicators, and the machine does not appear on the Ethernet
> network. I don't have a serial port on this machine. I didn't try
> netconsole, but I suspect it won't work.
>
> Setting mem_encrypt=0 seems to resolve the issue. Reverting f5ff79fddf0e
> ("dma-mapping: remove CONFIG_DMA_REMAP") also appears to resolve the
> issue.
>
> The machine in question has an AMD Ryzen 5 1600 and ASRock B450 Pro4.
This looks like something in the AMD IOMMU code or it's users can't
deal with vmalloc addresses. I'll start looking for a culprit ASAP.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
On Wed, Mar 30, 2022 at 08:19:37PM +0300, Tony Lindgren wrote:
> > > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > > omap_iommu_init from do_one_initcall+0x44/0x24c
> > >
> > > Any ideas for a fix?
> > >
> > > It would be good to fix this quickly so we don't end up with a broken
> > > v5.18-rc1..
> > >
> > > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > > argument in is_attach_deferred").
> >
> > Are you confident in the bisection? I don't see how that commit could
> > NULL deref..
>
> Oops sorry you're right, the breaking commit is a different patch, it's
> 3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
> have picked the wrong commit while testing.
That makes alot more sense
> > Can you find the code that is the NULL deref?
>
> I can debug a bit more tomorrow.
Looks like omap's omap_iommu_probe_device() is buggy, it returns 0 on
error paths:
num_iommus = of_property_count_elems_of_size(dev->of_node, "iommus",
sizeof(phandle));
if (num_iommus < 0)
return 0;
This function needs to return an errno -ENODEV
Otherwise it causes a NULL dev->iommu->iommu_dev and dev_iommu_ops() will
crash.
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
* Jason Gunthorpe [220330 14:21]:
> On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> > Hi,
> >
> > * Lu Baolu [700101 02:00]:
> > > The is_attach_deferred iommu_ops callback is a device op. The domain
> > > argument is unnecessary and never used. Remove it to make code clean.
> >
> > Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
> >
> > To me it seems the issue is there is no is_attach_deferred implemented, so
> > we get a NULL pointer dereference at virtual address 0008:
> >
> > __iommu_probe_device from probe_iommu_group+0x2c/0x38
> > probe_iommu_group from bus_for_each_dev+0x74/0xbc
> > bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> > bus_iommu_probe from bus_set_iommu+0x80/0xc8
> > bus_set_iommu from omap_iommu_init+0x88/0xcc
> > omap_iommu_init from do_one_initcall+0x44/0x24c
> >
> > Any ideas for a fix?
> >
> > It would be good to fix this quickly so we don't end up with a broken
> > v5.18-rc1..
> >
> > For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> > argument in is_attach_deferred").
>
> Are you confident in the bisection? I don't see how that commit could
> NULL deref..
Oops sorry you're right, the breaking commit is a different patch, it's
3f6634d997db ("iommu: Use right way to retrieve iommu_ops") instead. I must
have picked the wrong commit while testing.
> Can you find the code that is the NULL deref?
I can debug a bit more tomorrow.
Regards,
Tony
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, 30 Mar 2022 14:18:47 + "Tian, Kevin" wrote: > +Alex > > > From: Tian, Kevin > > Sent: Wednesday, March 30, 2022 10:13 PM > > > > > From: Jason Gunthorpe > > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > > > One thing that I'm not very sure is about DMA alias. Even when > > > > physically > > > > there is only a single device within the group the aliasing could lead > > > > to multiple RIDs in the group making it non-singleton. But probably we > > > > don't need support SVA on such device until a real demand comes? > > > > > > How can we have multiple RIDs in the same group and have only one > > > device in the group? > > > > Alex may help throw some insight here. Per what I read from the code > > looks like certain device can generate traffic with multiple RIDs. You only need to look so far as the dma alias quirks to find devices that use the wrong RID for DMA. In general I don't think we have enough confidence to say that for all these devices the wrong RID is exclusively used versus some combination of both RIDs. Also, the aliased RID is not always a physical device. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, Mar 30, 2022 at 02:12:57PM +, Tian, Kevin wrote: > > From: Jason Gunthorpe > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > One thing that I'm not very sure is about DMA alias. Even when physically > > > there is only a single device within the group the aliasing could lead > > > to multiple RIDs in the group making it non-singleton. But probably we > > > don't need support SVA on such device until a real demand comes? > > > > How can we have multiple RIDs in the same group and have only one > > device in the group? > > Alex may help throw some insight here. Per what I read from the code > looks like certain device can generate traffic with multiple RIDs. IIRC "dma alias" refers to things like legacy PCI to PCIe bridges that do still have multiple PCI ID's behind the bridge used in configuration cycles however the PCI to PCIe bridge will tag all PCIe TLPs with its own RID because classic PCI has no way for the requestor to convey a RID to the bridge. So, from a Linux perspective the group should have have multiple struct devices behind the bridge, the bridge itself, and the RID the IOMMU HW matches on is only the RID of the PCI bridge. But we know this because we know there is classic PCI stuff in the heigharchy, so we can just mark that group as incompatible. > > Add a flag to the group that positively indicates the group can never > > have more than one member, even after hot plug. eg because it is > > impossible due to ACS, or lack of bridges, and so on. > > OK, I see your point. It essentially refers to a singleton group which > is immutable to hotplug. Yes, known at creation time, not retroactively enforced because someone used SVA Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
On Wed, Mar 30, 2022 at 05:00:39PM +0300, Tony Lindgren wrote:
> Hi,
>
> * Lu Baolu [700101 02:00]:
> > The is_attach_deferred iommu_ops callback is a device op. The domain
> > argument is unnecessary and never used. Remove it to make code clean.
>
> Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
>
> To me it seems the issue is there is no is_attach_deferred implemented, so
> we get a NULL pointer dereference at virtual address 0008:
>
> __iommu_probe_device from probe_iommu_group+0x2c/0x38
> probe_iommu_group from bus_for_each_dev+0x74/0xbc
> bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
> bus_iommu_probe from bus_set_iommu+0x80/0xc8
> bus_set_iommu from omap_iommu_init+0x88/0xcc
> omap_iommu_init from do_one_initcall+0x44/0x24c
>
> Any ideas for a fix?
>
> It would be good to fix this quickly so we don't end up with a broken
> v5.18-rc1..
>
> For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
> argument in is_attach_deferred").
Are you confident in the bisection? I don't see how that commit could
NULL deref..
Can you find the code that is the NULL deref?
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V3 1/2] Documentation: x86: Add documentation for AMD IOMMU
On 3/30/22 07:01, Deucher, Alexander wrote: >> Just scanning this, it looks *awfully* generic. Is anything in >> here AMD- specific? Should this be in an AMD-specific file? > There is some information about the ACPI tables used to enumerate the > IOMMUs and a link to the AMD IOMMU programming documentation. Would > you prefer I just create a combined x86 IOMMU document? Yeah, I think that would make a lot of sense. Let's just have one document with an AMD section and an Intel section. Maybe just rename the existing one to intel-iommu => x86-iommu.rst. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 8/9] iommu: Remove unused argument in is_attach_deferred
Hi,
* Lu Baolu [700101 02:00]:
> The is_attach_deferred iommu_ops callback is a device op. The domain
> argument is unnecessary and never used. Remove it to make code clean.
Looks like this causes a regression for at least drivers/iommu/omap-iommu.c.
To me it seems the issue is there is no is_attach_deferred implemented, so
we get a NULL pointer dereference at virtual address 0008:
__iommu_probe_device from probe_iommu_group+0x2c/0x38
probe_iommu_group from bus_for_each_dev+0x74/0xbc
bus_for_each_dev from bus_iommu_probe+0x34/0x2e8
bus_iommu_probe from bus_set_iommu+0x80/0xc8
bus_set_iommu from omap_iommu_init+0x88/0xcc
omap_iommu_init from do_one_initcall+0x44/0x24c
Any ideas for a fix?
It would be good to fix this quickly so we don't end up with a broken
v5.18-rc1..
For reference, this is mainline commit 41bb23e70b50 ("iommu: Remove unused
argument in is_attach_deferred").
Regards,
Tony
#regzbot ^introduced 41bb23e70b50
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
+Alex > From: Tian, Kevin > Sent: Wednesday, March 30, 2022 10:13 PM > > > From: Jason Gunthorpe > > Sent: Wednesday, March 30, 2022 7:58 PM > > > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > > > One thing that I'm not very sure is about DMA alias. Even when physically > > > there is only a single device within the group the aliasing could lead > > > to multiple RIDs in the group making it non-singleton. But probably we > > > don't need support SVA on such device until a real demand comes? > > > > How can we have multiple RIDs in the same group and have only one > > device in the group? > > Alex may help throw some insight here. Per what I read from the code > looks like certain device can generate traffic with multiple RIDs. > > > > > > > ie if we have a singleton group that doesn't have ACS and someone > > > > hotplugs in another device on a bridge, then our SVA is completely > > > > broken and we get data corruption. > > > > > > Can we capture that in iommu_probe_device() when identifying > > > the group which the probed device will be added to has already been > > > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > > > in this patch to lock down the fact of singleton group instead of > > > the fact of singleton driver... > > > > No, that is backwards > > > > > > Testing the group size is inherently the wrong test to make. > > > > > > What is your suggestion then? > > > > Add a flag to the group that positively indicates the group can never > > have more than one member, even after hot plug. eg because it is > > impossible due to ACS, or lack of bridges, and so on. > > > > OK, I see your point. It essentially refers to a singleton group which > is immutable to hotplug. > > Thanks > Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Jason Gunthorpe > Sent: Wednesday, March 30, 2022 7:58 PM > > On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > > > One thing that I'm not very sure is about DMA alias. Even when physically > > there is only a single device within the group the aliasing could lead > > to multiple RIDs in the group making it non-singleton. But probably we > > don't need support SVA on such device until a real demand comes? > > How can we have multiple RIDs in the same group and have only one > device in the group? Alex may help throw some insight here. Per what I read from the code looks like certain device can generate traffic with multiple RIDs. > > > > ie if we have a singleton group that doesn't have ACS and someone > > > hotplugs in another device on a bridge, then our SVA is completely > > > broken and we get data corruption. > > > > Can we capture that in iommu_probe_device() when identifying > > the group which the probed device will be added to has already been > > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > > in this patch to lock down the fact of singleton group instead of > > the fact of singleton driver... > > No, that is backwards > > > > Testing the group size is inherently the wrong test to make. > > > > What is your suggestion then? > > Add a flag to the group that positively indicates the group can never > have more than one member, even after hot plug. eg because it is > impossible due to ACS, or lack of bridges, and so on. > OK, I see your point. It essentially refers to a singleton group which is immutable to hotplug. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V3 1/2] Documentation: x86: Add documentation for AMD IOMMU
[Public] > -Original Message- > From: Dave Hansen > Sent: Tuesday, March 29, 2022 11:25 AM > To: Deucher, Alexander ; linux- > d...@vger.kernel.org; linux-ker...@vger.kernel.org; cor...@lwn.net; > h...@zytor.com; x...@kernel.org; dave.han...@linux.intel.com; > b...@alien8.de; mi...@redhat.com; t...@linutronix.de; j...@8bytes.org; > Suthikulpanit, Suravee ; w...@kernel.org; > iommu@lists.linux-foundation.org; robin.mur...@arm.com; Hegde, Vasant > > Subject: Re: [PATCH V3 1/2] Documentation: x86: Add documentation for > AMD IOMMU > > On 3/28/22 10:28, Alex Deucher wrote: > > +How is IOVA generated? > > +-- > > + > > +Well behaved drivers call dma_map_*() calls before sending command to > > +device that needs to perform DMA. Once DMA is completed and mapping > > +is no longer required, driver performs dma_unmap_*() calls to unmap the > region. > > + > > +Fault reporting > > +--- > > + > > +When errors are reported, the IOMMU signals via an interrupt. The > > +fault reason and device that caused it is printed on the console. > > Just scanning this, it looks *awfully* generic. Is anything in here AMD- > specific? Should this be in an AMD-specific file? There is some information about the ACPI tables used to enumerate the IOMMUs and a link to the AMD IOMMU programming documentation. Would you prefer I just create a combined x86 IOMMU document? Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 08/12] iommufd: IOCTLs for the io_pagetable
Hi Jason,
On 2022/3/19 01:27, Jason Gunthorpe wrote:
Connect the IOAS to its IOCTL interface. This exposes most of the
functionality in the io_pagetable to userspace.
This is intended to be the core of the generic interface that IOMMUFD will
provide. Every IOMMU driver should be able to implement an iommu_domain
that is compatible with this generic mechanism.
It is also designed to be easy to use for simple non virtual machine
monitor users, like DPDK:
- Universal simple support for all IOMMUs (no PPC special path)
- An IOVA allocator that considerds the aperture and the reserved ranges
- io_pagetable allows any number of iommu_domains to be connected to the
IOAS
Along with room in the design to add non-generic features to cater to
specific HW functionality.
Signed-off-by: Jason Gunthorpe
---
drivers/iommu/iommufd/Makefile | 1 +
drivers/iommu/iommufd/ioas.c| 248
drivers/iommu/iommufd/iommufd_private.h | 27 +++
drivers/iommu/iommufd/main.c| 17 ++
include/uapi/linux/iommufd.h| 132 +
5 files changed, 425 insertions(+)
create mode 100644 drivers/iommu/iommufd/ioas.c
diff --git a/drivers/iommu/iommufd/Makefile b/drivers/iommu/iommufd/Makefile
index b66a8c47ff55ec..2b4f36f1b72f9d 100644
--- a/drivers/iommu/iommufd/Makefile
+++ b/drivers/iommu/iommufd/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
iommufd-y := \
io_pagetable.o \
+ ioas.o \
main.o \
pages.o
diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
new file mode 100644
index 00..c530b2ba74b06b
--- /dev/null
+++ b/drivers/iommu/iommufd/ioas.c
@@ -0,0 +1,248 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021-2022, NVIDIA CORPORATION & AFFILIATES
+ */
+#include
+#include
+#include
+#include
+
+#include "io_pagetable.h"
+
+void iommufd_ioas_destroy(struct iommufd_object *obj)
+{
+ struct iommufd_ioas *ioas = container_of(obj, struct iommufd_ioas, obj);
+ int rc;
+
+ rc = iopt_unmap_all(>iopt);
+ WARN_ON(rc);
+ iopt_destroy_table(>iopt);
+}
+
+struct iommufd_ioas *iommufd_ioas_alloc(struct iommufd_ctx *ictx)
+{
+ struct iommufd_ioas *ioas;
+ int rc;
+
+ ioas = iommufd_object_alloc(ictx, ioas, IOMMUFD_OBJ_IOAS);
+ if (IS_ERR(ioas))
+ return ioas;
+
+ rc = iopt_init_table(>iopt);
+ if (rc)
+ goto out_abort;
+ return ioas;
+
+out_abort:
+ iommufd_object_abort(ictx, >obj);
+ return ERR_PTR(rc);
+}
+
+int iommufd_ioas_alloc_ioctl(struct iommufd_ucmd *ucmd)
+{
+ struct iommu_ioas_alloc *cmd = ucmd->cmd;
+ struct iommufd_ioas *ioas;
+ int rc;
+
+ if (cmd->flags)
+ return -EOPNOTSUPP;
+
+ ioas = iommufd_ioas_alloc(ucmd->ictx);
+ if (IS_ERR(ioas))
+ return PTR_ERR(ioas);
+
+ cmd->out_ioas_id = ioas->obj.id;
+ rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+ if (rc)
+ goto out_table;
+ iommufd_object_finalize(ucmd->ictx, >obj);
+ return 0;
+
+out_table:
+ iommufd_ioas_destroy(>obj);
+ return rc;
+}
+
+int iommufd_ioas_iova_ranges(struct iommufd_ucmd *ucmd)
+{
+ struct iommu_ioas_iova_ranges __user *uptr = ucmd->ubuffer;
+ struct iommu_ioas_iova_ranges *cmd = ucmd->cmd;
+ struct iommufd_ioas *ioas;
+ struct interval_tree_span_iter span;
+ u32 max_iovas;
+ int rc;
+
+ if (cmd->__reserved)
+ return -EOPNOTSUPP;
+
+ max_iovas = cmd->size - sizeof(*cmd);
+ if (max_iovas % sizeof(cmd->out_valid_iovas[0]))
+ return -EINVAL;
+ max_iovas /= sizeof(cmd->out_valid_iovas[0]);
+
+ ioas = iommufd_get_ioas(ucmd, cmd->ioas_id);
+ if (IS_ERR(ioas))
+ return PTR_ERR(ioas);
+
+ down_read(>iopt.iova_rwsem);
+ cmd->out_num_iovas = 0;
+ for (interval_tree_span_iter_first(
+, >iopt.reserved_iova_itree, 0, ULONG_MAX);
+!interval_tree_span_iter_done();
+interval_tree_span_iter_next()) {
+ if (!span.is_hole)
+ continue;
+ if (cmd->out_num_iovas < max_iovas) {
+ rc = put_user((u64)span.start_hole,
+ >out_valid_iovas[cmd->out_num_iovas]
+ .start);
+ if (rc)
+ goto out_put;
+ rc = put_user(
+ (u64)span.last_hole,
+
>out_valid_iovas[cmd->out_num_iovas].last);
+ if (rc)
+ goto out_put;
+ }
+ cmd->out_num_iovas++;
+ }
+ rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
+ if (rc)
+
Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP
On Mon, 28 Mar 2022 08:37:23 +0200 Christoph Hellwig wrote: > On Fri, Mar 25, 2022 at 11:46:09AM -0700, Linus Torvalds wrote: > > I think my list of three different sync cases (not just two! It's not > > just about whether to sync for the CPU or the device, it's also about > > what direction the data itself is taking) is correct. > > > > But maybe I'm wrong. > > At the high level you are correct. It is all about which direction > the data is taking. That is the direction argument that all the > map/unmap/sync call take. The sync calls then just toggle the ownership. > You seem to hate that ownership concept, but I don't see how things > could work without that ownership concept as we're going to be in > trouble without having that. And yes, a peek operation could work in > some cases, but it would have to be at the cache line granularity. > > arch/arc/mm/dma.c has a really good comment how these transfers relate > to actual cache operations btw> > > * > * | map == for_device | unmap == for_cpu > * | > * TO_DEV | writebackwriteback | none none > * FROM_DEV | invalidate invalidate | invalidate* > invalidate* Very interesting! Does that mean that memset(buf, 0, size); dma_map(buf, size, FROM_DEVICE) device does a partial write dma_unmap(buf, size, FROM_DEVICE) may boil down to being the same as without the memset(), because the effect of the memset() may get thrown away by the cache invalidate? That is in this scenario we could actually leak the original content of the buffer if we have a non-dma-coherent cache? Regards, Halil > * BIDIR| writeback+invwriteback+inv | invalidateinvalidate > * > * [*] needed for CPU speculative prefetches ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 01/11] iommu: Add pasid_bits field in struct dev_iommu
On 2022/3/30 15:05, Tian, Kevin wrote: From: Lu Baolu Sent: Tuesday, March 29, 2022 1:38 PM Use this field to save the pasid/ssid bits that a device is able to support with its IOMMU hardware. It is a generic attribute of a device and lifting it into the per-device dev_iommu struct makes it possible to allocate a PASID for device without calls into the IOMMU drivers. Any iommu driver which suports PASID related features should set this field before features are enabled on the devices. Miss a clarification for non-PCI SVA as discussed here: https://lore.kernel.org/all/85d61ad6-0cf0-ac65-3312-32d0cdeb1...@linux.intel.com/ Yes. Thanks for the reminding. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
On Wed, Mar 30, 2022 at 06:50:11AM +, Tian, Kevin wrote: > One thing that I'm not very sure is about DMA alias. Even when physically > there is only a single device within the group the aliasing could lead > to multiple RIDs in the group making it non-singleton. But probably we > don't need support SVA on such device until a real demand comes? How can we have multiple RIDs in the same group and have only one device in the group? > > ie if we have a singleton group that doesn't have ACS and someone > > hotplugs in another device on a bridge, then our SVA is completely > > broken and we get data corruption. > > Can we capture that in iommu_probe_device() when identifying > the group which the probed device will be added to has already been > locked down for SVA? i.e. make iommu_group_singleton_lockdown() > in this patch to lock down the fact of singleton group instead of > the fact of singleton driver... No, that is backwards > > Testing the group size is inherently the wrong test to make. > > What is your suggestion then? Add a flag to the group that positively indicates the group can never have more than one member, even after hot plug. eg because it is impossible due to ACS, or lack of bridges, and so on. Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
Hi Kevin, On 2022/3/30 14:50, Tian, Kevin wrote: ie if we have a singleton group that doesn't have ACS and someone hotplugs in another device on a bridge, then our SVA is completely broken and we get data corruption. Can we capture that in iommu_probe_device() when identifying the group which the probed device will be added to has already been locked down for SVA? i.e. make iommu_group_singleton_lockdown() in this patch to lock down the fact of singleton group instead of the fact of singleton driver... The iommu_probe_device() is called in the bus notifier callback. It has no way to stop the probe of the device. Unless we could add a direct call in the device probe path of the driver core? Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.15 12/50] media: iommu/mediatek: Add device_link between the consumer and the larb devices
From: Yong Wu
[ Upstream commit 635319a4a7444ca97124d781cd96deb277ff4d40 ]
MediaTek IOMMU-SMI diagram is like below. all the consumer connect with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and the
smi-larbs. then add device link between the smi-larb and smi-common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
pm_runtime_xx to keep the original status of clocks. It can avoid two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
all the clocks are enabled before entering kernel, but the clocks for
display HW(always in larb0) will be gated after clk_enable and clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
operation happened before display driver probe. At that time, the display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
Meanwhile, Currently we don't have a device connect with 2 larbs at the
same time. Disallow this case, print the error log.
[1] https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 30 ++
drivers/iommu/mtk_iommu_v1.c | 29 -
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 77ae20ff9b35..5971a1168666 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -562,22 +562,52 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid, larbidx, i;
if (!fwspec || fwspec->ops != _iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device */
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier).
+* The device that connects with each a larb is a independent HW.
+* All the ports in each a device should be in the same larbs.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ for (i = 1; i < fwspec->num_ids; i++) {
+ larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
+ if (larbid != larbidx) {
+ dev_err(dev, "Can only use one larb. Fail@larb%d-%d.\n",
+ larbid, larbidx);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
return >iommu;
}
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct mtk_iommu_data *data;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return;
+ data = dev_iommu_priv_get(dev);
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ device_link_remove(dev, larbdev);
+
iommu_fwspec_free(dev);
}
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 68bf02f87cfd..bc7ee90b9373 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -423,7 +423,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct of_phandle_args iommu_spec;
struct mtk_iommu_data *data;
- int err, idx = 0;
+ int err, idx = 0, larbid, larbidx;
+ struct device_link *link;
+ struct device *larbdev;
/*
* In the deferred case, free the existed fwspec.
@@ -453,6 +455,23 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
data = dev_iommu_priv_get(dev);
+ /* Link the
[PATCH AUTOSEL 5.15 10/50] media: iommu/mediatek-v1: Free the existed fwspec if the master dev already has
From: Yong Wu
[ Upstream commit 822a2ed8c606caf6a11b1a180b8e46292bd77d71 ]
When the iommu master device enters of_iommu_xlate, the ops may be
NULL(iommu dev is defered), then it will initialize the fwspec here:
[] (dev_iommu_fwspec_set) from []
(iommu_fwspec_init+0xbc/0xd4)
[] (iommu_fwspec_init) from []
(of_iommu_xlate+0x7c/0x12c)
[] (of_iommu_xlate) from []
(of_iommu_configure+0x144/0x1e8)
BUT the mtk_iommu_v1.c only supports arm32, the probing flow still is a bit
weird. We always expect create the fwspec internally. otherwise it will
enter here and return fail.
static int mtk_iommu_create_mapping(struct device *dev,
struct of_phandle_args *args)
{
...
if (!fwspec) {
} else if (dev_iommu_fwspec_get(dev)->ops != _iommu_ops) {
>>Enter here. return fail.
return -EINVAL;
}
...
}
Thus, Free the existed fwspec if the master device already has fwspec.
This issue is reported at:
https://lore.kernel.org/linux-mediatek/trinity-7d9ebdc9-4849-4d93-bfb5-429dcb4ee449-1626253158870@3c-app-gmx-bs01/
Reported-by: Frank Wunderlich
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Acked-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu_v1.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index be22fcf988ce..1467ba1e4417 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -425,6 +425,15 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct mtk_iommu_data *data;
int err, idx = 0;
+ /*
+* In the deferred case, free the existed fwspec.
+* Always initialize the fwspec internally.
+*/
+ if (fwspec) {
+ iommu_fwspec_free(dev);
+ fwspec = dev_iommu_fwspec_get(dev);
+ }
+
while (!of_parse_phandle_with_args(dev->of_node, "iommus",
"#iommu-cells",
idx, _spec)) {
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.15 11/50] media: iommu/mediatek: Return ENODEV if the device is NULL
From: Yong Wu
[ Upstream commit 2fb0feed51085db77606de9b9477c96894328809 ]
The platform device is created at:
of_platform_default_populate_init: arch_initcall_sync
->of_platform_populate
->of_platform_device_create_pdata
When entering our probe, all the devices should be already created.
if it is null, means NODEV. Currently we don't get the fail case.
It's a minor fix, no need add fixes tags.
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 2 +-
drivers/iommu/mtk_iommu_v1.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 25b834104790..77ae20ff9b35 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -848,7 +848,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[id].dev = >dev;
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 1467ba1e4417..68bf02f87cfd 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -604,7 +604,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[i].dev = >dev;
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.16 13/59] media: iommu/mediatek: Add device_link between the consumer and the larb devices
From: Yong Wu
[ Upstream commit 635319a4a7444ca97124d781cd96deb277ff4d40 ]
MediaTek IOMMU-SMI diagram is like below. all the consumer connect with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and the
smi-larbs. then add device link between the smi-larb and smi-common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
pm_runtime_xx to keep the original status of clocks. It can avoid two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
all the clocks are enabled before entering kernel, but the clocks for
display HW(always in larb0) will be gated after clk_enable and clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
operation happened before display driver probe. At that time, the display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
Meanwhile, Currently we don't have a device connect with 2 larbs at the
same time. Disallow this case, print the error log.
[1] https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 30 ++
drivers/iommu/mtk_iommu_v1.c | 29 -
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 77ae20ff9b35..5971a1168666 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -562,22 +562,52 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid, larbidx, i;
if (!fwspec || fwspec->ops != _iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device */
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier).
+* The device that connects with each a larb is a independent HW.
+* All the ports in each a device should be in the same larbs.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ for (i = 1; i < fwspec->num_ids; i++) {
+ larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
+ if (larbid != larbidx) {
+ dev_err(dev, "Can only use one larb. Fail@larb%d-%d.\n",
+ larbid, larbidx);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
return >iommu;
}
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct mtk_iommu_data *data;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return;
+ data = dev_iommu_priv_get(dev);
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ device_link_remove(dev, larbdev);
+
iommu_fwspec_free(dev);
}
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 68bf02f87cfd..bc7ee90b9373 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -423,7 +423,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct of_phandle_args iommu_spec;
struct mtk_iommu_data *data;
- int err, idx = 0;
+ int err, idx = 0, larbid, larbidx;
+ struct device_link *link;
+ struct device *larbdev;
/*
* In the deferred case, free the existed fwspec.
@@ -453,6 +455,23 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
data = dev_iommu_priv_get(dev);
+ /* Link the
[PATCH AUTOSEL 5.16 12/59] media: iommu/mediatek: Return ENODEV if the device is NULL
From: Yong Wu
[ Upstream commit 2fb0feed51085db77606de9b9477c96894328809 ]
The platform device is created at:
of_platform_default_populate_init: arch_initcall_sync
->of_platform_populate
->of_platform_device_create_pdata
When entering our probe, all the devices should be already created.
if it is null, means NODEV. Currently we don't get the fail case.
It's a minor fix, no need add fixes tags.
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 2 +-
drivers/iommu/mtk_iommu_v1.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 25b834104790..77ae20ff9b35 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -848,7 +848,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[id].dev = >dev;
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 1467ba1e4417..68bf02f87cfd 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -604,7 +604,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[i].dev = >dev;
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.16 11/59] media: iommu/mediatek-v1: Free the existed fwspec if the master dev already has
From: Yong Wu
[ Upstream commit 822a2ed8c606caf6a11b1a180b8e46292bd77d71 ]
When the iommu master device enters of_iommu_xlate, the ops may be
NULL(iommu dev is defered), then it will initialize the fwspec here:
[] (dev_iommu_fwspec_set) from []
(iommu_fwspec_init+0xbc/0xd4)
[] (iommu_fwspec_init) from []
(of_iommu_xlate+0x7c/0x12c)
[] (of_iommu_xlate) from []
(of_iommu_configure+0x144/0x1e8)
BUT the mtk_iommu_v1.c only supports arm32, the probing flow still is a bit
weird. We always expect create the fwspec internally. otherwise it will
enter here and return fail.
static int mtk_iommu_create_mapping(struct device *dev,
struct of_phandle_args *args)
{
...
if (!fwspec) {
} else if (dev_iommu_fwspec_get(dev)->ops != _iommu_ops) {
>>Enter here. return fail.
return -EINVAL;
}
...
}
Thus, Free the existed fwspec if the master device already has fwspec.
This issue is reported at:
https://lore.kernel.org/linux-mediatek/trinity-7d9ebdc9-4849-4d93-bfb5-429dcb4ee449-1626253158870@3c-app-gmx-bs01/
Reported-by: Frank Wunderlich
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Acked-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu_v1.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index be22fcf988ce..1467ba1e4417 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -425,6 +425,15 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct mtk_iommu_data *data;
int err, idx = 0;
+ /*
+* In the deferred case, free the existed fwspec.
+* Always initialize the fwspec internally.
+*/
+ if (fwspec) {
+ iommu_fwspec_free(dev);
+ fwspec = dev_iommu_fwspec_get(dev);
+ }
+
while (!of_parse_phandle_with_args(dev->of_node, "iommus",
"#iommu-cells",
idx, _spec)) {
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.17 11/66] media: iommu/mediatek-v1: Free the existed fwspec if the master dev already has
From: Yong Wu
[ Upstream commit 822a2ed8c606caf6a11b1a180b8e46292bd77d71 ]
When the iommu master device enters of_iommu_xlate, the ops may be
NULL(iommu dev is defered), then it will initialize the fwspec here:
[] (dev_iommu_fwspec_set) from []
(iommu_fwspec_init+0xbc/0xd4)
[] (iommu_fwspec_init) from []
(of_iommu_xlate+0x7c/0x12c)
[] (of_iommu_xlate) from []
(of_iommu_configure+0x144/0x1e8)
BUT the mtk_iommu_v1.c only supports arm32, the probing flow still is a bit
weird. We always expect create the fwspec internally. otherwise it will
enter here and return fail.
static int mtk_iommu_create_mapping(struct device *dev,
struct of_phandle_args *args)
{
...
if (!fwspec) {
} else if (dev_iommu_fwspec_get(dev)->ops != _iommu_ops) {
>>Enter here. return fail.
return -EINVAL;
}
...
}
Thus, Free the existed fwspec if the master device already has fwspec.
This issue is reported at:
https://lore.kernel.org/linux-mediatek/trinity-7d9ebdc9-4849-4d93-bfb5-429dcb4ee449-1626253158870@3c-app-gmx-bs01/
Reported-by: Frank Wunderlich
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Acked-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu_v1.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index be22fcf988ce..1467ba1e4417 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -425,6 +425,15 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct mtk_iommu_data *data;
int err, idx = 0;
+ /*
+* In the deferred case, free the existed fwspec.
+* Always initialize the fwspec internally.
+*/
+ if (fwspec) {
+ iommu_fwspec_free(dev);
+ fwspec = dev_iommu_fwspec_get(dev);
+ }
+
while (!of_parse_phandle_with_args(dev->of_node, "iommus",
"#iommu-cells",
idx, _spec)) {
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH AUTOSEL 5.17 13/66] media: iommu/mediatek: Add device_link between the consumer and the larb devices
From: Yong Wu
[ Upstream commit 635319a4a7444ca97124d781cd96deb277ff4d40 ]
MediaTek IOMMU-SMI diagram is like below. all the consumer connect with
smi-larb, then connect with smi-common.
M4U
|
smi-common
|
-
| |...
| |
larb1 larb2
| |
vdec venc
When the consumer works, it should enable the smi-larb's power which
also need enable the smi-common's power firstly.
Thus, First of all, use the device link connect the consumer and the
smi-larbs. then add device link between the smi-larb and smi-common.
This patch adds device_link between the consumer and the larbs.
When device_link_add, I add the flag DL_FLAG_STATELESS to avoid calling
pm_runtime_xx to keep the original status of clocks. It can avoid two
issues:
1) Display HW show fastlogo abnormally reported in [1]. At the beggining,
all the clocks are enabled before entering kernel, but the clocks for
display HW(always in larb0) will be gated after clk_enable and clk_disable
called from device_link_add(->pm_runtime_resume) and rpm_idle. The clock
operation happened before display driver probe. At that time, the display
HW will be abnormal.
2) A deadlock issue reported in [2]. Use DL_FLAG_STATELESS to skip
pm_runtime_xx to avoid the deadlock.
Corresponding, DL_FLAG_AUTOREMOVE_CONSUMER can't be added, then
device_link_removed should be added explicitly.
Meanwhile, Currently we don't have a device connect with 2 larbs at the
same time. Disallow this case, print the error log.
[1] https://lore.kernel.org/linux-mediatek/1564213888.22908.4.camel@mhfsdcap03/
[2] https://lore.kernel.org/patchwork/patch/1086569/
Suggested-by: Tomasz Figa
Signed-off-by: Yong Wu
Tested-by: Frank Wunderlich # BPI-R2/MT7623
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 30 ++
drivers/iommu/mtk_iommu_v1.c | 29 -
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 77ae20ff9b35..5971a1168666 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -562,22 +562,52 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct mtk_iommu_data *data;
+ struct device_link *link;
+ struct device *larbdev;
+ unsigned int larbid, larbidx, i;
if (!fwspec || fwspec->ops != _iommu_ops)
return ERR_PTR(-ENODEV); /* Not a iommu client device */
data = dev_iommu_priv_get(dev);
+ /*
+* Link the consumer device with the smi-larb device(supplier).
+* The device that connects with each a larb is a independent HW.
+* All the ports in each a device should be in the same larbs.
+*/
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ for (i = 1; i < fwspec->num_ids; i++) {
+ larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
+ if (larbid != larbidx) {
+ dev_err(dev, "Can only use one larb. Fail@larb%d-%d.\n",
+ larbid, larbidx);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+ larbdev = data->larb_imu[larbid].dev;
+ link = device_link_add(dev, larbdev,
+ DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+ if (!link)
+ dev_err(dev, "Unable to link %s\n", dev_name(larbdev));
return >iommu;
}
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct mtk_iommu_data *data;
+ struct device *larbdev;
+ unsigned int larbid;
if (!fwspec || fwspec->ops != _iommu_ops)
return;
+ data = dev_iommu_priv_get(dev);
+ larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+ larbdev = data->larb_imu[larbid].dev;
+ device_link_remove(dev, larbdev);
+
iommu_fwspec_free(dev);
}
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 68bf02f87cfd..bc7ee90b9373 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -423,7 +423,9 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct of_phandle_args iommu_spec;
struct mtk_iommu_data *data;
- int err, idx = 0;
+ int err, idx = 0, larbid, larbidx;
+ struct device_link *link;
+ struct device *larbdev;
/*
* In the deferred case, free the existed fwspec.
@@ -453,6 +455,23 @@ static struct iommu_device *mtk_iommu_probe_device(struct
device *dev)
data = dev_iommu_priv_get(dev);
+ /* Link the
[PATCH AUTOSEL 5.17 12/66] media: iommu/mediatek: Return ENODEV if the device is NULL
From: Yong Wu
[ Upstream commit 2fb0feed51085db77606de9b9477c96894328809 ]
The platform device is created at:
of_platform_default_populate_init: arch_initcall_sync
->of_platform_populate
->of_platform_device_create_pdata
When entering our probe, all the devices should be already created.
if it is null, means NODEV. Currently we don't get the fail case.
It's a minor fix, no need add fixes tags.
Signed-off-by: Yong Wu
Acked-by: Joerg Roedel
Reviewed-by: AngeloGioacchino Del Regno
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
drivers/iommu/mtk_iommu.c| 2 +-
drivers/iommu/mtk_iommu_v1.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 25b834104790..77ae20ff9b35 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -848,7 +848,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[id].dev = >dev;
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 1467ba1e4417..68bf02f87cfd 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -604,7 +604,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
plarbdev = of_find_device_by_node(larbnode);
if (!plarbdev) {
of_node_put(larbnode);
- return -EPROBE_DEFER;
+ return -ENODEV;
}
data->larb_imu[i].dev = >dev;
--
2.34.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch v1] iommu: arm-smmu: Use arm-smmu-nvidia impl for Tegra234
On Tue, Mar 29, 2022 at 10:14:36AM +0530, Ashish Mhetre wrote: > Tegra234 has 2 pairs of ARM MMU-500 instances. Each pair is used > together and should be programmed identically. > Add compatible string of Tegra234 iommu nodes in arm_smmu_impl_init() > so that arm-smmu-nvidia implementation will be used for programming > these SMMU instances. > > Signed-off-by: Ashish Mhetre > --- > drivers/iommu/arm/arm-smmu/arm-smmu-impl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) I already sent out this patch a couple of months ago, though I realize that it still hasn't been applied: http://patchwork.ozlabs.org/project/linux-tegra/list/?series=276030 Joerg, any chance we can still get that series into v5.18? I've already applied patch 4 given that Rob had acked the DT bindings changes. I know it's a bit late, but this has been on the list for a couple of months and has Rob's Reviewed-by on the bindings and Will's Acked-by on the ARM SMMU driver patches. If it's too late for v5.18, is there anything else you're waiting for so that this can go into v5.19? Thanks, Thierry signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 01/11] iommu: Add pasid_bits field in struct dev_iommu
> From: Lu Baolu
> Sent: Tuesday, March 29, 2022 1:38 PM
>
> Use this field to save the pasid/ssid bits that a device is able to
> support with its IOMMU hardware. It is a generic attribute of a device
> and lifting it into the per-device dev_iommu struct makes it possible
> to allocate a PASID for device without calls into the IOMMU drivers.
> Any iommu driver which suports PASID related features should set this
> field before features are enabled on the devices.
Miss a clarification for non-PCI SVA as discussed here:
https://lore.kernel.org/all/85d61ad6-0cf0-ac65-3312-32d0cdeb1...@linux.intel.com/
>
> Signed-off-by: Lu Baolu
> ---
> include/linux/iommu.h | 1 +
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 ++
> drivers/iommu/intel/iommu.c | 5 -
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 6ef2df258673..36f43af0af53 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -368,6 +368,7 @@ struct dev_iommu {
> struct iommu_fwspec *fwspec;
> struct iommu_device *iommu_dev;
> void*priv;
> + unsigned intpasid_bits;
> };
>
> int iommu_device_register(struct iommu_device *iommu,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 627a3ed5ee8f..afc63fce6107 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2681,6 +2681,8 @@ static struct iommu_device
> *arm_smmu_probe_device(struct device *dev)
> smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> master->stall_enabled = true;
>
> + dev->iommu->pasid_bits = master->ssid_bits;
> +
> return >iommu;
>
> err_free_master:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 6f7485c44a4b..c1b91bce1530 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4587,8 +4587,11 @@ static struct iommu_device
> *intel_iommu_probe_device(struct device *dev)
> if (pasid_supported(iommu)) {
> int features = pci_pasid_features(pdev);
>
> - if (features >= 0)
> + if (features >= 0) {
> info->pasid_supported = features | 1;
> + dev->iommu->pasid_bits =
> + fls(pci_max_pasids(pdev)) - 1;
> + }
> }
>
> if (info->ats_supported && ecap_prs(iommu->ecap)
> &&
> --
> 2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Lu Baolu > Sent: Wednesday, March 30, 2022 1:00 PM > > > > btw I'm not sure whether this is what SVA requires. IIRC the problem with > > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > > a DMA target address with PASID might be treated as P2P if the address > > falls into the MMIO BAR of other devices in the group. This is why the > > original code needs to strictly apply SVA in a group containing a single > > device, instead of a group attached by a single driver, unless we want to > > reserve those MMIO ranges in CPU VA space. > > You are right. But I don't think the IOMMU core is able to guarantee > above in a platform/device-agnostic way. Or any suggestions? > > I guess this should be somewhat off-loaded to the device driver which > knows details of the device. The device driver should know this and > guarantee it before calling > iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA). How would the device driver know whether SVA requests from a device might be mis-interpreted as p2p by upstreaming ports? > > This patch itself just replaces the existing > "iommu_group_device_count(group) != 1" logic with a new one based on the > group ownership logistics. The former is obviously not friendly to > device hot joined afterward. > IMHO this replacement changes the semantics and device hotplug is something that we must deal with... Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH RFC v2 02/11] iommu: Add iommu_group_singleton_lockdown()
> From: Jason Gunthorpe > Sent: Tuesday, March 29, 2022 7:43 PM > > On Tue, Mar 29, 2022 at 08:42:13AM +, Tian, Kevin wrote: > > > btw I'm not sure whether this is what SVA requires. IIRC the problem with > > SVA is because PASID TLP prefix is not counted in PCI packet routing thus > > a DMA target address with PASID might be treated as P2P if the address > > falls into the MMIO BAR of other devices in the group. This is why the > > original code needs to strictly apply SVA in a group containing a single > > device, instead of a group attached by a single driver, unless we want to > > reserve those MMIO ranges in CPU VA space. > > I think it is not such a good idea to mix up group with this test > > Here you want to say that all TLPs from the RID route to the host > bridge - ie ACS is on/etc. This is subtly different from a group with > a single device. Specifically it is an immutable property of the > fabric and doesn't change after hot plug events. Yes, in concept your description is more accurate. However according to pci_device_group() a group is created mainly for lacking of ACS. Without ACS there is no guarantee that all TLPs from the RID in a multi-devices group are routed to the host bridge. In this case only singleton group can meet this requirement. One thing that I'm not very sure is about DMA alias. Even when physically there is only a single device within the group the aliasing could lead to multiple RIDs in the group making it non-singleton. But probably we don't need support SVA on such device until a real demand comes? > > ie if we have a singleton group that doesn't have ACS and someone > hotplugs in another device on a bridge, then our SVA is completely > broken and we get data corruption. Can we capture that in iommu_probe_device() when identifying the group which the probed device will be added to has already been locked down for SVA? i.e. make iommu_group_singleton_lockdown() in this patch to lock down the fact of singleton group instead of the fact of singleton driver... > > Testing the group size is inherently the wrong test to make. > What is your suggestion then? Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
