RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken
Alex, I had to revise the patch. Please see attachment. It is actually two more SSIDs affected to that. Best regards, Edgar -Original Message- From: Merger, Edgar [AUTOSOL/MAS/AUGS] Sent: Dienstag, 8. Dezember 2020 09:23 To: 'Deucher, Alexander' ; 'Huang, Ray' ; 'Kuehling, Felix' Cc: 'Will Deacon' ; 'linux-ker...@vger.kernel.org' ; 'linux-...@vger.kernel.org' ; 'iommu@lists.linux-foundation.org' ; 'Bjorn Helgaas' ; 'Joerg Roedel' ; 'Zhu, Changfeng' Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken Applied the patch as in attachment. Verified that ATS for GPU-Device had been disabled. See attachment "dmesg_ATS.log". Was running that build over night successfully. -Original Message- From: Merger, Edgar [AUTOSOL/MAS/AUGS] Sent: Montag, 7. Dezember 2020 05:53 To: Deucher, Alexander ; Huang, Ray ; Kuehling, Felix Cc: Will Deacon ; linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn Helgaas ; Joerg Roedel ; Zhu, Changfeng Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken Hi Alex, I believe in the patch file, this + (pdev->subsystem_device == 0x0c19 || +pdev->subsystem_device == 0x0c10)) Has to be changed to: + (pdev->subsystem_device == 0xce19 || +pdev->subsystem_device == 0xcc10)) Because our SSIDs are "ea50:ce19" and "ea50:cc10" respectively and another one would "ea50:cc08". I will apply that patch and feedback the results soon plus the patch file that I actually had applied. -Original Message- From: Deucher, Alexander Sent: Montag, 30. November 2020 19:36 To: Merger, Edgar [AUTOSOL/MAS/AUGS] ; Huang, Ray ; Kuehling, Felix Cc: Will Deacon ; linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn Helgaas ; Joerg Roedel ; Zhu, Changfeng Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken [AMD Public Use] > -Original Message- > From: Merger, Edgar [AUTOSOL/MAS/AUGS] > Sent: Thursday, November 26, 2020 4:24 AM > To: Deucher, Alexander ; Huang, Ray > ; Kuehling, Felix > Cc: Will Deacon ; linux-ker...@vger.kernel.org; > linux- p...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn > Helgaas ; Joerg Roedel ; Zhu, > Changfeng > Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as > broken > > Alex, > > This is pretty much the same patch as what I have received from Joerg > previously, except that it is tied to the particular Emerson platform > and its derivatives (listed with Subsystem IDs). Right. As per my original point, I don't want to disable ATS on all Picasso chips because doing so would break GPU compute on them, so I'd like to apply this quirk as narrowly as possible. > > Below patch was what Joerg provided me and I successfully tested. > > This diff to the kernel should do that: > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index > f70692ac79c5..3911b0ec57ba 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5176,6 +5176,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, > 0x6900, quirk_amd_harvest_no_ats); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7312, > quirk_amd_harvest_no_ats); > /* AMD Navi14 dGPU */ > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7340, > quirk_amd_harvest_no_ats); > +/* AMD Raven platform iGPU */ > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, > +quirk_amd_harvest_no_ats); > #endif /* CONFIG_PCI_ATS */ > > /* Freescale PCIe doesn't support MSI in RC mode */ > > So far I have seen this issue on two instances of this chip, but I > must admit that I did test only two of them to this extent, so I guess > it is not a bad chip in particular, but the chips we use are from the > same production lot, so it might be a systematical problem of that production > lot? > > UEFI-Setup shows: > Processor Family: 17h > Procossor Model: 20h - 2Fh > CPUID: 00820F01 > Microcode Patch Level: 8200103 > > Looking at the chip-die I found that this is a fully qualified IP > Silicon (according to Ryzen Embedded R1000 SOC Interlock). > YE1305C9T20FG > BI2015SUY > 9JB6496P00123 > 2016 AMD > DIFFUSED IN USA > MADE IN CHINA > > Currently used SBIOS is a branch from "EmbeddedPI-FP5 1.2.0.3RC3". > > In the future our SBIOS might merge with EmbeddedPI-FP5_1.2.0.5RC3. > I think it's more likely an sbios issue, so hopefully the new release fixes it. Alex > > > > -Original Message- > From: Deucher, Alexander > Sent: Mittwoch, 25. November 2020 17:08 > To: Merger, Edgar [AUTOSOL/MAS/AUGS] ; > Huang, Ray ; Kuehling, Felix > > Cc: Will Deacon ; linux-ker...@vger.kernel.org; > linux- p...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn > Helgaas ; Joerg Roedel ; Zhu, > Changfeng > Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as > broken > > [AMD Public Use] > > > -
[bug report] dma-mapping: add benchmark support for streaming DMA APIs
Hello Barry Song,
The patch 65789daa8087: "dma-mapping: add benchmark support for
streaming DMA APIs" from Nov 16, 2020, leads to the following static
checker warning:
kernel/dma/map_benchmark.c:241 map_benchmark_ioctl()
error: undefined (user controlled) shift '1 << (map->bparam.dma_bits)'
kernel/dma/map_benchmark.c
191 static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
192 unsigned long arg)
193 {
194 struct map_benchmark_data *map = file->private_data;
195 void __user *argp = (void __user *)arg;
196 u64 old_dma_mask;
197
198 int ret;
199
200 if (copy_from_user(&map->bparam, argp, sizeof(map->bparam)))
^
Comes from the user
201 return -EFAULT;
202
203 switch (cmd) {
204 case DMA_MAP_BENCHMARK:
205 if (map->bparam.threads == 0 ||
206 map->bparam.threads > DMA_MAP_MAX_THREADS) {
207 pr_err("invalid thread number\n");
208 return -EINVAL;
209 }
210
211 if (map->bparam.seconds == 0 ||
212 map->bparam.seconds > DMA_MAP_MAX_SECONDS) {
213 pr_err("invalid duration seconds\n");
214 return -EINVAL;
215 }
216
217 if (map->bparam.node != NUMA_NO_NODE &&
218 !node_possible(map->bparam.node)) {
219 pr_err("invalid numa node\n");
220 return -EINVAL;
221 }
222
223 switch (map->bparam.dma_dir) {
224 case DMA_MAP_BIDIRECTIONAL:
225 map->dir = DMA_BIDIRECTIONAL;
226 break;
227 case DMA_MAP_FROM_DEVICE:
228 map->dir = DMA_FROM_DEVICE;
229 break;
230 case DMA_MAP_TO_DEVICE:
231 map->dir = DMA_TO_DEVICE;
232 break;
233 default:
234 pr_err("invalid DMA direction\n");
235 return -EINVAL;
236 }
237
238 old_dma_mask = dma_get_mask(map->dev);
239
240 ret = dma_set_mask(map->dev,
241 DMA_BIT_MASK(map->bparam.dma_bits));
^^
If this is more than 31 then the behavior is undefined (but in real life
it will shift wrap).
242 if (ret) {
243 pr_err("failed to set dma_mask on device %s\n",
244 dev_name(map->dev));
245 return -EINVAL;
246 }
247
248 ret = do_map_benchmark(map);
249
250 /*
251 * restore the original dma_mask as many devices'
dma_mask are
252 * set by architectures, acpi, busses. When we bind
them back
253 * to their original drivers, those drivers shouldn't
see
254 * dma_mask changed by benchmark
255 */
256 dma_set_mask(map->dev, old_dma_mask);
257 break;
258 default:
259 return -EINVAL;
260 }
261
262 if (copy_to_user(argp, &map->bparam, sizeof(map->bparam)))
263 return -EFAULT;
264
265 return ret;
266 }
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v4 1/1] vfio/type1: Add vfio_group_iommu_domain()
Add the API for getting the domain from a vfio group. This could be used
by the physical device drivers which rely on the vfio/mdev framework for
mediated device user level access. The typical use case like below:
unsigned int pasid;
struct vfio_group *vfio_group;
struct iommu_domain *iommu_domain;
struct device *dev = mdev_dev(mdev);
struct device *iommu_device = mdev_get_iommu_device(dev);
if (!iommu_device ||
!iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
return -EINVAL;
vfio_group = vfio_group_get_external_user_from_dev(dev);
if (IS_ERR_OR_NULL(vfio_group))
return -EFAULT;
iommu_domain = vfio_group_iommu_domain(vfio_group);
if (IS_ERR_OR_NULL(iommu_domain)) {
vfio_group_put_external_user(vfio_group);
return -EFAULT;
}
pasid = iommu_aux_get_pasid(iommu_domain, iommu_device);
if (pasid < 0) {
vfio_group_put_external_user(vfio_group);
return -EFAULT;
}
/* Program device context with pasid value. */
...
Signed-off-by: Lu Baolu
---
drivers/vfio/vfio.c | 18 ++
drivers/vfio/vfio_iommu_type1.c | 24
include/linux/vfio.h| 4
3 files changed, 46 insertions(+)
Change log:
- v3:
https://lore.kernel.org/linux-iommu/20201201012328.2465735-1-baolu...@linux.intel.com/
- Changed according to comments @
https://lore.kernel.org/linux-iommu/20201202144834.1dd09...@w520.home/
- Rename group_domain to group_iommu_domain;
- Remove an unnecessary else branch.
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 2151bc7f87ab..4ad8a35667a7 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -2331,6 +2331,24 @@ int vfio_unregister_notifier(struct device *dev, enum
vfio_notify_type type,
}
EXPORT_SYMBOL(vfio_unregister_notifier);
+struct iommu_domain *vfio_group_iommu_domain(struct vfio_group *group)
+{
+ struct vfio_container *container;
+ struct vfio_iommu_driver *driver;
+
+ if (!group)
+ return ERR_PTR(-EINVAL);
+
+ container = group->container;
+ driver = container->iommu_driver;
+ if (likely(driver && driver->ops->group_iommu_domain))
+ return driver->ops->group_iommu_domain(container->iommu_data,
+ group->iommu_group);
+
+ return ERR_PTR(-ENOTTY);
+}
+EXPORT_SYMBOL_GPL(vfio_group_iommu_domain);
+
/**
* Module/class support
*/
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 67e827638995..0b4dedaa9128 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -2980,6 +2980,29 @@ static int vfio_iommu_type1_dma_rw(void *iommu_data,
dma_addr_t user_iova,
return ret;
}
+static struct iommu_domain *
+vfio_iommu_type1_group_iommu_domain(void *iommu_data,
+ struct iommu_group *iommu_group)
+{
+ struct iommu_domain *domain = ERR_PTR(-ENODEV);
+ struct vfio_iommu *iommu = iommu_data;
+ struct vfio_domain *d;
+
+ if (!iommu || !iommu_group)
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&iommu->lock);
+ list_for_each_entry(d, &iommu->domain_list, next) {
+ if (find_iommu_group(d, iommu_group)) {
+ domain = d->domain;
+ break;
+ }
+ }
+ mutex_unlock(&iommu->lock);
+
+ return domain;
+}
+
static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = {
.name = "vfio-iommu-type1",
.owner = THIS_MODULE,
@@ -2993,6 +3016,7 @@ static const struct vfio_iommu_driver_ops
vfio_iommu_driver_ops_type1 = {
.register_notifier = vfio_iommu_type1_register_notifier,
.unregister_notifier= vfio_iommu_type1_unregister_notifier,
.dma_rw = vfio_iommu_type1_dma_rw,
+ .group_iommu_domain = vfio_iommu_type1_group_iommu_domain,
};
static int __init vfio_iommu_type1_init(void)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 38d3c6a8dc7e..f45940b38a02 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -90,6 +90,8 @@ struct vfio_iommu_driver_ops {
struct notifier_block *nb);
int (*dma_rw)(void *iommu_data, dma_addr_t user_iova,
void *data, size_t count, bool write);
+ struct iommu_domain *(*group_iommu_domain)(void *iommu_data,
+ struct iommu_group *group);
};
extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
@@ -126,6 +128,8 @@ extern int vfio_group_unpin_pages(struct vfio_group *group,
extern int
Re: [PATCH v8] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests.
On Tue, Dec 08, 2020 at 06:27:39PM -0500, Konrad Rzeszutek Wilk wrote: > That said if you have the time to take a peek at the x86 bits - that > would be awesome! Sure, tomorrow. Good night. :-) -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests.
On December 8, 2020 6:01:19 PM EST, Borislav Petkov wrote: >On Tue, Dec 08, 2020 at 05:22:20PM -0500, Konrad Rzeszutek Wilk wrote: >> I will fix it up. > >So who's picking this up? If not me then I probably should have a >detailed look at the x86 bits before it goes in... I was planning to pick this up (got one more SWIOTLB related patch). That said if you have the time to take a peek at the x86 bits - that would be awesome! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests.
On Tue, Dec 08, 2020 at 05:22:20PM -0500, Konrad Rzeszutek Wilk wrote: > I will fix it up. So who's picking this up? If not me then I probably should have a detailed look at the x86 bits before it goes in... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests.
On Mon, Dec 07, 2020 at 11:10:57PM +, Ashish Kalra wrote:
> From: Ashish Kalra
>
> For SEV, all DMA to and from guest has to use shared (un-encrypted) pages.
> SEV uses SWIOTLB to make this happen without requiring changes to device
> drivers. However, depending on workload being run, the default 64MB of
> SWIOTLB might not be enough and SWIOTLB may run out of buffers to use
> for DMA, resulting in I/O errors and/or performance degradation for
> high I/O workloads.
>
> Adjust the default size of SWIOTLB for SEV guests using a
> percentage of the total memory available to guest for SWIOTLB buffers.
>
> Using late_initcall() interface to invoke swiotlb_adjust() does not
> work as the size adjustment needs to be done before mem_encrypt_init()
> and reserve_crashkernel() which use the allocated SWIOTLB buffer size,
> hence call it explicitly from setup_arch().
>
> The SWIOTLB default size adjustment needs to be added as an architecture
> specific interface/callback to allow architectures such as those supporting
> memory encryption to adjust/expand SWIOTLB size for their use.
>
> v5 fixed build errors and warnings as
> Reported-by: kbuild test robot
>
> Signed-off-by: Ashish Kalra
> ---
> arch/x86/kernel/setup.c | 2 ++
> arch/x86/mm/mem_encrypt.c | 37 +
> include/linux/swiotlb.h | 6 ++
> kernel/dma/swiotlb.c | 22 ++
> 4 files changed, 67 insertions(+)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 84f581c91db4..31e24e198061 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1149,6 +1149,8 @@ void __init setup_arch(char **cmdline_p)
> if (boot_cpu_has(X86_FEATURE_GBPAGES))
> hugetlb_cma_reserve(PUD_SHIFT - PAGE_SHIFT);
>
> + swiotlb_adjust();
> +
> /*
>* Reserve memory for crash kernel after SRAT is parsed so that it
>* won't consume hotpluggable memory.
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 1bcfbcd2bfd7..d1b8d60040cf 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -485,7 +485,44 @@ static void print_mem_encrypt_feature_info(void)
> pr_cont("\n");
> }
>
> +/*
> + * The percentage of guest memory used here for SWIOTLB buffers
> + * is more of an approximation of the static adjustment which
> + * is 128M for <1G guests, 256M for 1G-4G guests and 512M for >4G guests.
No?
it is 64MB for <1G, and ~128M to 256M for 1G-to-4G
I will fix it up.
> + */
> +#define SEV_ADJUST_SWIOTLB_SIZE_PERCENT 6
> +
> /* Architecture __weak replacement functions */
> +unsigned long __init arch_swiotlb_adjust(unsigned long iotlb_default_size)
> +{
> + unsigned long size = iotlb_default_size;
> +
> + /*
> + * For SEV, all DMA has to occur via shared/unencrypted pages.
> + * SEV uses SWOTLB to make this happen without changing device
> + * drivers. However, depending on the workload being run, the
> + * default 64MB of SWIOTLB may not be enough and`SWIOTLB may
> + * run out of buffers for DMA, resulting in I/O errors and/or
> + * performance degradation especially with high I/O workloads.
> + * Adjust the default size of SWIOTLB for SEV guests using
> + * a percentage of guest memory for SWIOTLB buffers.
> + * Also as the SWIOTLB bounce buffer memory is allocated
> + * from low memory, ensure that the adjusted size is within
> + * the limits of low available memory.
> + *
> + */
> + if (sev_active()) {
> + phys_addr_t total_mem = memblock_phys_mem_size();
> +
> + size = total_mem * SEV_ADJUST_SWIOTLB_SIZE_PERCENT / 100;
> + size = clamp_val(size, iotlb_default_size, SZ_1G);
> + pr_info("SWIOTLB bounce buffer size adjusted to %luMB for SEV",
> + size >> 20);
> + }
> +
> + return size;
> +}
> +
> void __init mem_encrypt_init(void)
> {
> if (!sme_me_mask)
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 3bb72266a75a..b5904fa4b67c 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -33,6 +33,7 @@ extern void swiotlb_init(int verbose);
> int swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose);
> extern unsigned long swiotlb_nr_tbl(void);
> unsigned long swiotlb_size_or_default(void);
> +unsigned long __init arch_swiotlb_adjust(unsigned long size);
> extern int swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs);
> extern int swiotlb_late_init_with_default_size(size_t default_size);
> extern void __init swiotlb_update_mem_attributes(void);
> @@ -77,6 +78,7 @@ void __init swiotlb_exit(void);
> unsigned int swiotlb_max_segment(void);
> size_t swiotlb_max_mapping_size(struct device *dev);
> bool is_swiotlb_active(void);
> +void __init swiotlb_adjust(void);
> #else
> #define swiotlb_force SWIOTLB_NO_FORCE
> static inline bool is_swi
Re: [PATCH] [PATCH] Keep offset when mapping data via SWIOTLB.
On Mon, Dec 07, 2020 at 01:42:04PM -0800, Jianxiong Gao wrote: > NVMe driver and other applications depend on the data offset > to operate correctly. Currently when unaligned data is mapped via > SWIOTLB, the data is mapped as slab aligned with the SWIOTLB. When > booting with --swiotlb=force option and using NVMe as interface, > running mkfs.xfs on Rhel fails because of the unalignment issue. > This patch makes sure the mapped data preserves > its offset of the orginal address. Tested on latest kernel that > this patch fixes the issue. Lets reword this comment a bit more since you are not providing the RHEL Bug, and instead are focusing on the upstream kernel. I can do that for you.. > > Signed-off-by: Jianxiong Gao > Acked-by: David Rientjes > --- > kernel/dma/swiotlb.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c > index 781b9dca197c..56a35e71b3fd 100644 > --- a/kernel/dma/swiotlb.c > +++ b/kernel/dma/swiotlb.c > @@ -483,6 +483,12 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > phys_addr_t orig_addr, > max_slots = mask + 1 > ? ALIGN(mask + 1, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT > : 1UL << (BITS_PER_LONG - IO_TLB_SHIFT); > + > + /* > + * We need to keep the offset when mapping, so adding the offset > + * to the total set we need to allocate in SWIOTLB > + */ > + alloc_size += offset_in_page(orig_addr); > > /* >* For mappings greater than or equal to a page, we limit the stride > @@ -567,6 +573,11 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, > phys_addr_t orig_addr, >*/ > for (i = 0; i < nslots; i++) > io_tlb_orig_addr[index+i] = orig_addr + (i << IO_TLB_SHIFT); > + /* > + * When keeping the offset of the original data, we need to advance > + * the tlb_addr by the offset of orig_addr. > + */ > + tlb_addr += orig_addr & (PAGE_SIZE - 1); > if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) && > (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)) > swiotlb_bounce(orig_addr, tlb_addr, mapping_size, > DMA_TO_DEVICE); > -- > 2.27.0 > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: Defer the early return in arm_(v7s/lpae)_map
On Mon, 7 Dec 2020 19:57:58 +0800, Keqian Zhu wrote: > Although handling a mapping request with no permissions is a > trivial no-op, defer the early return until after the size/range > checks so that we are consistent with other mapping requests. Applied to arm64 (for-next/iommu/misc), thanks! [1/1] iommu: Defer the early return in arm_(v7s/lpae)_map https://git.kernel.org/arm64/c/f12e0d22903e Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] IOMMU: Some IOVA code tidy-up
On Fri, 4 Dec 2020 02:34:49 +0800, John Garry wrote: > This series contains some minor tidy-ups by deleting an unreferenced > function and unexporting some functions, highlighted by: > https://lore.kernel.org/linux-iommu/6e09d847-fb7f-1ec1-02bf-f0c8b3158...@huawei.com/T/#med5a019f9d3835c162c16a48f34d05cc0111b0ca > > John Garry (3): > iommu: Delete split_and_remove_iova() > iommu: Stop exporting alloc_iova_mem() > iommu: Stop exporting free_iova_mem() > > [...] Applied to arm64 (for-next/iommu/iova), thanks! [1/3] iommu: Delete split_and_remove_iova() https://git.kernel.org/arm64/c/2f24dfb71208 [2/3] iommu: Stop exporting alloc_iova_mem() https://git.kernel.org/arm64/c/51b70b817b18 [3/3] iommu: Stop exporting free_iova_mem() https://git.kernel.org/arm64/c/176cfc187c24 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/io-pgtable: Remove tlb_flush_leaf
On Wed, 25 Nov 2020 17:29:39 +, Robin Murphy wrote: > The only user of tlb_flush_leaf is a particularly hairy corner of the > Arm short-descriptor code, which wants a synchronous invalidation to > minimise the races inherent in trying to split a large page mapping. > This is already far enough into "here be dragons" territory that no > sensible caller should ever hit it, and thus it really doesn't need > optimising. Although using tlb_flush_walk there may technically be > more heavyweight than needed, it does the job and saves everyone else > having to carry around useless baggage. Applied to arm64 (for-next/iommu/core), thanks! [1/1] iommu/io-pgtable: Remove tlb_flush_leaf https://git.kernel.org/arm64/c/fefe8527a1e0 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RESEND] iommu/io-pgtalbe-arm: Remove "iopte_type(pte, l)" extra parameter "l"
On Mon, 7 Dec 2020 20:01:50 +0800, Kunkun Jiang wrote: > Knowing from the code, the macro "iopte_type(pte, l)" doesn't use the > parameter "l" (level). So we'd better to remove it. Applied to arm64 (for-next/iommu/misc), thanks! [1/1] iommu/io-pgtalbe-arm: Remove "iopte_type(pte, l)" extra parameter "l" https://git.kernel.org/arm64/c/f37eb48466d2 Cheers, -- Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: Defer the early return in arm_(v7s/lpae)_map
On 2020-12-07 11:57, Keqian Zhu wrote: Although handling a mapping request with no permissions is a trivial no-op, defer the early return until after the size/range checks so that we are consistent with other mapping requests. Reviewed-by: Robin Murphy Signed-off-by: Keqian Zhu --- drivers/iommu/io-pgtable-arm-v7s.c | 8 drivers/iommu/io-pgtable-arm.c | 8 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c index a688f22cbe3b..359b96b0fa3e 100644 --- a/drivers/iommu/io-pgtable-arm-v7s.c +++ b/drivers/iommu/io-pgtable-arm-v7s.c @@ -522,14 +522,14 @@ static int arm_v7s_map(struct io_pgtable_ops *ops, unsigned long iova, struct io_pgtable *iop = &data->iop; int ret; - /* If no access, then nothing to do */ - if (!(prot & (IOMMU_READ | IOMMU_WRITE))) - return 0; - if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) || paddr >= (1ULL << data->iop.cfg.oas))) return -ERANGE; + /* If no access, then nothing to do */ + if (!(prot & (IOMMU_READ | IOMMU_WRITE))) + return 0; + ret = __arm_v7s_map(data, iova, paddr, size, prot, 1, data->pgd, gfp); /* * Synchronise all PTE updates for the new mapping before there's diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c index a7a9bc08dcd1..8ade72adab31 100644 --- a/drivers/iommu/io-pgtable-arm.c +++ b/drivers/iommu/io-pgtable-arm.c @@ -444,10 +444,6 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, arm_lpae_iopte prot; long iaext = (s64)iova >> cfg->ias; - /* If no access, then nothing to do */ - if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) - return 0; - if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size)) return -EINVAL; @@ -456,6 +452,10 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, if (WARN_ON(iaext || paddr >> cfg->oas)) return -ERANGE; + /* If no access, then nothing to do */ + if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) + return 0; + prot = arm_lpae_prot_to_pte(data, iommu_prot); ret = __arm_lpae_map(data, iova, paddr, size, prot, lvl, ptep, gfp); /* ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu-v3: Fix not checking return value about devm_add_action
On Tue, Dec 08, 2020 at 08:15:22PM +0800, Tian Tao wrote:
> Use devm_add_action_or_reset to avoid the situation where the release
> function is not called when devm_add_action returns an error.
>
> Signed-off-by: Tian Tao
> ---
> v2:
> repositioning devm_add_action_or_reset in the function
> arm_smmu_setup_msis, and check the return value.
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> 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 2ddf5ec..b4d3b7d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2680,7 +2680,8 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device
> *smmu)
> ret = -ENOMEM;
> } else {
> cmdq->valid_map = bitmap;
> - devm_add_action(smmu->dev, arm_smmu_cmdq_free_bitmap, bitmap);
> + ret = devm_add_action_or_reset(smmu->dev,
> +arm_smmu_cmdq_free_bitmap,
> bitmap);
> }
>
> return ret;
> @@ -2921,6 +2922,13 @@ static void arm_smmu_setup_msis(struct arm_smmu_device
> *smmu)
> return;
> }
>
> + /* Add callback to free MSIs on teardown */
> + ret = devm_add_action_or_reset(dev, arm_smmu_free_msis, dev);
> + if (ret) {
> + dev_warn(dev, "failed to add callback to free MSIs on
> teardown\n");
> + return;
Honestly, wouldn't we just be better leaking memory in this case? Tearing
down the SMMU is a pretty specialist sport _anyway_, but this seems to throw
the baby out with the bath water by failing to initialise because we can't
defer freeing something that we've already allocated. I think we're better
off continuing and trying to get the device up and running.
In fact, the same applies to the cmdq 'valid_map' too -- why do we care?
WIll
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/arm-smmu-v3: Fix not checking return value about devm_add_action
Use devm_add_action_or_reset to avoid the situation where the release
function is not called when devm_add_action returns an error.
Signed-off-by: Tian Tao
---
v2:
repositioning devm_add_action_or_reset in the function
arm_smmu_setup_msis, and check the return value.
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
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 2ddf5ec..b4d3b7d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2680,7 +2680,8 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device
*smmu)
ret = -ENOMEM;
} else {
cmdq->valid_map = bitmap;
- devm_add_action(smmu->dev, arm_smmu_cmdq_free_bitmap, bitmap);
+ ret = devm_add_action_or_reset(smmu->dev,
+ arm_smmu_cmdq_free_bitmap,
bitmap);
}
return ret;
@@ -2921,6 +2922,13 @@ static void arm_smmu_setup_msis(struct arm_smmu_device
*smmu)
return;
}
+ /* Add callback to free MSIs on teardown */
+ ret = devm_add_action_or_reset(dev, arm_smmu_free_msis, dev);
+ if (ret) {
+ dev_warn(dev, "failed to add callback to free MSIs on
teardown\n");
+ return;
+ }
+
for_each_msi_entry(desc, dev) {
switch (desc->platform.msi_index) {
case EVTQ_MSI_INDEX:
@@ -2936,9 +2944,6 @@ static void arm_smmu_setup_msis(struct arm_smmu_device
*smmu)
continue;
}
}
-
- /* Add callback to free MSIs on teardown */
- devm_add_action(dev, arm_smmu_free_msis, dev);
}
static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken
Applied the patch as in attachment. Verified that ATS for GPU-Device had been disabled. See attachment "dmesg_ATS.log". Was running that build over night successfully. -Original Message- From: Merger, Edgar [AUTOSOL/MAS/AUGS] Sent: Montag, 7. Dezember 2020 05:53 To: Deucher, Alexander ; Huang, Ray ; Kuehling, Felix Cc: Will Deacon ; linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn Helgaas ; Joerg Roedel ; Zhu, Changfeng Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken Hi Alex, I believe in the patch file, this + (pdev->subsystem_device == 0x0c19 || +pdev->subsystem_device == 0x0c10)) Has to be changed to: + (pdev->subsystem_device == 0xce19 || +pdev->subsystem_device == 0xcc10)) Because our SSIDs are "ea50:ce19" and "ea50:cc10" respectively and another one would "ea50:cc08". I will apply that patch and feedback the results soon plus the patch file that I actually had applied. -Original Message- From: Deucher, Alexander Sent: Montag, 30. November 2020 19:36 To: Merger, Edgar [AUTOSOL/MAS/AUGS] ; Huang, Ray ; Kuehling, Felix Cc: Will Deacon ; linux-ker...@vger.kernel.org; linux-...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn Helgaas ; Joerg Roedel ; Zhu, Changfeng Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as broken [AMD Public Use] > -Original Message- > From: Merger, Edgar [AUTOSOL/MAS/AUGS] > Sent: Thursday, November 26, 2020 4:24 AM > To: Deucher, Alexander ; Huang, Ray > ; Kuehling, Felix > Cc: Will Deacon ; linux-ker...@vger.kernel.org; > linux- p...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn > Helgaas ; Joerg Roedel ; Zhu, > Changfeng > Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as > broken > > Alex, > > This is pretty much the same patch as what I have received from Joerg > previously, except that it is tied to the particular Emerson platform > and its derivatives (listed with Subsystem IDs). Right. As per my original point, I don't want to disable ATS on all Picasso chips because doing so would break GPU compute on them, so I'd like to apply this quirk as narrowly as possible. > > Below patch was what Joerg provided me and I successfully tested. > > This diff to the kernel should do that: > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index > f70692ac79c5..3911b0ec57ba 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5176,6 +5176,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, > 0x6900, quirk_amd_harvest_no_ats); > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7312, > quirk_amd_harvest_no_ats); > /* AMD Navi14 dGPU */ > DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x7340, > quirk_amd_harvest_no_ats); > +/* AMD Raven platform iGPU */ > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x15d8, > +quirk_amd_harvest_no_ats); > #endif /* CONFIG_PCI_ATS */ > > /* Freescale PCIe doesn't support MSI in RC mode */ > > So far I have seen this issue on two instances of this chip, but I > must admit that I did test only two of them to this extent, so I guess > it is not a bad chip in particular, but the chips we use are from the > same production lot, so it might be a systematical problem of that production > lot? > > UEFI-Setup shows: > Processor Family: 17h > Procossor Model: 20h - 2Fh > CPUID: 00820F01 > Microcode Patch Level: 8200103 > > Looking at the chip-die I found that this is a fully qualified IP > Silicon (according to Ryzen Embedded R1000 SOC Interlock). > YE1305C9T20FG > BI2015SUY > 9JB6496P00123 > 2016 AMD > DIFFUSED IN USA > MADE IN CHINA > > Currently used SBIOS is a branch from "EmbeddedPI-FP5 1.2.0.3RC3". > > In the future our SBIOS might merge with EmbeddedPI-FP5_1.2.0.5RC3. > I think it's more likely an sbios issue, so hopefully the new release fixes it. Alex > > > > -Original Message- > From: Deucher, Alexander > Sent: Mittwoch, 25. November 2020 17:08 > To: Merger, Edgar [AUTOSOL/MAS/AUGS] ; > Huang, Ray ; Kuehling, Felix > > Cc: Will Deacon ; linux-ker...@vger.kernel.org; > linux- p...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn > Helgaas ; Joerg Roedel ; Zhu, > Changfeng > Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as > broken > > [AMD Public Use] > > > -Original Message- > > From: Merger, Edgar [AUTOSOL/MAS/AUGS] > > > Sent: Wednesday, November 25, 2020 5:04 AM > > To: Deucher, Alexander ; Huang, Ray > > ; Kuehling, Felix > > Cc: Will Deacon ; linux-ker...@vger.kernel.org; > > linux- p...@vger.kernel.org; iommu@lists.linux-foundation.org; Bjorn > > Helgaas ; Joerg Roedel ; Zhu, > > Changfeng > > Subject: RE: [EXTERNAL] Re: [PATCH] PCI: Mark AMD Raven iGPU ATS as > > broken > > > > I do have also other problems with this unit, when IOMMU is enabled > > and pci=noats is not set as
