Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
Em Wed, 09 Sep 2020 13:06:39 -0700
Joe Perches escreveu:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
>
> drivers/media/dvb-frontends/drxd_hard.c | 2 +-
> drivers/media/i2c/ov5640.c| 2 +-
> drivers/media/i2c/ov6650.c| 5 ++---
> drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
> drivers/media/i2c/tvp5150.c | 2 +-
> drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
> drivers/media/usb/cpia2/cpia2_core.c | 2 +-
For media drivers:
Reviewed-by: Mauro Carvalho Chehab
Thanks,
Mauro
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/3] dma-mapping: introduce DMA range map, supplanting dma_pfn_offset
From: Jim Quinlan
The new field 'dma_range_map' in struct device is used to facilitate the
use of single or multiple offsets between mapping regions of cpu addrs and
dma addrs. It subsumes the role of "dev->dma_pfn_offset" which was only
capable of holding a single uniform offset and had no region bounds
checking.
The function of_dma_get_range() has been modified so that it takes a single
argument -- the device node -- and returns a map, NULL, or an error code.
The map is an array that holds the information regarding the DMA regions.
Each range entry contains the address offset, the cpu_start address, the
dma_start address, and the size of the region.
of_dma_configure() is the typical manner to set range offsets but there are
a number of ad hoc assignments to "dev->dma_pfn_offset" in the kernel
driver code. These cases now invoke the function
dma_attach_offset_range(dev, cpu_addr, dma_addr, size).
Signed-off-by: Jim Quinlan
[hch: various interface cleanups]
Signed-off-by: Christoph Hellwig
Tested-by: Nathan Chancellor
---
arch/arm/include/asm/dma-direct.h | 9 +--
arch/arm/mach-keystone/keystone.c | 17 ++--
arch/sh/drivers/pci/pcie-sh7786.c | 9 ++-
arch/x86/pci/sta2x11-fixup.c | 6 +-
drivers/acpi/arm64/iort.c | 6 +-
drivers/base/core.c | 2 +
drivers/gpu/drm/sun4i/sun4i_backend.c | 8 +-
drivers/iommu/io-pgtable-arm.c| 2 +-
.../platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++-
.../platform/sunxi/sun6i-csi/sun6i_csi.c | 11 ++-
drivers/of/address.c | 73 -
drivers/of/device.c | 44 ++
drivers/of/of_private.h | 11 +--
drivers/of/unittest.c | 34 +---
drivers/remoteproc/remoteproc_core.c | 4 +-
.../staging/media/sunxi/cedrus/cedrus_hw.c| 10 ++-
drivers/usb/core/message.c| 5 +-
drivers/usb/core/usb.c| 3 +-
include/linux/device.h| 4 +-
include/linux/dma-direct.h| 52 ++--
include/linux/dma-mapping.h | 19 -
kernel/dma/coherent.c | 7 +-
kernel/dma/direct.c | 81 ++-
23 files changed, 303 insertions(+), 123 deletions(-)
diff --git a/arch/arm/include/asm/dma-direct.h
b/arch/arm/include/asm/dma-direct.h
index de0f4ff9279615..a443d5257a21ed 100644
--- a/arch/arm/include/asm/dma-direct.h
+++ b/arch/arm/include/asm/dma-direct.h
@@ -16,8 +16,8 @@
#ifndef __arch_pfn_to_dma
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
- if (dev)
- pfn -= dev->dma_pfn_offset;
+ if (dev && dev->dma_range_map)
+ pfn = PFN_DOWN(translate_phys_to_dma(dev, PFN_PHYS(pfn)));
return (dma_addr_t)__pfn_to_bus(pfn);
}
@@ -25,9 +25,8 @@ static inline unsigned long dma_to_pfn(struct device *dev,
dma_addr_t addr)
{
unsigned long pfn = __bus_to_pfn(addr);
- if (dev)
- pfn += dev->dma_pfn_offset;
-
+ if (dev && dev->dma_range_map)
+ pfn = PFN_DOWN(translate_dma_to_phys(dev, PFN_PHYS(pfn)));
return pfn;
}
diff --git a/arch/arm/mach-keystone/keystone.c
b/arch/arm/mach-keystone/keystone.c
index dcd031ba84c2e0..09a65c2dfd7327 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -8,6 +8,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -25,8 +26,6 @@
#include "keystone.h"
#ifdef CONFIG_ARM_LPAE
-static unsigned long keystone_dma_pfn_offset __read_mostly;
-
static int keystone_platform_notifier(struct notifier_block *nb,
unsigned long event, void *data)
{
@@ -39,9 +38,12 @@ static int keystone_platform_notifier(struct notifier_block
*nb,
return NOTIFY_BAD;
if (!dev->of_node) {
- dev->dma_pfn_offset = keystone_dma_pfn_offset;
- dev_err(dev, "set dma_pfn_offset%08lx\n",
- dev->dma_pfn_offset);
+ int ret = dma_direct_set_offset(dev, KEYSTONE_HIGH_PHYS_START,
+ KEYSTONE_LOW_PHYS_START,
+ KEYSTONE_HIGH_PHYS_SIZE);
+ dev_err(dev, "set dma_offset%08llx%s\n",
+ KEYSTONE_HIGH_PHYS_START - KEYSTONE_LOW_PHYS_START,
+ ret ? " failed" : "");
}
return NOTIFY_OK;
}
@@ -54,11 +56,8 @@ static struct notifier_block platform_nb = {
static void __init keystone_init(void)
{
#ifdef CONFIG_ARM_LPAE
- if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) {
- keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
- KEYSTONE_LOW_
[PATCH 1/3] ARM/dma-mapping: move various helpers from dma-mapping.h to dma-direct.h
Move the helpers to translate to and from direct mapping DMA addresses
to dma-direct.h. This not only is the most logical place, but the new
placement also avoids dependency loops with pending commits.
Signed-off-by: Christoph Hellwig
---
arch/arm/common/dmabounce.c| 2 +-
arch/arm/include/asm/dma-direct.h | 70 ++
arch/arm/include/asm/dma-mapping.h | 70 --
3 files changed, 71 insertions(+), 71 deletions(-)
diff --git a/arch/arm/common/dmabounce.c b/arch/arm/common/dmabounce.c
index f4b719bde76367..d3e00ea9208834 100644
--- a/arch/arm/common/dmabounce.c
+++ b/arch/arm/common/dmabounce.c
@@ -24,7 +24,7 @@
#include
#include
#include
-#include
+#include
#include
#include
#include
diff --git a/arch/arm/include/asm/dma-direct.h
b/arch/arm/include/asm/dma-direct.h
index 7c3001a6a775bf..de0f4ff9279615 100644
--- a/arch/arm/include/asm/dma-direct.h
+++ b/arch/arm/include/asm/dma-direct.h
@@ -2,6 +2,76 @@
#ifndef ASM_ARM_DMA_DIRECT_H
#define ASM_ARM_DMA_DIRECT_H 1
+#include
+
+#ifdef __arch_page_to_dma
+#error Please update to __arch_pfn_to_dma
+#endif
+
+/*
+ * dma_to_pfn/pfn_to_dma/dma_to_virt/virt_to_dma are architecture private
+ * functions used internally by the DMA-mapping API to provide DMA
+ * addresses. They must not be used by drivers.
+ */
+#ifndef __arch_pfn_to_dma
+static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
+{
+ if (dev)
+ pfn -= dev->dma_pfn_offset;
+ return (dma_addr_t)__pfn_to_bus(pfn);
+}
+
+static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
+{
+ unsigned long pfn = __bus_to_pfn(addr);
+
+ if (dev)
+ pfn += dev->dma_pfn_offset;
+
+ return pfn;
+}
+
+static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
+{
+ if (dev) {
+ unsigned long pfn = dma_to_pfn(dev, addr);
+
+ return phys_to_virt(__pfn_to_phys(pfn));
+ }
+
+ return (void *)__bus_to_virt((unsigned long)addr);
+}
+
+static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
+{
+ if (dev)
+ return pfn_to_dma(dev, virt_to_pfn(addr));
+
+ return (dma_addr_t)__virt_to_bus((unsigned long)(addr));
+}
+
+#else
+static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
+{
+ return __arch_pfn_to_dma(dev, pfn);
+}
+
+static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
+{
+ return __arch_dma_to_pfn(dev, addr);
+}
+
+static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
+{
+ return __arch_dma_to_virt(dev, addr);
+}
+
+static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
+{
+ return __arch_virt_to_dma(dev, addr);
+}
+#endif
+
static inline dma_addr_t __phys_to_dma(struct device *dev, phys_addr_t paddr)
{
unsigned int offset = paddr & ~PAGE_MASK;
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index bdd80ddbca3451..0a1a536368c3a4 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -8,8 +8,6 @@
#include
#include
-#include
-
#include
#include
@@ -23,74 +21,6 @@ static inline const struct dma_map_ops
*get_arch_dma_ops(struct bus_type *bus)
return NULL;
}
-#ifdef __arch_page_to_dma
-#error Please update to __arch_pfn_to_dma
-#endif
-
-/*
- * dma_to_pfn/pfn_to_dma/dma_to_virt/virt_to_dma are architecture private
- * functions used internally by the DMA-mapping API to provide DMA
- * addresses. They must not be used by drivers.
- */
-#ifndef __arch_pfn_to_dma
-static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
-{
- if (dev)
- pfn -= dev->dma_pfn_offset;
- return (dma_addr_t)__pfn_to_bus(pfn);
-}
-
-static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
-{
- unsigned long pfn = __bus_to_pfn(addr);
-
- if (dev)
- pfn += dev->dma_pfn_offset;
-
- return pfn;
-}
-
-static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
-{
- if (dev) {
- unsigned long pfn = dma_to_pfn(dev, addr);
-
- return phys_to_virt(__pfn_to_phys(pfn));
- }
-
- return (void *)__bus_to_virt((unsigned long)addr);
-}
-
-static inline dma_addr_t virt_to_dma(struct device *dev, void *addr)
-{
- if (dev)
- return pfn_to_dma(dev, virt_to_pfn(addr));
-
- return (dma_addr_t)__virt_to_bus((unsigned long)(addr));
-}
-
-#else
-static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
-{
- return __arch_pfn_to_dma(dev, pfn);
-}
-
-static inline unsigned long dma_to_pfn(struct device *dev, dma_addr_t addr)
-{
- return __arch_dma_to_pfn(dev, addr);
-}
-
-static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
-{
- return __arch_dma_to_virt(dev, addr);
-}
-
-static inline dma_ad
support range based offsets in dma-direct
Hi all, this series adds range-based offsets to the dma-direct implementation. The guts of the change are a patch from Jim with some modifications from me, but to do it nicely we need to ARM patches to prepare for it as well. Diffstat: arch/arm/common/dmabounce.c|2 arch/arm/include/asm/dma-direct.h | 69 + arch/arm/include/asm/dma-mapping.h | 70 -- arch/arm/mach-keystone/keystone.c | 21 +++-- arch/sh/drivers/pci/pcie-sh7786.c |9 +- arch/x86/pci/sta2x11-fixup.c |6 + drivers/acpi/arm64/iort.c |6 + drivers/base/core.c|2 drivers/gpu/drm/sun4i/sun4i_backend.c |8 +- drivers/iommu/io-pgtable-arm.c |2 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c |9 ++ drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 11 ++ drivers/of/address.c | 73 -- drivers/of/device.c| 44 +++ drivers/of/of_private.h| 11 +- drivers/of/unittest.c | 34 ++-- drivers/remoteproc/remoteproc_core.c |4 - drivers/staging/media/sunxi/cedrus/cedrus_hw.c | 10 ++ drivers/usb/core/message.c |5 - drivers/usb/core/usb.c |3 include/linux/device.h |4 - include/linux/dma-direct.h | 52 +++-- include/linux/dma-mapping.h| 19 kernel/dma/coherent.c |7 - kernel/dma/direct.c| 81 - 25 files changed, 373 insertions(+), 189 deletions(-) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/3] ARM/keystone: move the DMA offset handling under ifdef CONFIG_ARM_LPAE
The DMA offset notifier can only be used if PHYS_OFFSET is at least
KEYSTONE_HIGH_PHYS_START, which can't be represented by a 32-bit
phys_addr_t. Currently the code compiles fine despite that, a pending
change to the DMA offset handling would create a compiler warning for
this case. Add an ifdef to not compile the code except for LPAE
configs.
Signed-off-by: Christoph Hellwig
---
arch/arm/mach-keystone/keystone.c | 4
1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-keystone/keystone.c
b/arch/arm/mach-keystone/keystone.c
index 638808c4e12247..dcd031ba84c2e0 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -24,6 +24,7 @@
#include "keystone.h"
+#ifdef CONFIG_ARM_LPAE
static unsigned long keystone_dma_pfn_offset __read_mostly;
static int keystone_platform_notifier(struct notifier_block *nb,
@@ -48,14 +49,17 @@ static int keystone_platform_notifier(struct notifier_block
*nb,
static struct notifier_block platform_nb = {
.notifier_call = keystone_platform_notifier,
};
+#endif /* CONFIG_ARM_LPAE */
static void __init keystone_init(void)
{
+#ifdef CONFIG_ARM_LPAE
if (PHYS_OFFSET >= KEYSTONE_HIGH_PHYS_START) {
keystone_dma_pfn_offset = PFN_DOWN(KEYSTONE_HIGH_PHYS_START -
KEYSTONE_LOW_PHYS_START);
bus_register_notifier(&platform_bus_type, &platform_nb);
}
+#endif
keystone_pm_runtime_init();
}
--
2.28.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 7/9] iommu/vt-d: Listen to IOASID notifications
On Tue, 1 Sep 2020 19:03:23 +0200
Auger Eric wrote:
> Hi Jacob,
>
> On 8/22/20 6:35 AM, Jacob Pan wrote:
> > On Intel Scalable I/O Virtualization (SIOV) enabled platforms, IOMMU
> > driver is one of the users of IOASIDs. In normal flow, callers will
> > perform IOASID allocation, bind, unbind, and free in order. However, for
> > guest SVA, IOASID free could come before unbind as guest is untrusted.
> > This patch registers IOASID notification handler such that IOMMU driver
> > can perform PASID teardown upon receiving an unexpected IOASID free
> > event.
> >
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/intel/svm.c | 74
> > -
> > include/linux/intel-iommu.h | 2 ++
> > 2 files changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> > index 634e191ca2c3..600e3ae5b656 100644
> > --- a/drivers/iommu/intel/svm.c
> > +++ b/drivers/iommu/intel/svm.c
> > @@ -95,6 +95,72 @@ static inline bool intel_svm_capable(struct intel_iommu
> > *iommu)
> > return iommu->flags & VTD_FLAG_SVM_CAPABLE;
> > }
> >
> > +#define pasid_lock_held() lock_is_held(&pasid_mutex.dep_map)
> put after the pasid_mutex definition?
yes,
> > +static DEFINE_MUTEX(pasid_mutex);
> > +
> > +static void intel_svm_free_async_fn(struct work_struct *work)
> > +{
> > + struct intel_svm *svm = container_of(work, struct intel_svm, work);
> > + struct intel_svm_dev *sdev;
> > +
> > + /*
> > +* Unbind all devices associated with this PASID which is
> > +* being freed by other users such as VFIO.
> > +*/
> > + mutex_lock(&pasid_mutex);
> > + list_for_each_entry_rcu(sdev, &svm->devs, list, pasid_lock_held()) {
> > + /* Does not poison forward pointer */
> > + list_del_rcu(&sdev->list);
> > + spin_lock(&svm->iommu->lock);
> > + intel_pasid_tear_down_entry(svm->iommu, sdev->dev,
> > + svm->pasid, true);
> > + spin_unlock(&svm->iommu->lock);
> > + kfree_rcu(sdev, rcu);
> > + /*
> > +* Free before unbind only happens with guest usaged
> usaged?
used. I meant host PASID used to back guest PASID. I will reword the comments:
/*
* Free before unbind can only happen with host PASIDs used for
* guest SVM. We get here because ioasid_free is called with
* outstanding references. So we need to drop the reference
* such that the PASID can be reclaimed. Once refcount reaches
* zero, IOASID core will detach the private data and erase the
* IOASID entry.
*/
> > +* host PASIDs. IOASID free will detach private data
> > +* and free the IOASID entry.
> > +*/
> > + ioasid_put(NULL, svm->pasid);
> > + if (list_empty(&svm->devs))
> > + kfree(svm);
> > + }
> > + mutex_unlock(&pasid_mutex);
> > +}
> > +
> > +
> > +static int pasid_status_change(struct notifier_block *nb,
> > + unsigned long code, void *data)
> > +{
> > + struct ioasid_nb_args *args = (struct ioasid_nb_args *)data;
> > + struct intel_svm *svm = (struct intel_svm *)args->pdata;
> > + int ret = NOTIFY_DONE;
> > +
> > + if (code == IOASID_FREE) {
> > + if (!svm)
> > + goto done;
> > + if (args->id != svm->pasid) {
> > + pr_warn("Notify PASID does not match data %d : %d\n",
> > + args->id, svm->pasid);
> > + goto done;
> > + }
> > + schedule_work(&svm->work);
> > + return NOTIFY_OK;
> > + }
> > +done:
> > + return ret;> +}
> > +
> > +static struct notifier_block pasid_nb = {
> > + .notifier_call = pasid_status_change,
> > +};
> > +
> > +void intel_svm_add_pasid_notifier(void)
> > +{
> > + /* Listen to all PASIDs, not specific to a set */
> > + ioasid_register_notifier(NULL, &pasid_nb);
> > +}
> > +
> > void intel_svm_check(struct intel_iommu *iommu)
> > {
> > if (!pasid_supported(iommu))
> > @@ -221,7 +287,6 @@ static const struct mmu_notifier_ops intel_mmuops = {
> > .invalidate_range = intel_invalidate_range,
> > };
> >
> > -static DEFINE_MUTEX(pasid_mutex);
> > static LIST_HEAD(global_svm_list);
> >
> > #define for_each_svm_dev(sdev, svm, d) \
> > @@ -342,7 +407,14 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain,
> > struct device *dev,
> > svm->gpasid = data->gpasid;
> > svm->flags |= SVM_FLAG_GUEST_PASID;
> > }
> > + svm->iommu = iommu;
> > + /*
> > +* Set up cleanup async work in case IOASID core notify us PASID
> > +* is freed before unbind.
> > +*/
> > + INIT_WORK(&svm->work, intel_svm_free_async_fn);
Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request
- Original Message -
> Hi Jason
>
> On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
> > Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
> > page aligned address.") disables ATS for device that can do unaligned
> > page request.
>
> Did you take a look at the PCI specification?
> Page Aligned Request is in the ATS capability Register.
>
> ATS Capability Register (Offset 0x04h)
>
> bit (5):
> Page Aligned Request - If Set, indicates the Untranslated address is always
> aligned to 4096 byte boundary. Setting this field is recommended. This
> field permits software to distinguish between implemntations compatible
> with this specification and those compatible with an earlier version of
> this specification in which a Requester was permitted to supply anything in
> bits [11:2].
Yes, my understanding is that this is optional not mandatory.
>
> >
> > This looks wrong, since the commit log said it's because the page
> > request descriptor doesn't support reporting unaligned request.
>
> I don't think you can change the definition from ATS to PRI. Both are
> orthogonal feature.
I may miss something, here's my understanding is that:
- page request descriptor will only be used when PRS is enabled
- ATS spec allows unaligned request
So any reason for disabling ATS for unaligned request even if PRS is
not enabled?
>
> >
> > A victim is Qemu's virtio-pci which doesn't advertise the page aligned
> > address. Fixing by disable PRI instead of ATS if device doesn't have
> > page aligned request.
>
> This is a requirement for the Intel IOMMU's.
>
> You say virtio, so is it all emulated device or you talking about some
> hardware that implemented virtio-pci compliant hw? If you are sure the
> device actually does comply with the requirement, but just not enumerating
> the capability, you can maybe work a quirk to overcome that?
So far only emulated devices. But we are helping some vendor to
implement virtio hardware so we need to understand the connection
between ATS alignment and page request descriptor.
>
> Now PRI also has an alignment requirement, and Intel IOMMU's requires that
> as well. If your device supports SRIOV as well, PASID and PRI are
> enumerated just on the PF and not the VF. You might want to pay attension
> to that. We are still working on a solution for that problem.
Thanks for the reminding, but it looks to me according to the ATS
spec, all PRI message is 4096 byte aligned? E.g lower bites were used
for group index etc.
Thanks
>
> I don't think this is the right fix for your problem.
>
> >
> > Cc: sta...@vger.kernel.org
> > Cc: Ashok Raj
> > Cc: Jacob Pan
> > Cc: Keith Busch
> > Cc: Kuppuswamy Sathyanarayanan
> > Signed-off-by: Jason Wang
> > ---
> > drivers/iommu/intel/iommu.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] iommu/ioasid: Introduce notification APIs
On Tue, 1 Sep 2020 18:49:38 +0200
Auger Eric wrote:
> Hi Jacob,
>
> On 8/22/20 6:35 AM, Jacob Pan wrote:
> > Relations among IOASID users largely follow a publisher-subscriber
> > pattern. E.g. to support guest SVA on Intel Scalable I/O
> > Virtualization (SIOV) enabled platforms, VFIO, IOMMU, device
> > drivers, KVM are all users of IOASIDs. When a state change occurs,
> > VFIO publishes the change event that needs to be processed by other
> > users/subscribers.
> >
> > This patch introduced two types of notifications: global and per
> > ioasid_set. The latter is intended for users who only needs to
> > handle events related to the IOASID of a given set.
> > For more information, refer to the kernel documentation at
> > Documentation/ioasid.rst.
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Wu Hao
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 280
> > -
> > include/linux/ioasid.h | 70 + 2 files changed, 348
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index c0aef38a4fde..6ddc09a7fe74 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -9,8 +9,35 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/*
> > + * An IOASID could have multiple consumers where each consumeer
> > may have
> can have multiple consumers
Sounds good, I used past tense to describe a possibility :)
> > + * hardware contexts associated with IOASIDs.
> > + * When a status change occurs, such as IOASID is being freed,
> > notifier chains
> s/such as IOASID is being freed/, like on IOASID deallocation,
Better, will do.
> > + * are used to keep the consumers in sync.
> > + * This is a publisher-subscriber pattern where publisher can
> > change the
> > + * state of each IOASID, e.g. alloc/free, bind IOASID to a device
> > and mm.
> > + * On the other hand, subscribers gets notified for the state
> > change and
> > + * keep local states in sync.
> > + *
> > + * Currently, the notifier is global. A further optimization could
> > be per
> > + * IOASID set notifier chain.
> > + */
> > +static ATOMIC_NOTIFIER_HEAD(ioasid_chain);
> > +
> > +/* List to hold pending notification block registrations */
> > +static LIST_HEAD(ioasid_nb_pending_list);
> > +static DEFINE_SPINLOCK(ioasid_nb_lock);
> > +struct ioasid_set_nb {
> > + struct list_headlist;
> > + struct notifier_block *nb;
> > + void*token;
> > + struct ioasid_set *set;
> > + boolactive;
> > +};
> > +
> > enum ioasid_state {
> > IOASID_STATE_INACTIVE,
> > IOASID_STATE_ACTIVE,
> > @@ -394,6 +421,7 @@ EXPORT_SYMBOL_GPL(ioasid_find_by_spid);
> > ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
> > ioasid_t max, void *private)
> > {
> > + struct ioasid_nb_args args;
> > struct ioasid_data *data;
> > void *adata;
> > ioasid_t id = INVALID_IOASID;
> > @@ -445,8 +473,14 @@ ioasid_t ioasid_alloc(struct ioasid_set *set,
> > ioasid_t min, ioasid_t max, goto exit_free;
> > }
> > set->nr_ioasids++;
> > - goto done_unlock;
> > + args.id = id;
> > + /* Set private ID is not attached during allocation */
> > + args.spid = INVALID_IOASID;
> > + args.set = set;
> > + atomic_notifier_call_chain(&set->nh, IOASID_ALLOC, &args);
> >
> > + spin_unlock(&ioasid_allocator_lock);
> > + return id;
> spurious change
Good catch. should just goto done_unlock.
> > exit_free:
> > kfree(data);
> > done_unlock:
> > @@ -479,6 +513,7 @@ static void ioasid_do_free(struct ioasid_data
> > *data)
> > static void ioasid_free_locked(struct ioasid_set *set, ioasid_t
> > ioasid) {
> > + struct ioasid_nb_args args;
> > struct ioasid_data *data;
> >
> > data = xa_load(&active_allocator->xa, ioasid);
> > @@ -491,7 +526,16 @@ static void ioasid_free_locked(struct
> > ioasid_set *set, ioasid_t ioasid) pr_warn("Cannot free IOASID %u
> > due to set ownership\n", ioasid); return;
> > }
> > +
> spurious new line
got it
> > data->state = IOASID_STATE_FREE_PENDING;
> > + /* Notify all users that this IOASID is being freed */
> > + args.id = ioasid;
> > + args.spid = data->spid;
> > + args.pdata = data->private;
> > + args.set = data->set;
> > + atomic_notifier_call_chain(&ioasid_chain, IOASID_FREE,
> > &args);
> > + /* Notify the ioasid_set for per set users */
> > + atomic_notifier_call_chain(&set->nh, IOASID_FREE, &args);
> >
> > if (!refcount_dec_and_test(&data->users))
> > return;
> Shouldn't we call the notifier only when ref count == 0?
Not in the current scheme. The idea is to notify all users the PASID is
being freed, then each user can drop its reference. When refcount == 0,
the PASID will be returned to the pool.
> > @@ -514,6 +558,28 @@ void ioasid_free(struct ioasid_set *set,
> > ioasid
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, 2020-09-09 at 19:36 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
>
> IB part looks OK, I prefer it like this
>
> You could do the same for continue as well, I saw a few of those..
I saw some continue uses as well but wasn't sure
and didn't look to see if the switch/case with
continue was in a for/while loop.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
IB part looks OK, I prefer it like this
You could do the same for continue as well, I saw a few of those..
Thanks,
Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[trivial PATCH] treewide: Convert switch/case fallthrough; to break;
fallthrough to a separate case/default label break; isn't very readable.
Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;
Found using:
$ grep-2.5.4 -rP --include=*.[ch] -n
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
Miscellanea:
o Move or coalesce a couple label blocks above a default: block.
Signed-off-by: Joe Perches
---
Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.
arch/arm/mach-mmp/pm-pxa910.c | 2 +-
arch/arm64/kvm/handle_exit.c | 2 +-
arch/mips/kernel/cpu-probe.c | 2 +-
arch/mips/math-emu/cp1emu.c | 2 +-
arch/s390/pci/pci.c | 2 +-
crypto/tcrypt.c | 4 ++--
drivers/ata/sata_mv.c | 2 +-
drivers/atm/lanai.c | 2 +-
drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
drivers/hid/wacom_wac.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 2 +-
drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/irqchip/irq-vic.c | 4 ++--
drivers/md/dm.c | 2 +-
drivers/media/dvb-frontends/drxd_hard.c | 2 +-
drivers/media/i2c/ov5640.c| 2 +-
drivers/media/i2c/ov6650.c| 5 ++---
drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
drivers/media/i2c/tvp5150.c | 2 +-
drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
drivers/media/usb/cpia2/cpia2_core.c | 2 +-
drivers/mfd/iqs62x.c | 3 +--
drivers/mmc/host/atmel-mci.c | 2 +-
drivers/mtd/nand/raw/nandsim.c| 2 +-
drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
drivers/net/ethernet/sfc/farch.c | 2 +-
drivers/net/phy/adin.c| 3 +--
drivers/net/usb/pegasus.c | 4 ++--
drivers/net/usb/usbnet.c | 2 +-
drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
drivers/net/wireless/mediatek/mt7601u/dma.c | 8
drivers/nvme/host/core.c | 12 ++--
drivers/pcmcia/db1xxx_ss.c| 4 ++--
drivers/power/supply/abx500_chargalg.c| 2 +-
drivers/power/supply/charger-manager.c| 2 +-
drivers/rtc/rtc-pcf85063.c| 2 +-
drivers/s390/scsi/zfcp_fsf.c | 2 +-
drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
drivers/scsi/lpfc/lpfc_sli.c | 2 +-
drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
drivers/scsi/sr.c | 2 +-
drivers/tty/serial/sunsu.c| 2 +-
drivers/tty/serial/sunzilog.c | 2 +-
drivers/tty/vt/vt_ioctl.c | 2 +-
drivers/usb/dwc3/core.c | 2 +-
drivers/usb/gadget/legacy/inode.c | 2 +-
drivers/usb/gadget/udc/pxa25x_udc.c | 4 ++--
drivers/usb/host/ohci-hcd.c | 2 +-
drivers/usb/isp1760/isp1760-hcd.c | 2 +-
drivers/usb/musb/cppi_dma.c
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On 9/9/20 15:06, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
>
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
>
> Found using:
>
> $ grep-2.5.4 -rP --include=*.[ch] -n
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
>
> Miscellanea:
>
> o Move or coalesce a couple label blocks above a default: block.
>
> Signed-off-by: Joe Perches
Acked-by: Gustavo A. R. Silva
Thanks
--
Gustavo
> ---
>
> Compiled allyesconfig x86-64 only.
> A few files for other arches were not compiled.
>
> arch/arm/mach-mmp/pm-pxa910.c | 2 +-
> arch/arm64/kvm/handle_exit.c | 2 +-
> arch/mips/kernel/cpu-probe.c | 2 +-
> arch/mips/math-emu/cp1emu.c | 2 +-
> arch/s390/pci/pci.c | 2 +-
> crypto/tcrypt.c | 4 ++--
> drivers/ata/sata_mv.c | 2 +-
> drivers/atm/lanai.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
> drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
> drivers/hid/wacom_wac.c | 2 +-
> drivers/i2c/busses/i2c-i801.c | 2 +-
> drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
> drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
> drivers/irqchip/irq-vic.c | 4 ++--
> drivers/md/dm.c | 2 +-
> drivers/media/dvb-frontends/drxd_hard.c | 2 +-
> drivers/media/i2c/ov5640.c| 2 +-
> drivers/media/i2c/ov6650.c| 5 ++---
> drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
> drivers/media/i2c/tvp5150.c | 2 +-
> drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
> drivers/media/usb/cpia2/cpia2_core.c | 2 +-
> drivers/mfd/iqs62x.c | 3 +--
> drivers/mmc/host/atmel-mci.c | 2 +-
> drivers/mtd/nand/raw/nandsim.c| 2 +-
> drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
> drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
> drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
> drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
> drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
> drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
> drivers/net/ethernet/sfc/farch.c | 2 +-
> drivers/net/phy/adin.c| 3 +--
> drivers/net/usb/pegasus.c | 4 ++--
> drivers/net/usb/usbnet.c | 2 +-
> drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
> drivers/net/wireless/mediatek/mt7601u/dma.c | 8
> drivers/nvme/host/core.c | 12 ++--
> drivers/pcmcia/db1xxx_ss.c| 4 ++--
> drivers/power/supply/abx500_chargalg.c| 2 +-
> drivers/power/supply/charger-manager.c| 2 +-
> drivers/rtc/rtc-pcf85063.c| 2 +-
> drivers/s390/scsi/zfcp_fsf.c | 2 +-
> drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
> drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
> drivers/scsi/lpfc/lpfc_sli.c | 2 +-
> drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
> drivers/scsi/sr.c | 2 +-
> drivers/tty/serial/sunsu.c| 2 +-
> drivers/tty/serial/sunzilog.c | 2 +-
> drivers/tty/vt/vt_ioctl.c | 2 +-
> drivers/usb/dwc3/core.c | 2 +-
> drivers/usb/gadget/legacy/inode.c |
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
> index eea0f453cfb6..8aac5bc60f4c 100644
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -2464,7 +2464,7 @@ static int do_test(const char *alg, u32 type, u32 mask,
> int m, u32 num_mb)
> test_hash_speed("streebog512", sec,
> generic_hash_speed_template);
> if (mode > 300 && mode < 400) break;
> - fallthrough;
> + break;
> case 399:
> break;
Just imho, this change makes the preceding 'if' look even more
pointless. Maybe the fallthrough was a deliberate choice? Not that my
opinion matters here as I don't know this module, but it looked a bit
odd to me.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] iommu/ioasid: Introduce notification APIs
On Tue, 25 Aug 2020 12:26:17 +0200
Jean-Philippe Brucker wrote:
> On Fri, Aug 21, 2020 at 09:35:15PM -0700, Jacob Pan wrote:
> > Relations among IOASID users largely follow a publisher-subscriber
> > pattern. E.g. to support guest SVA on Intel Scalable I/O
> > Virtualization (SIOV) enabled platforms, VFIO, IOMMU, device
> > drivers, KVM are all users of IOASIDs. When a state change occurs,
> > VFIO publishes the change event that needs to be processed by other
> > users/subscribers.
> >
> > This patch introduced two types of notifications: global and per
> > ioasid_set. The latter is intended for users who only needs to
> > handle events related to the IOASID of a given set.
> > For more information, refer to the kernel documentation at
> > Documentation/ioasid.rst.
> >
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Wu Hao
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 280
> > -
> > include/linux/ioasid.h | 70 + 2 files changed, 348
> > insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index c0aef38a4fde..6ddc09a7fe74 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -9,8 +9,35 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > static DEFINE_XARRAY_ALLOC(ioasid_sets);
> > +/*
> > + * An IOASID could have multiple consumers where each consumeer
> > may have
>
> consumer
>
got it
> > + * hardware contexts associated with IOASIDs.
> > + * When a status change occurs, such as IOASID is being freed,
> > notifier chains
> > + * are used to keep the consumers in sync.
> > + * This is a publisher-subscriber pattern where publisher can
> > change the
> > + * state of each IOASID, e.g. alloc/free, bind IOASID to a device
> > and mm.
> > + * On the other hand, subscribers gets notified for the state
> > change and
> > + * keep local states in sync.
> > + *
> > + * Currently, the notifier is global. A further optimization could
> > be per
> > + * IOASID set notifier chain.
>
> The patch adds both
>
right, the comment is old. I will remove the paragraph since it is in
the doc.
> > + */
> > +static ATOMIC_NOTIFIER_HEAD(ioasid_chain);
>
> "ioasid_notifier" may be clearer
>
will do.
> > +
> > +/* List to hold pending notification block registrations */
> > +static LIST_HEAD(ioasid_nb_pending_list);
> > +static DEFINE_SPINLOCK(ioasid_nb_lock);
> > +struct ioasid_set_nb {
> > + struct list_headlist;
> > + struct notifier_block *nb;
> > + void*token;
> > + struct ioasid_set *set;
> > + boolactive;
> > +};
> > +
> > enum ioasid_state {
> > IOASID_STATE_INACTIVE,
> > IOASID_STATE_ACTIVE,
> > @@ -394,6 +421,7 @@ EXPORT_SYMBOL_GPL(ioasid_find_by_spid);
> > ioasid_t ioasid_alloc(struct ioasid_set *set, ioasid_t min,
> > ioasid_t max, void *private)
> > {
> > + struct ioasid_nb_args args;
> > struct ioasid_data *data;
> > void *adata;
> > ioasid_t id = INVALID_IOASID;
> > @@ -445,8 +473,14 @@ ioasid_t ioasid_alloc(struct ioasid_set *set,
> > ioasid_t min, ioasid_t max, goto exit_free;
> > }
> > set->nr_ioasids++;
> > - goto done_unlock;
> > + args.id = id;
> > + /* Set private ID is not attached during allocation */
> > + args.spid = INVALID_IOASID;
> > + args.set = set;
>
> args.pdata is uninitialized
>
right, it should be
args.pdata = data->private;
> > + atomic_notifier_call_chain(&set->nh, IOASID_ALLOC,
> > &args);
>
> No global notification?
>
There hasn't been a need since the only global notifier listener is
vt-d driver which cares about FREE event only.
> >
> > + spin_unlock(&ioasid_allocator_lock);
> > + return id;
> > exit_free:
> > kfree(data);
> > done_unlock:
> > @@ -479,6 +513,7 @@ static void ioasid_do_free(struct ioasid_data
> > *data)
> > static void ioasid_free_locked(struct ioasid_set *set, ioasid_t
> > ioasid) {
> > + struct ioasid_nb_args args;
> > struct ioasid_data *data;
> >
> > data = xa_load(&active_allocator->xa, ioasid);
> > @@ -491,7 +526,16 @@ static void ioasid_free_locked(struct
> > ioasid_set *set, ioasid_t ioasid) pr_warn("Cannot free IOASID %u
> > due to set ownership\n", ioasid); return;
> > }
> > +
> > data->state = IOASID_STATE_FREE_PENDING;
> > + /* Notify all users that this IOASID is being freed */
> > + args.id = ioasid;
> > + args.spid = data->spid;
> > + args.pdata = data->private;
> > + args.set = data->set;
> > + atomic_notifier_call_chain(&ioasid_chain, IOASID_FREE,
> > &args);
> > + /* Notify the ioasid_set for per set users */
> > + atomic_notifier_call_chain(&set->nh, IOASID_FREE, &args);
> >
> > if (!refcount_dec_and_test(&data->users))
> > return;
> > @@ -514,6 +558,28 @@ void ioasid_free(struct ioasid_set *set,
> > ioasid_t ioasid) }
> > EXPORT_SYMBOL_GPL(ioasid_free);
> >
> > +static
Re: [PATCH v2 1/3] swiotlb: Use %pa to print phys_addr_t variables
On Wed, Sep 09, 2020 at 06:59:13PM +0300, Andy Shevchenko wrote: > On Wed, Sep 02, 2020 at 11:02:46PM -0300, Fabio Estevam wrote: > > On Wed, Sep 2, 2020 at 2:31 PM Andy Shevchenko > > wrote: > > > > > > There is an extension to a %p to print phys_addr_t type of variables. > > > Use it here. > > > > > > Signed-off-by: Andy Shevchenko > > > --- > > > v2: dropped bytes replacement (Fabio) > > > > Reviewed-by: Fabio Estevam > > Thanks! > > Guys, can this series be applied? Sure. > > -- > With Best Regards, > Andy Shevchenko > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] intel-iommu: don't disable ATS for device without page aligned request
Hi Jason
On Wed, Sep 09, 2020 at 04:34:32PM +0800, Jason Wang wrote:
> Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
> page aligned address.") disables ATS for device that can do unaligned
> page request.
Did you take a look at the PCI specification?
Page Aligned Request is in the ATS capability Register.
ATS Capability Register (Offset 0x04h)
bit (5):
Page Aligned Request - If Set, indicates the Untranslated address is always
aligned to 4096 byte boundary. Setting this field is recommended. This
field permits software to distinguish between implemntations compatible
with this specification and those compatible with an earlier version of
this specification in which a Requester was permitted to supply anything in
bits [11:2].
>
> This looks wrong, since the commit log said it's because the page
> request descriptor doesn't support reporting unaligned request.
I don't think you can change the definition from ATS to PRI. Both are
orthogonal feature.
>
> A victim is Qemu's virtio-pci which doesn't advertise the page aligned
> address. Fixing by disable PRI instead of ATS if device doesn't have
> page aligned request.
This is a requirement for the Intel IOMMU's.
You say virtio, so is it all emulated device or you talking about some
hardware that implemented virtio-pci compliant hw? If you are sure the
device actually does comply with the requirement, but just not enumerating
the capability, you can maybe work a quirk to overcome that?
Now PRI also has an alignment requirement, and Intel IOMMU's requires that
as well. If your device supports SRIOV as well, PASID and PRI are
enumerated just on the PF and not the VF. You might want to pay attension
to that. We are still working on a solution for that problem.
I don't think this is the right fix for your problem.
>
> Cc: sta...@vger.kernel.org
> Cc: Ashok Raj
> Cc: Jacob Pan
> Cc: Keith Busch
> Cc: Kuppuswamy Sathyanarayanan
> Signed-off-by: Jason Wang
> ---
> drivers/iommu/intel/iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/8] iommu/arm-smmu: Support maintaining bootloader mappings
Hi Bjorn, On 9/4/2020 6:55 PM, Bjorn Andersson wrote: > Based on previous attempts and discussions this is the latest attempt at > inheriting stream mappings set up by the bootloader, for e.g. boot splash or > efifb. > > Per Will's request this builds on the work by Jordan and Rob for the Adreno > SMMU support. It applies cleanly ontop of v16 of their series, which can be > found at > https://lore.kernel.org/linux-arm-msm/20200901164707.2645413-1-robdcl...@gmail.com/ Is there a git repo available with all the patches put together? --- Thanks & Best Regards, Laurentiu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/3] swiotlb: Use %pa to print phys_addr_t variables
On Wed, Sep 02, 2020 at 11:02:46PM -0300, Fabio Estevam wrote: > On Wed, Sep 2, 2020 at 2:31 PM Andy Shevchenko > wrote: > > > > There is an extension to a %p to print phys_addr_t type of variables. > > Use it here. > > > > Signed-off-by: Andy Shevchenko > > --- > > v2: dropped bytes replacement (Fabio) > > Reviewed-by: Fabio Estevam Thanks! Guys, can this series be applied? -- With Best Regards, Andy Shevchenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 09/09/2020 10:16, Tvrtko Ursulin wrote:
On 08/09/2020 23:43, Tom Murphy wrote:
On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
wrote:
On 08/09/2020 16:44, Logan Gunthorpe wrote:
On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
} __sgt_iter(struct scatterlist *sgl, bool dma) {
struct sgt_iter s = { .sgp = sgl };
+ if (sgl && !sg_dma_len(s.sgp))
I'd extend the condition to be, just to be safe:
if (dma && sgl && !sg_dma_len(s.sgp))
Right, good catch, that's definitely necessary.
+ s.sgp = NULL;
+
if (s.sgp) {
s.max = s.curr = s.sgp->offset;
- s.max += s.sgp->length;
- if (dma)
+
+ if (dma) {
+ s.max += sg_dma_len(s.sgp);
s.dma = sg_dma_address(s.sgp);
- else
+ } else {
+ s.max += s.sgp->length;
s.pfn = page_to_pfn(sg_page(s.sgp));
+ }
Otherwise has this been tested or alternatively how to test it?
(How to
repro the issue.)
It has not been tested. To test it, you need Tom's patch set without
the
last "DO NOT MERGE" patch:
https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
Tom, do you have a branch somewhere I could pull from? (Just being lazy
about downloading a bunch of messages from the archives.)
I don't unfortunately. I'm working locally with poor internet.
What GPU is in your Lenovo x1 carbon 5th generation and what
graphical/desktop setup I need to repro?
Is this enough info?:
$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
Flags: bus master, fast devsel, latency 0, IRQ 148
Memory at eb00 (64-bit, non-prefetchable) [size=16M]
Memory at 6000 (64-bit, prefetchable) [size=256M]
I/O ports at e000 [size=64]
[virtual] Expansion ROM at 000c [disabled] [size=128K]
Capabilities: [40] Vendor Specific Information: Len=0c
Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [100] Process Address Space ID (PASID)
Capabilities: [200] Address Translation Service (ATS)
Works for a start. What about the steps to repro? Any desktop
environment and it is just visual corruption, no hangs/stalls or such?
I've submitted a series consisting of what I understood are the patches
needed to repro the issue to our automated CI here:
https://patchwork.freedesktop.org/series/81489/
So will see if it will catch something, or more targeted testing will be
required. Hopefully it does trip over in which case I can add the patch
suggested by Logan on top and see if that fixes it. Or I'll need to
write a new test case.
If you could glance over my series to check I identified the patches
correctly it would be appreciated.
Our CI was more than capable at catching the breakage so I've copied you
on a patch (https://patchwork.freedesktop.org/series/81497/) which has a
good potential to fix this. (Or improve the robustness of our sg walks,
depends how you look at it.)
Would you be able to test it in your environment by any chance? If it
works I understand it unblocks your IOMMU work, right?
Regards,
Tvrtko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/dma: Fix IOVA reserve dma ranges
On 2020-09-09 06:32, Srinath Mannam wrote:
Fix IOVA reserve failure for memory regions listed in dma-ranges in the
following cases.
- start address of memory region is 0x0.
That's fair enough, and in fact generalises to the case of zero-sized
gaps between regions, which is indeed an oversight.
- end address of a memory region is equal to start address of next memory
region.
This part doesn't make much sense, however - if the regions described in
bridge->dma_ranges overlap, that's a bug in whoever created a malformed
list to begin with. Possibly it's just poor wording, and you're using
"memory regions" to refer to any or all of the dma_ranges, the reserved
IOVA ranges, and what "start" and "end" in this function represent which
isn't quite either of those.
Fixes: aadad097cd46f ("iommu/dma: Reserve IOVA for PCIe inaccessible DMA
address")
Signed-off-by: Srinath Mannam
---
drivers/iommu/dma-iommu.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5141d49a046b..0a3f67a4f9ae 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -213,14 +213,21 @@ static int iova_reserve_pci_windows(struct pci_dev *dev,
resource_list_for_each_entry(window, &bridge->dma_ranges) {
end = window->res->start - window->offset;
resv_iova:
+ if (end < start) {
+ /* dma_ranges list should be sorted */
+ dev_err(&dev->dev, "Failed to reserve IOVA\n");
+ return -EINVAL;
+ }
+ /*
+* Skip the cases when start address of first memory region is
+* 0x0 and end address of one memory region and start address
+* of next memory region are equal. Reserve IOVA for rest of
+* addresses fall in between given memory ranges.
+*/
if (end > start) {
lo = iova_pfn(iovad, start);
hi = iova_pfn(iovad, end);
reserve_iova(iovad, lo, hi);
- } else {
Surely this only needs to be a one-liner?
- } else {
+ } else if (end < start) {
(or possibly "end != start"; I can't quite decide which expresses the
semantic intent better)
The rest just looks like unnecessary churn - I don't think it needs
commenting that a sorted list may simply not have gaps between entries,
and as above I think the wording of that comment is actively misleading.
Robin.
- /* dma_ranges list should be sorted */
- dev_err(&dev->dev, "Failed to reserve IOVA\n");
- return -EINVAL;
}
start = window->res->end - window->offset + 1;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[bug report] iommu/amd: Restore IRTE.RemapEn bit after programming IRTE
Hello Suravee Suthikulpanit, This is a semi-automatic email about new static checker warnings. The patch 26e495f34107: "iommu/amd: Restore IRTE.RemapEn bit after programming IRTE" from Sep 3, 2020, leads to the following Smatch complaint: drivers/iommu/amd/iommu.c:3870 amd_iommu_deactivate_guest_mode() warn: variable dereferenced before check 'entry' (see line 3867) drivers/iommu/amd/iommu.c 3866 struct irq_cfg *cfg = ir_data->cfg; 3867 u64 valid = entry->lo.fields_remap.valid; The patch adds a new dereference 3868 3869 if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir) || 3870 !entry || !entry->lo.fields_vapic.guest_mode) ^^ before "entry" has been checked for NULL. 3871 return 0; 3872 regards, dan carpenter ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 08/09/2020 23:43, Tom Murphy wrote:
On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
wrote:
On 08/09/2020 16:44, Logan Gunthorpe wrote:
On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
} __sgt_iter(struct scatterlist *sgl, bool dma) {
struct sgt_iter s = { .sgp = sgl };
+ if (sgl && !sg_dma_len(s.sgp))
I'd extend the condition to be, just to be safe:
if (dma && sgl && !sg_dma_len(s.sgp))
Right, good catch, that's definitely necessary.
+ s.sgp = NULL;
+
if (s.sgp) {
s.max = s.curr = s.sgp->offset;
- s.max += s.sgp->length;
- if (dma)
+
+ if (dma) {
+ s.max += sg_dma_len(s.sgp);
s.dma = sg_dma_address(s.sgp);
- else
+ } else {
+ s.max += s.sgp->length;
s.pfn = page_to_pfn(sg_page(s.sgp));
+ }
Otherwise has this been tested or alternatively how to test it? (How to
repro the issue.)
It has not been tested. To test it, you need Tom's patch set without the
last "DO NOT MERGE" patch:
https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
Tom, do you have a branch somewhere I could pull from? (Just being lazy
about downloading a bunch of messages from the archives.)
I don't unfortunately. I'm working locally with poor internet.
What GPU is in your Lenovo x1 carbon 5th generation and what
graphical/desktop setup I need to repro?
Is this enough info?:
$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
Flags: bus master, fast devsel, latency 0, IRQ 148
Memory at eb00 (64-bit, non-prefetchable) [size=16M]
Memory at 6000 (64-bit, prefetchable) [size=256M]
I/O ports at e000 [size=64]
[virtual] Expansion ROM at 000c [disabled] [size=128K]
Capabilities: [40] Vendor Specific Information: Len=0c
Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [100] Process Address Space ID (PASID)
Capabilities: [200] Address Translation Service (ATS)
Works for a start. What about the steps to repro? Any desktop
environment and it is just visual corruption, no hangs/stalls or such?
I've submitted a series consisting of what I understood are the patches
needed to repro the issue to our automated CI here:
https://patchwork.freedesktop.org/series/81489/
So will see if it will catch something, or more targeted testing will be
required. Hopefully it does trip over in which case I can add the patch
suggested by Logan on top and see if that fixes it. Or I'll need to
write a new test case.
If you could glance over my series to check I identified the patches
correctly it would be appreciated.
Regards,
Tvrtko
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] intel-iommu: don't disable ATS for device without page aligned request
Commit 61363c1474b1 ("iommu/vt-d: Enable ATS only if the device uses
page aligned address.") disables ATS for device that can do unaligned
page request.
This looks wrong, since the commit log said it's because the page
request descriptor doesn't support reporting unaligned request.
A victim is Qemu's virtio-pci which doesn't advertise the page aligned
address. Fixing by disable PRI instead of ATS if device doesn't have
page aligned request.
Cc: sta...@vger.kernel.org
Cc: Ashok Raj
Cc: Jacob Pan
Cc: Keith Busch
Cc: Kuppuswamy Sathyanarayanan
Signed-off-by: Jason Wang
---
drivers/iommu/intel/iommu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e9864e52b0e9..ef5214a8a4dd 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1440,10 +1440,11 @@ static void iommu_enable_dev_iotlb(struct
device_domain_info *info)
if (info->pri_supported &&
(info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1) &&
+ pci_ats_page_aligned(pdev) &&
!pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
info->pri_enabled = 1;
#endif
- if (info->ats_supported && pci_ats_page_aligned(pdev) &&
+ if (info->ats_supported &&
!pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
info->ats_enabled = 1;
domain_update_iotlb(info->domain);
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu
On Wed, Sep 09, 2020 at 09:43:09AM +0800, Lu Baolu wrote:
> + /*
> +* The Intel graphic device driver is used to assume that the
> returned
> +* sg list is not combound. This blocks the efforts of converting
> the
This adds pointless overly long lines.
> +* Intel IOMMU driver to dma-iommu api's. Add this quirk to make the
> +* device driver work and should be removed once it's fixed in i915
> +* driver.
> +*/
> + if (dev_is_pci(dev) &&
> + to_pci_dev(dev)->vendor == PCI_VENDOR_ID_INTEL &&
> + (to_pci_dev(dev)->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
> + for_each_sg(sg, s, nents, i) {
> + unsigned int s_iova_off = sg_dma_address(s);
> + unsigned int s_length = sg_dma_len(s);
> + unsigned int s_iova_len = s->length;
> +
> + s->offset += s_iova_off;
> + s->length = s_length;
> + sg_dma_address(s) = dma_addr + s_iova_off;
> + sg_dma_len(s) = s_length;
> + dma_addr += s_iova_len;
> + }
> +
> + return nents;
> + }
This wants an IS_ENABLED() check. And probably a pr_once reminding
of the workaround.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V2 2/5] iommu: Add iommu_dma_free_cpu_cached_iovas function
> +static inline void iommu_dma_free_cpu_cached_iovas(unsigned int cpu, > + struct iommu_domain > *domain) This adds a crazy long line. Which is rather pointless as other bits of code in the patch use the more compact two tab indentations for the prototype continuation lines anyway. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
