Re: [PATCH v4 13/13] iommu/arm-smmu-v3: Add support for PCI PASID
On Tue, Jan 14, 2020 at 12:45:42PM +, Will Deacon wrote:
> On Thu, Dec 19, 2019 at 05:30:33PM +0100, Jean-Philippe Brucker wrote:
> > Enable PASID for PCI devices that support it. Since the SSID tables are
> > allocated by arm_smmu_attach_dev(), PASID has to be enabled early enough.
> > arm_smmu_dev_feature_enable() would be too late, since by that time the
>
> What is arm_smmu_dev_feature_enable()?
It's the implementation of the IOMMU op .dev_enable_feat(), which I'll add
later (called by a device driver to enable the SVA feature). I'll reword
this comment, since the only real requirement is enabling PASID before
ATS.
> > static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> > {
> > unsigned long flags;
> > @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev)
> >
> > master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits);
> >
> > + /* Note that PASID must be enabled before, and disabled after ATS */
> > + arm_smmu_enable_pasid(master);
>
> Is that part of the PCIe specs? If so, please can you add a citation to the
> comment?
Yes (PCIe 4.0r1.0 10.5.1.3 ATS Control register).
> Are there any other ordering requirements, i.e. with respect to enabling
> substreams at the SMMU? For example, can a speculative ATS request provide
> a PASID?
You recent fix bfff88ec1afe ("iommu/arm-smmu-v3: Rework enabling/disabling
of ATS for PCI masters") should prevent from speculative ATS requests.
More generally both ATS and SSID are enabled and disabled at the same time
in the SMMU, when toggling STE.V, so any request arriving before STE
enablement will be aborted regardless of SSID.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PULL REQUEST] iommu/vt-d: patches for v5.6 - 2nd wave
Hi Joerg, Below patches have been piled up for v5.6 since v5.5-rc3. They are mostly misc changes and cleanups. - Instead of aborting DMAR processing, mark firmware tainted if any RMRRs fail sanity check. - Check host bridge type correctly. - Allow devices with RMRRs to use identity domain. - Remove duplicated default identity domain treatment. Please consider them for the iommu/vt-d branch. Best regards, -baolu Barret Rhoden (2): iommu/vt-d: Mark firmware tainted if RMRR fails sanity check iommu/vt-d: Add RMRR base and end addresses sanity check Lu Baolu (2): iommu/vt-d: Allow devices with RMRRs to use identity domain iommu/vt-d: Unnecessary to handle default identity domain jimyan (1): iommu/vt-d: Don't reject Host Bridge due to scope mismatch drivers/iommu/dmar.c| 2 +- drivers/iommu/intel-iommu.c | 47 ++--- 2 files changed, 24 insertions(+), 25 deletions(-) -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3/5] iommu/vt-d: Add RMRR base and end addresses sanity check
From: Barret Rhoden
The VT-d spec specifies requirements for the RMRR entries base and
end (called 'Limit' in the docs) addresses.
This commit will cause the DMAR processing to mark the firmware as
tainted if any RMRR entries that do not meet these requirements.
Signed-off-by: Barret Rhoden
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-iommu.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0505731b9e47..c6843642f462 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4454,13 +4454,24 @@ static void __init init_iommu_pm_ops(void)
static inline void init_iommu_pm_ops(void) {}
#endif /* CONFIG_PM */
+static int rmrr_sanity_check(struct acpi_dmar_reserved_memory *rmrr)
+{
+ if (!IS_ALIGNED(rmrr->base_address, PAGE_SIZE) ||
+ !IS_ALIGNED(rmrr->end_address + 1, PAGE_SIZE) ||
+ rmrr->end_address <= rmrr->base_address ||
+ arch_rmrr_sanity_check(rmrr))
+ return -EINVAL;
+
+ return 0;
+}
+
int __init dmar_parse_one_rmrr(struct acpi_dmar_header *header, void *arg)
{
struct acpi_dmar_reserved_memory *rmrr;
struct dmar_rmrr_unit *rmrru;
rmrr = (struct acpi_dmar_reserved_memory *)header;
- if (arch_rmrr_sanity_check(rmrr))
+ if (rmrr_sanity_check(rmrr))
WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND,
"Your BIOS is broken; bad RMRR [%#018Lx-%#018Lx]\n"
"BIOS vendor: %s; Ver: %s; Product Version: %s\n",
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4/5] iommu/vt-d: Allow devices with RMRRs to use identity domain
Since commit ea2447f700cab ("intel-iommu: Prevent devices with
RMRRs from being placed into SI Domain"), the Intel IOMMU driver
doesn't allow any devices with RMRR locked to use the identity
domain. This was added to to fix the issue where the RMRR info
for devices being placed in and out of the identity domain gets
lost. This identity maps all RMRRs when setting up the identity
domain, so that devices with RMRRs could also use it.
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-iommu.c | 15 ++-
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index c6843642f462..3446da5a2186 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2893,10 +2893,8 @@ static int __init si_domain_init(int hw)
}
/*
-* Normally we use DMA domains for devices which have RMRRs. But we
-* loose this requirement for graphic and usb devices. Identity map
-* the RMRRs for graphic and USB devices so that they could use the
-* si_domain.
+* Identity map the RMRRs so that devices with RMRRs could also use
+* the si_domain.
*/
for_each_rmrr_units(rmrr) {
for_each_active_dev_scope(rmrr->devices, rmrr->devices_cnt,
@@ -2904,9 +2902,6 @@ static int __init si_domain_init(int hw)
unsigned long long start = rmrr->base_address;
unsigned long long end = rmrr->end_address;
- if (device_is_rmrr_locked(dev))
- continue;
-
if (WARN_ON(end < start ||
end >> agaw_to_width(si_domain->agaw)))
continue;
@@ -3045,9 +3040,6 @@ static int device_def_domain_type(struct device *dev)
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
- if (device_is_rmrr_locked(dev))
- return IOMMU_DOMAIN_DMA;
-
/*
* Prevent any device marked as untrusted from getting
* placed into the statically identity mapping domain.
@@ -3085,9 +3077,6 @@ static int device_def_domain_type(struct device *dev)
return IOMMU_DOMAIN_DMA;
} else if (pci_pcie_type(pdev) == PCI_EXP_TYPE_PCI_BRIDGE)
return IOMMU_DOMAIN_DMA;
- } else {
- if (device_has_rmrr(dev))
- return IOMMU_DOMAIN_DMA;
}
return (iommu_identity_mapping & IDENTMAP_ALL) ?
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/5] iommu/vt-d: Don't reject Host Bridge due to scope mismatch
From: jimyan
On a system with two host bridges(:00:00.0,:80:00.0), iommu
initialization fails with
DMAR: Device scope type does not match for :80:00.0
This is because the DMAR table reports this device as having scope 2
(ACPI_DMAR_SCOPE_TYPE_BRIDGE):
but the device has a type 0 PCI header:
80:00.0 Class 0600: Device 8086:2020 (rev 06)
00: 86 80 20 20 47 05 10 00 06 00 00 06 10 00 00 00
10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
20: 00 00 00 00 00 00 00 00 00 00 00 00 86 80 00 00
30: 00 00 00 00 90 00 00 00 00 00 00 00 00 01 00 00
VT-d works perfectly on this system, so there's no reason to bail out
on initialization due to this apparent scope mismatch. Add the class
0x06 ("PCI_BASE_CLASS_BRIDGE") as a heuristic for allowing DMAR
initialization for non-bridge PCI devices listed with scope bridge.
Signed-off-by: jimyan
Reviewed-by: Jerry Snitselaar
Reviewed-by: Roland Dreier
Signed-off-by: Lu Baolu
---
drivers/iommu/dmar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index fb30d5053664..613b7153905d 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -244,7 +244,7 @@ int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
info->dev->hdr_type != PCI_HEADER_TYPE_NORMAL) ||
(scope->entry_type == ACPI_DMAR_SCOPE_TYPE_BRIDGE &&
(info->dev->hdr_type == PCI_HEADER_TYPE_NORMAL &&
- info->dev->class >> 8 != PCI_CLASS_BRIDGE_OTHER))) {
+ info->dev->class >> 16 != PCI_BASE_CLASS_BRIDGE))) {
pr_warn("Device scope type does not match for %s\n",
pci_name(info->dev));
return -EINVAL;
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/5] iommu/vt-d: Mark firmware tainted if RMRR fails sanity check
From: Barret Rhoden
RMRR entries describe memory regions that are DMA targets for devices
outside the kernel's control.
RMRR entries that fail the sanity check are pointing to regions of
memory that the firmware did not tell the kernel are reserved or
otherwise should not be used.
Instead of aborting DMAR processing, this commit marks the firmware
as tainted. These RMRRs will still be identity mapped, otherwise,
some devices, e.x. graphic devices, will not work during boot.
Signed-off-by: Barret Rhoden
Signed-off-by: Lu Baolu
---
drivers/iommu/intel-iommu.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9be6717c8286..0505731b9e47 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4458,12 +4458,16 @@ int __init dmar_parse_one_rmrr(struct acpi_dmar_header
*header, void *arg)
{
struct acpi_dmar_reserved_memory *rmrr;
struct dmar_rmrr_unit *rmrru;
- int ret;
rmrr = (struct acpi_dmar_reserved_memory *)header;
- ret = arch_rmrr_sanity_check(rmrr);
- if (ret)
- return ret;
+ if (arch_rmrr_sanity_check(rmrr))
+ WARN_TAINT(1, TAINT_FIRMWARE_WORKAROUND,
+ "Your BIOS is broken; bad RMRR [%#018Lx-%#018Lx]\n"
+ "BIOS vendor: %s; Ver: %s; Product Version: %s\n",
+ rmrr->base_address, rmrr->end_address,
+ dmi_get_system_info(DMI_BIOS_VENDOR),
+ dmi_get_system_info(DMI_BIOS_VERSION),
+ dmi_get_system_info(DMI_PRODUCT_VERSION));
rmrru = kzalloc(sizeof(*rmrru), GFP_KERNEL);
if (!rmrru)
--
2.17.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5/5] iommu/vt-d: Unnecessary to handle default identity domain
The iommu default domain framework has been designed to take care of setting identity default domain type. It's unnecessary to handle this again in the VT-d driver. Hence, remove it. Signed-off-by: Lu Baolu --- drivers/iommu/intel-iommu.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c index 3446da5a2186..f1f812cb7b9d 100644 --- a/drivers/iommu/intel-iommu.c +++ b/drivers/iommu/intel-iommu.c @@ -387,7 +387,6 @@ static int intel_iommu_superpage = 1; static int iommu_identity_mapping; static int intel_no_bounce; -#define IDENTMAP_ALL 1 #define IDENTMAP_GFX 2 #define IDENTMAP_AZALIA4 @@ -3079,8 +3078,7 @@ static int device_def_domain_type(struct device *dev) return IOMMU_DOMAIN_DMA; } - return (iommu_identity_mapping & IDENTMAP_ALL) ? - IOMMU_DOMAIN_IDENTITY : 0; + return 0; } static void intel_iommu_init_qi(struct intel_iommu *iommu) @@ -3424,9 +3422,6 @@ static int __init init_dmars(void) iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH); } - if (iommu_default_passthrough()) - iommu_identity_mapping |= IDENTMAP_ALL; - #ifdef CONFIG_INTEL_IOMMU_BROKEN_GFX_WA dmar_map_gfx = 0; #endif @@ -5038,7 +5033,7 @@ static int __init platform_optin_force_iommu(void) * map for all devices except those marked as being untrusted. */ if (dmar_disabled) - iommu_identity_mapping |= IDENTMAP_ALL; + iommu_set_default_passthrough(false); dmar_disabled = 0; no_iommu = 0; -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PoC] arm: dma-mapping: direct: Apply dma_pfn_offset only when it is valid
On 14/01/2020 4:43 pm, Peter Ujfalusi wrote:
The dma_pfn_offset should only be applied to an address which is within the
dma-ranges range. Any address outside should have offset as 0.
No, that's wrong. If a non-empty dma-ranges is present, then addresses
which do not fall within any specified range are invalid altogether.
The current long-term plan is indeed to try to move to some sort of
internal "DMA range descriptor" in order to properly cope with the kind
of esoteric integrations which have multiple disjoint windows,
potentially even with different offsets, but as you point out there are
still many hurdles between now and that becoming reality. So although
this patch does represent the "right" thing, it's for entirely the wrong
reason. AFAICT for your case it basically just works out as a very
baroque way to hack dma_direct_supported() again - we shouldn't need a
special case to map a bogus physical address to valid DMA address, we
should be fixing the source of the bogus PA in the first place.
This is a proof of concept patch which works on k2g where we have
dma-ranges = <0x8000 0x8 0x 0x8000>;
for the SoC.
TBH it's probably extra-confusing that you're on Keystone 2, where
technically this ends up closer-to-OK than most, since IIRC the 0-2GB
MMIO region is the same on all 3(?) interconnect maps. Thus the 100%
honest description would really be:
dma-ranges = <0x0 0x0 0x0 0x8000>,
<0x8000 0x8 0x 0x8000>;
but yeah, that would just go horribly wrong with Linux today. The
subtelty that dma_map_resource() ignores the pfn_offset happens to be a
"feature" in this regard ;)
Robin.
Without this patch everything which tries to set DMA_BIT_MASK(32) or less
fails -> DMA and peripherals with built in DMA (SD with ADMA) will not
probe or fall back to PIO mode.
With this patch EDMA probes, SD's ADMA is working.
Audio and dma-test is working just fine with EDMA, mmc accesses with ADMA
also operational.
The patch does not tried to address the incomplete handling of dma-ranges
from DT and it is not fixing/updating arch code or drivers which uses
dma_pfn_offset.
Neither provides fallback support for kernel setting only dma_pfn_offset to
arbitrary number without paddr/dma_addr/size.
Signed-off-by: Peter Ujfalusi
---
Hi Christoph, Robin,
I know it is a bit more complicated, but with this patch k2g is working fine...
I wanted to test the concept I was describing and a patch speaks better than
words.
Kind regards,
Peter
arch/arm/include/asm/dma-mapping.h | 25 --
drivers/of/device.c| 7 ++-
include/linux/device.h | 8
include/linux/dma-direct.h | 33 --
kernel/dma/coherent.c | 9 +---
5 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index bdd80ddbca34..9bff6ad2d8c8 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -33,10 +33,31 @@ static inline const struct dma_map_ops
*get_arch_dma_ops(struct bus_type *bus)
* addresses. They must not be used by drivers.
*/
#ifndef __arch_pfn_to_dma
+
+static inline unsigned long __phys_to_dma_pfn_offset(struct device *dev,
+phys_addr_t paddr)
+{
+ if (paddr >= dev->dma_ranges.paddr &&
+ paddr <= (dev->dma_ranges.paddr + dev->dma_ranges.size))
+ return dev->dma_ranges.pfn_offset;
+
+ return 0;
+}
+
+static inline unsigned long __dma_to_phys_pfn_offset(struct device *dev,
+dma_addr_t dma_addr)
+{
+ if (dma_addr >= dev->dma_ranges.dma_addr &&
+ dma_addr <= (dev->dma_ranges.dma_addr + dev->dma_ranges.size))
+ return dev->dma_ranges.pfn_offset;
+
+ return 0;
+}
+
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
if (dev)
- pfn -= dev->dma_pfn_offset;
+ pfn -= __phys_to_dma_pfn_offset(dev, __pfn_to_phys(pfn));
return (dma_addr_t)__pfn_to_bus(pfn);
}
@@ -45,7 +66,7 @@ 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;
+ pfn += __dma_to_phys_pfn_offset(dev, addr);
return pfn;
}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 27203bfd0b22..07a8cc1a7d7f 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,7 +105,7 @@ int of_dma_configure(struct device *dev, struct device_node
*np, bool force_dma)
if (!force_dma)
return ret == -ENODEV ? 0 : ret;
- dma_addr = offset = 0;
+ dma_addr = offset = paddr = 0;
} else {
offset = PFN_DOWN(paddr -
[PoC] arm: dma-mapping: direct: Apply dma_pfn_offset only when it is valid
The dma_pfn_offset should only be applied to an address which is within the
dma-ranges range. Any address outside should have offset as 0.
This is a proof of concept patch which works on k2g where we have
dma-ranges = <0x8000 0x8 0x 0x8000>;
for the SoC.
Without this patch everything which tries to set DMA_BIT_MASK(32) or less
fails -> DMA and peripherals with built in DMA (SD with ADMA) will not
probe or fall back to PIO mode.
With this patch EDMA probes, SD's ADMA is working.
Audio and dma-test is working just fine with EDMA, mmc accesses with ADMA
also operational.
The patch does not tried to address the incomplete handling of dma-ranges
from DT and it is not fixing/updating arch code or drivers which uses
dma_pfn_offset.
Neither provides fallback support for kernel setting only dma_pfn_offset to
arbitrary number without paddr/dma_addr/size.
Signed-off-by: Peter Ujfalusi
---
Hi Christoph, Robin,
I know it is a bit more complicated, but with this patch k2g is working fine...
I wanted to test the concept I was describing and a patch speaks better than
words.
Kind regards,
Peter
arch/arm/include/asm/dma-mapping.h | 25 --
drivers/of/device.c| 7 ++-
include/linux/device.h | 8
include/linux/dma-direct.h | 33 --
kernel/dma/coherent.c | 9 +---
5 files changed, 74 insertions(+), 8 deletions(-)
diff --git a/arch/arm/include/asm/dma-mapping.h
b/arch/arm/include/asm/dma-mapping.h
index bdd80ddbca34..9bff6ad2d8c8 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -33,10 +33,31 @@ static inline const struct dma_map_ops
*get_arch_dma_ops(struct bus_type *bus)
* addresses. They must not be used by drivers.
*/
#ifndef __arch_pfn_to_dma
+
+static inline unsigned long __phys_to_dma_pfn_offset(struct device *dev,
+phys_addr_t paddr)
+{
+ if (paddr >= dev->dma_ranges.paddr &&
+ paddr <= (dev->dma_ranges.paddr + dev->dma_ranges.size))
+ return dev->dma_ranges.pfn_offset;
+
+ return 0;
+}
+
+static inline unsigned long __dma_to_phys_pfn_offset(struct device *dev,
+dma_addr_t dma_addr)
+{
+ if (dma_addr >= dev->dma_ranges.dma_addr &&
+ dma_addr <= (dev->dma_ranges.dma_addr + dev->dma_ranges.size))
+ return dev->dma_ranges.pfn_offset;
+
+ return 0;
+}
+
static inline dma_addr_t pfn_to_dma(struct device *dev, unsigned long pfn)
{
if (dev)
- pfn -= dev->dma_pfn_offset;
+ pfn -= __phys_to_dma_pfn_offset(dev, __pfn_to_phys(pfn));
return (dma_addr_t)__pfn_to_bus(pfn);
}
@@ -45,7 +66,7 @@ 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;
+ pfn += __dma_to_phys_pfn_offset(dev, addr);
return pfn;
}
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 27203bfd0b22..07a8cc1a7d7f 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,7 +105,7 @@ int of_dma_configure(struct device *dev, struct device_node
*np, bool force_dma)
if (!force_dma)
return ret == -ENODEV ? 0 : ret;
- dma_addr = offset = 0;
+ dma_addr = offset = paddr = 0;
} else {
offset = PFN_DOWN(paddr - dma_addr);
@@ -144,6 +144,11 @@ int of_dma_configure(struct device *dev, struct
device_node *np, bool force_dma)
dev->dma_pfn_offset = offset;
+ dev->dma_ranges.paddr = paddr;
+ dev->dma_ranges.dma_addr = dma_addr;
+ dev->dma_ranges.size = size;
+ dev->dma_ranges.pfn_offset = offset;
+
/*
* Limit coherent and dma mask based on size and default mask
* set by the driver.
diff --git a/include/linux/device.h b/include/linux/device.h
index ce6db68c3f29..57006b51a989 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -293,6 +293,13 @@ struct device_dma_parameters {
unsigned long segment_boundary_mask;
};
+struct dma_ranges {
+ u64 paddr;
+ u64 dma_addr;
+ u64 size;
+ unsigned long pfn_offset;
+};
+
/**
* struct device_connection - Device Connection Descriptor
* @fwnode: The device node of the connected device
@@ -581,6 +588,7 @@ struct device {
allocations such descriptors. */
u64 bus_dma_limit; /* upstream dma constraint */
unsigned long dma_pfn_offset;
+ struct dma_ranges dma_ranges;
struct device_dma_parameters *dma_parms;
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 24b8684aa21d..4a46a15945ea 100644
--- a/include/linux/dma-direct.h
+++
Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Prepare for handling arm_smmu_write_ctx_desc() failure
On Tue, Jan 14, 2020 at 12:42:47PM +, Will Deacon wrote: > On Thu, Dec 19, 2019 at 05:30:29PM +0100, Jean-Philippe Brucker wrote: > > Second-level context descriptor tables will be allocated lazily in > > arm_smmu_write_ctx_desc(). Help with handling allocation failure by > > moving the CD write into arm_smmu_domain_finalise_s1(). > > > > Reviewed-by: Eric Auger > > Reviewed-by: Jonathan Cameron > > Signed-off-by: Jean-Philippe Brucker > > --- > > drivers/iommu/arm-smmu-v3.c | 11 +++ > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > > index e147087198ef..b825a5639afc 100644 > > --- a/drivers/iommu/arm-smmu-v3.c > > +++ b/drivers/iommu/arm-smmu-v3.c > > @@ -2301,8 +2301,15 @@ static int arm_smmu_domain_finalise_s1(struct > > arm_smmu_domain *smmu_domain, > > cfg->cd.ttbr= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0]; > > cfg->cd.tcr = pgtbl_cfg->arm_lpae_s1_cfg.tcr; > > cfg->cd.mair= pgtbl_cfg->arm_lpae_s1_cfg.mair; > > + > > + ret = arm_smmu_write_ctx_desc(smmu_domain, 0, >cd); > > Hmm. This ends up calling arm_smmu_sync_cd() but I think that happens before > we've added the master to the devices list of the domain. Does that mean we > miss the new SSID during the invalidation? Yes, the arm_smmu_sync_cd() isn't useful in this case, it's only needed when the STE is live and arm_smmu_write_ctx_desc() is called for a ssid!=0. On this path, the CD cache is invalidated by a CFGI_STE executed later, when arm_smmu_attach_dev() installs the STE. I didn't want to add a special case that avoids the sync when ssid==0 in because a spurious sync probably doesn't impact performance here and arm_smmu_write_ctx_desc() is quite fiddly already. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Add support for Substream IDs
On Tue, Jan 14, 2020 at 12:38:19PM +, Will Deacon wrote:
> > +static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> > +int ssid, bool leaf)
> > +{
> > + size_t i;
> > + unsigned long flags;
> > + struct arm_smmu_master *master;
> > + struct arm_smmu_device *smmu = smmu_domain->smmu;
> > + struct arm_smmu_cmdq_ent cmd = {
> > + .opcode = CMDQ_OP_CFGI_CD,
> > + .cfgi = {
> > + .ssid = ssid,
> > + .leaf = leaf,
> > + },
> > + };
> > +
> > + spin_lock_irqsave(_domain->devices_lock, flags);
> > + list_for_each_entry(master, _domain->devices, domain_head) {
> > + for (i = 0; i < master->num_sids; i++) {
> > + cmd.cfgi.sid = master->sids[i];
> > + arm_smmu_cmdq_issue_cmd(smmu, );
> > + }
> > + }
> > + spin_unlock_irqrestore(_domain->devices_lock, flags);
> > +
> > + arm_smmu_cmdq_issue_sync(smmu);
>
> Can you send a follow-up patch converting this to batch submission, please?
Ok
> > +}
> > +
> > static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> > struct arm_smmu_cd_table *table,
> > size_t num_entries)
> > @@ -1498,34 +1541,65 @@ static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr)
> > return val;
> > }
> >
> > -static void arm_smmu_write_ctx_desc(struct arm_smmu_device *smmu,
> > - struct arm_smmu_s1_cfg *cfg)
> > +static int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain,
> > + int ssid, struct arm_smmu_ctx_desc *cd)
> > {
> > - u64 val;
> > - __le64 *cdptr = cfg->table.ptr;
> > -
> > /*
> > -* We don't need to issue any invalidation here, as we'll invalidate
> > -* the STE when installing the new entry anyway.
> > +* This function handles the following cases:
> > +*
> > +* (1) Install primary CD, for normal DMA traffic (SSID = 0).
> > +* (2) Install a secondary CD, for SID+SSID traffic.
> > +* (3) Update ASID of a CD. Atomically write the first 64 bits of the
> > +* CD, then invalidate the old entry and mappings.
> > +* (4) Remove a secondary CD.
> > */
> > - val = arm_smmu_cpu_tcr_to_cd(cfg->cd.tcr) |
> > -#ifdef __BIG_ENDIAN
> > - CTXDESC_CD_0_ENDI |
> > -#endif
> > - CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET |
> > - CTXDESC_CD_0_AA64 | FIELD_PREP(CTXDESC_CD_0_ASID, cfg->cd.asid) |
> > - CTXDESC_CD_0_V;
> > + u64 val;
> > + bool cd_live;
> > + struct arm_smmu_device *smmu = smmu_domain->smmu;
> > + __le64 *cdptr = smmu_domain->s1_cfg.table.ptr + ssid *
> > + CTXDESC_CD_DWORDS;
> >
> > - /* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
> > - if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> > - val |= CTXDESC_CD_0_S;
> > + val = le64_to_cpu(cdptr[0]);
> > + cd_live = !!(val & CTXDESC_CD_0_V);
> >
> > - cdptr[0] = cpu_to_le64(val);
> > + if (!cd) { /* (4) */
> > + val = 0;
> > + } else if (cd_live) { /* (3) */
> > + val &= ~CTXDESC_CD_0_ASID;
> > + val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);
> > + /*
> > +* Until CD+TLB invalidation, both ASIDs may be used for tagging
> > +* this substream's traffic
> > +*/
>
> I don't think you need to change anything here, but I do find it a little
> scary that we can modify live CDs like this. However, given that the
> hardware is permitted to cache the structures regardless of validity, it
> appears to be the only option. Terrifying!
>
> > + } else { /* (1) and (2) */
> > + cdptr[1] = cpu_to_le64(cd->ttbr & CTXDESC_CD_1_TTB0_MASK);
>
> Can you use FIELD_PREP here too?
No, FIELD_PREP will shift ttbr left by 4 bits
> > + cdptr[2] = 0;
> > + cdptr[3] = cpu_to_le64(cd->mair);
> > +
> > + /*
> > +* STE is live, and the SMMU might read dwords of this CD in any
> > +* order. Ensure that it observes valid values before reading
> > +* V=1.
> > +*/
> > + arm_smmu_sync_cd(smmu_domain, ssid, true);
> >
> > - val = cfg->cd.ttbr & CTXDESC_CD_1_TTB0_MASK;
> > - cdptr[1] = cpu_to_le64(val);
> > + val = arm_smmu_cpu_tcr_to_cd(cd->tcr) |
> > +#ifdef __BIG_ENDIAN
> > + CTXDESC_CD_0_ENDI |
> > +#endif
> > + CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET |
> > + CTXDESC_CD_0_AA64 |
> > + FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) |
> > + CTXDESC_CD_0_V;
> >
> > - cdptr[3] = cpu_to_le64(cfg->cd.mair);
> > + /* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
> > + if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> > + val |= CTXDESC_CD_0_S;
> > + }
> > +
> > +
Re: [PATCH v4 00/13] iommu: Add PASID support to Arm SMMUv3
On Thu, Dec 19, 2019 at 05:30:20PM +0100, Jean-Philippe Brucker wrote: > Add support for Substream ID and PASIDs to the SMMUv3 driver. Since v3 > [1], I added review and tested tags where appropriate and applied the > suggested changes, shown in the diff below. Thanks all! > > I'm testing using the zip accelerator on the Hisilicon KunPeng920 and > Zhangfei's uacce module [2]. The full SVA support, which I'll send out > early next year, is available on my branch sva/zip-devel at > https://jpbrucker.net/git/linux/ > > [1] > https://lore.kernel.org/linux-iommu/20191209180514.272727-1-jean-phili...@linaro.org/ > [2] > https://lore.kernel.org/linux-iommu/1576465697-27946-1-git-send-email-zhangfei@linaro.org/ > > Jean-Philippe Brucker (13): > iommu/arm-smmu-v3: Drop __GFP_ZERO flag from DMA allocation > dt-bindings: document PASID property for IOMMU masters > iommu/arm-smmu-v3: Parse PASID devicetree property of platform devices > ACPI/IORT: Parse SSID property of named component node > iommu/arm-smmu-v3: Prepare arm_smmu_s1_cfg for SSID support > iommu/arm-smmu-v3: Add context descriptor tables allocators > iommu/arm-smmu-v3: Add support for Substream IDs > iommu/arm-smmu-v3: Propagate ssid_bits > iommu/arm-smmu-v3: Prepare for handling arm_smmu_write_ctx_desc() > failure > iommu/arm-smmu-v3: Add second level of context descriptor table > iommu/arm-smmu-v3: Improve add_device() error handling > PCI/ATS: Add PASID stubs > iommu/arm-smmu-v3: Add support for PCI PASID > > .../devicetree/bindings/iommu/iommu.txt | 6 + > drivers/acpi/arm64/iort.c | 18 + > drivers/iommu/arm-smmu-v3.c | 467 +++--- > drivers/iommu/of_iommu.c | 6 +- > include/linux/iommu.h | 2 + > include/linux/pci-ats.h | 3 + > 6 files changed, 442 insertions(+), 60 deletions(-) This is close, and I've replied to all of the patches I have comments on. To summarise: 1-5 I could queue these now 6 I can make the small change we discussed 7 I can make the changes if you agree (but I'd prefer you to change to batch submission since I can't test this) 8 Good to go once above is solved 9 Need your opinion 10Some refactoring needed (sorry) 11Needs Robin's input 12Good to go once above is solved 13Need clarification on PCIe behaviour from you In other words, I could probably take the first 8 or 9 patches for 5.6 if you can resolve those issues asap. Cheers, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 11/13] iommu/arm-smmu-v3: Improve add_device() error handling
On Thu, Dec 19, 2019 at 05:30:31PM +0100, Jean-Philippe Brucker wrote:
> Let add_device() clean up after itself. The iommu_bus_init() function
> does call remove_device() on error, but other sites (e.g. of_iommu) do
> not.
>
> Don't free level-2 stream tables because we'd have to track if we
> allocated each of them or if they are used by other endpoints. It's not
> worth the hassle since they are managed resources.
>
> Reviewed-by: Eric Auger
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm-smmu-v3.c | 28 +---
> 1 file changed, 21 insertions(+), 7 deletions(-)
I think this is alright, with one caveat relating to:
/*
* We _can_ actually withstand dodgy bus code re-calling add_device()
* without an intervening remove_device()/of_xlate() sequence, but
* we're not going to do so quietly...
*/
if (WARN_ON_ONCE(fwspec->iommu_priv)) {
master = fwspec->iommu_priv;
smmu = master->smmu;
} ...
which may be on shakey ground if the subsequent add_device() call can fail
and free stuff that the first one allocated. At least, I don't know what
we're trying to support with this, so it's hard to tell whether or not it
still works as intended after your change.
How is this supposed to work? I don't recall ever seeing that WARN fire,
so can we just remove this and bail instead? Robin?
Something like below before your changes...
Will
--->8
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index effe72eb89e7..6ae3df2f3495 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2534,28 +2534,23 @@ static int arm_smmu_add_device(struct device *dev)
if (!fwspec || fwspec->ops != _smmu_ops)
return -ENODEV;
- /*
-* We _can_ actually withstand dodgy bus code re-calling add_device()
-* without an intervening remove_device()/of_xlate() sequence, but
-* we're not going to do so quietly...
-*/
- if (WARN_ON_ONCE(fwspec->iommu_priv)) {
- master = fwspec->iommu_priv;
- smmu = master->smmu;
- } else {
- smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
- if (!smmu)
- return -ENODEV;
- master = kzalloc(sizeof(*master), GFP_KERNEL);
- if (!master)
- return -ENOMEM;
- master->dev = dev;
- master->smmu = smmu;
- master->sids = fwspec->ids;
- master->num_sids = fwspec->num_ids;
- fwspec->iommu_priv = master;
- }
+ if (WARN_ON_ONCE(fwspec->iommu_priv))
+ return -EBUSY;
+
+ smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
+ if (!smmu)
+ return -ENODEV;
+
+ master = kzalloc(sizeof(*master), GFP_KERNEL);
+ if (!master)
+ return -ENOMEM;
+
+ master->dev = dev;
+ master->smmu = smmu;
+ master->sids = fwspec->ids;
+ master->num_sids = fwspec->num_ids;
+ fwspec->iommu_priv = master;
/* Check the SIDs are in range of the SMMU and our stream table */
for (i = 0; i < master->num_sids; i++) {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu: amd: Fix IOMMU perf counter clobbering during init
init_iommu_perf_ctr() clobbers the register when it checks write access
to IOMMU perf counters and fails to restore when they are writable.
Add save and restore to fix it.
Signed-off-by: Shuah Khan
---
drivers/iommu/amd_iommu_init.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 568c52317757..c0ad4f293522 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1655,27 +1655,37 @@ static int iommu_pc_get_set_reg(struct amd_iommu
*iommu, u8 bank, u8 cntr,
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
struct pci_dev *pdev = iommu->dev;
- u64 val = 0xabcd, val2 = 0;
+ u64 val = 0xabcd, val2 = 0, save_reg = 0;
if (!iommu_feature(iommu, FEATURE_PC))
return;
amd_iommu_pc_present = true;
+ /* save the value to restore, if writable */
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, false))
+ goto pc_false;
+
/* Check if the performance counters can be written to */
if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, , true)) ||
(iommu_pc_get_set_reg(iommu, 0, 0, 0, , false)) ||
- (val != val2)) {
- pci_err(pdev, "Unable to write to IOMMU perf counter.\n");
- amd_iommu_pc_present = false;
- return;
- }
+ (val != val2))
+ goto pc_false;
+
+ /* restore */
+ if (iommu_pc_get_set_reg(iommu, 0, 0, 0, _reg, true))
+ goto pc_false;
pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
iommu->max_banks = (u8) ((val >> 12) & 0x3f);
iommu->max_counters = (u8) ((val >> 7) & 0xf);
+
+pc_false:
+ pci_err(pdev, "Unable to read/write to IOMMU perf counter.\n");
+ amd_iommu_pc_present = false;
+ return;
}
static ssize_t amd_iommu_show_cap(struct device *dev,
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 10/13] iommu/arm-smmu-v3: Add second level of context descriptor table
On Thu, Dec 19, 2019 at 05:30:30PM +0100, Jean-Philippe Brucker wrote:
> The SMMU can support up to 20 bits of SSID. Add a second level of page
> tables to accommodate this. Devices that support more than 1024 SSIDs now
> have a table of 1024 L1 entries (8kB), pointing to tables of 1024 context
> descriptors (64kB), allocated on demand.
>
> Tested-by: Zhangfei Gao
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm-smmu-v3.c | 154 +---
> 1 file changed, 144 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index b825a5639afc..bf106a7b53eb 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -224,6 +224,7 @@
>
> #define STRTAB_STE_0_S1FMT GENMASK_ULL(5, 4)
> #define STRTAB_STE_0_S1FMT_LINEAR0
> +#define STRTAB_STE_0_S1FMT_64K_L22
> #define STRTAB_STE_0_S1CTXPTR_MASK GENMASK_ULL(51, 6)
> #define STRTAB_STE_0_S1CDMAX GENMASK_ULL(63, 59)
>
> @@ -263,7 +264,20 @@
>
> #define STRTAB_STE_3_S2TTB_MASK GENMASK_ULL(51, 4)
>
> -/* Context descriptor (stage-1 only) */
> +/*
> + * Context descriptors.
> + *
> + * Linear: when less than 1024 SSIDs are supported
> + * 2lvl: at most 1024 L1 entries,
> + * 1024 lazy entries per table.
> + */
> +#define CTXDESC_SPLIT10
> +#define CTXDESC_L2_ENTRIES (1 << CTXDESC_SPLIT)
> +
> +#define CTXDESC_L1_DESC_DWORDS 1
> +#define CTXDESC_L1_DESC_VALID1
#define CTXDESC_L1_DESC_V (1UL << 0)
fits better with the rest of the driver and also ensures that the thing
is unsigned (we should probably switch over the BIT macros, but that's a
separate cleanup patch).
> +#define CTXDESC_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 12)
> +
> #define CTXDESC_CD_DWORDS8
> #define CTXDESC_CD_0_TCR_T0SZGENMASK_ULL(5, 0)
> #define ARM64_TCR_T0SZ GENMASK_ULL(5, 0)
> @@ -575,7 +589,12 @@ struct arm_smmu_cd_table {
> };
>
> struct arm_smmu_s1_cfg {
> - struct arm_smmu_cd_tabletable;
> + /* Leaf tables or linear table */
> + struct arm_smmu_cd_table*tables;
> + size_t num_tables;
> + /* First level tables, when two levels are used */
> + __le64 *l1ptr;
> + dma_addr_t l1ptr_dma;
It probably feels like a nit, but I think this is all a little hard to read
because it differs unnecessarily from the way the stream table is handled.
Could we align the two as follows? (I've commented things with what they
refer to in your patch):
struct arm_smmu_l1_ctx_desc { // arm_smmu_cd_table
__le64 *l2ptr; // ptr
dma_addr_t l2ptr_dma; // ptr_dma
};
struct arm_smmu_ctx_desc_cfg {
__le64 *cdtab; // l1ptr
dma_addr_t cdtab_dma; // l1ptr_dma
struct arm_smmu_l1_ctx_desc *l1_desc; // tables
unsigned intnum_l1_ents;// num_tables
};
struct arm_smmu_s1_cfg {
struct arm_smmu_ctx_desc_cfgcdcfg;
struct arm_smmu_ctx_desccd;
u8 s1fmt;
u8 s1cdmax;
};
I don't know whether you'd then want to move s1fmt and s1cdmax into the
cdcfg, I'll leave that up to you. Similarly if you want any functions
to operate on arm_smmu_ctx_desc_cfg in preference to arm_smmu_s1_cfg.
Thoughts?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 2/4] uacce: add uacce driver
On Mon, Jan 13, 2020 at 11:34:55AM +0800, zhangfei wrote:
> Hi, Greg
>
> Thanks for the review.
>
> On 2020/1/12 上午3:40, Greg Kroah-Hartman wrote:
> > On Sat, Jan 11, 2020 at 10:48:37AM +0800, Zhangfei Gao wrote:
> > > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > > +{
> > > + struct uacce_mm *uacce_mm = NULL;
> > > + struct uacce_device *uacce;
> > > + struct uacce_queue *q;
> > > + int ret = 0;
> > > +
> > > + uacce = xa_load(_xa, iminor(inode));
> > > + if (!uacce)
> > > + return -ENODEV;
> > > +
> > > + if (!try_module_get(uacce->parent->driver->owner))
> > > + return -ENODEV;
> > Why are you trying to grab the module reference of the parent device?
> > Why is that needed and what is that going to help with here?
> >
> > This shouldn't be needed as the module reference of the owner of the
> > fileops for this module is incremented, and the "parent" module depends
> > on this module, so how could it be unloaded without this code being
> > unloaded?
> >
> > Yes, if you build this code into the kernel and the "parent" driver is a
> > module, then you will not have a reference, but when you remove that
> > parent driver the device will be removed as it has to be unregistered
> > before that parent driver can be removed from the system, right?
> >
> > Or what am I missing here?
> The refcount here is preventing rmmod "parent" module after fd is opened,
> since user driver has mmap kernel memory to user space, like mmio, which may
> still in-use.
>
> With the refcount protection, rmmod "parent" module will fail until
> application free the fd.
> log like: rmmod: ERROR: Module hisi_zip is in use
But if the "parent" module is to be unloaded, it has to unregister the
"child" device and that will call the destructor in here and then you
will tear everything down and all should be good.
There's no need to "forbid" a module from being unloaded, even if it is
being used. Look at all networking drivers, they work that way, right?
> > > +static void uacce_release(struct device *dev)
> > > +{
> > > + struct uacce_device *uacce = to_uacce_device(dev);
> > > +
> > > + kfree(uacce);
> > > + uacce = NULL;
> > That line didn't do anything :)
> Yes, this is a mistake.
> It is up to caller to set to NULL to prevent release multi times.
Release function is called by the driver core which will not touch the
value again.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 13/13] iommu/arm-smmu-v3: Add support for PCI PASID
On Thu, Dec 19, 2019 at 05:30:33PM +0100, Jean-Philippe Brucker wrote:
> Enable PASID for PCI devices that support it. Since the SSID tables are
> allocated by arm_smmu_attach_dev(), PASID has to be enabled early enough.
> arm_smmu_dev_feature_enable() would be too late, since by that time the
What is arm_smmu_dev_feature_enable()?
> main DMA domain has already been attached. Do it in add_device() instead.
>
> Tested-by: Zhangfei Gao
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm-smmu-v3.c | 55 -
> 1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index e62ca80f2f76..8e95ecad4c9a 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2644,6 +2644,53 @@ static void arm_smmu_disable_ats(struct
> arm_smmu_master *master)
> atomic_dec(_domain->nr_ats_masters);
> }
>
> +static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
> +{
> + int ret;
> + int features;
> + int num_pasids;
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(master->dev))
> + return -ENODEV;
> +
> + pdev = to_pci_dev(master->dev);
> +
> + features = pci_pasid_features(pdev);
> + if (features < 0)
> + return features;
> +
> + num_pasids = pci_max_pasids(pdev);
> + if (num_pasids <= 0)
> + return num_pasids;
> +
> + ret = pci_enable_pasid(pdev, features);
> + if (ret) {
> + dev_err(>dev, "Failed to enable PASID\n");
> + return ret;
> + }
> +
> + master->ssid_bits = min_t(u8, ilog2(num_pasids),
> + master->smmu->ssid_bits);
> + return 0;
> +}
> +
> +static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> +{
> + struct pci_dev *pdev;
> +
> + if (!dev_is_pci(master->dev))
> + return;
> +
> + pdev = to_pci_dev(master->dev);
> +
> + if (!pdev->pasid_enabled)
> + return;
> +
> + master->ssid_bits = 0;
> + pci_disable_pasid(pdev);
> +}
> +
> static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> {
> unsigned long flags;
> @@ -2852,13 +2899,16 @@ static int arm_smmu_add_device(struct device *dev)
>
> master->ssid_bits = min(smmu->ssid_bits, fwspec->num_pasid_bits);
>
> + /* Note that PASID must be enabled before, and disabled after ATS */
> + arm_smmu_enable_pasid(master);
Is that part of the PCIe specs? If so, please can you add a citation to the
comment?
Are there any other ordering requirements, i.e. with respect to enabling
substreams at the SMMU? For example, can a speculative ATS request provide
a PASID?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Prepare for handling arm_smmu_write_ctx_desc() failure
On Thu, Dec 19, 2019 at 05:30:29PM +0100, Jean-Philippe Brucker wrote: > Second-level context descriptor tables will be allocated lazily in > arm_smmu_write_ctx_desc(). Help with handling allocation failure by > moving the CD write into arm_smmu_domain_finalise_s1(). > > Reviewed-by: Eric Auger > Reviewed-by: Jonathan Cameron > Signed-off-by: Jean-Philippe Brucker > --- > drivers/iommu/arm-smmu-v3.c | 11 +++ > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index e147087198ef..b825a5639afc 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2301,8 +2301,15 @@ static int arm_smmu_domain_finalise_s1(struct > arm_smmu_domain *smmu_domain, > cfg->cd.ttbr= pgtbl_cfg->arm_lpae_s1_cfg.ttbr[0]; > cfg->cd.tcr = pgtbl_cfg->arm_lpae_s1_cfg.tcr; > cfg->cd.mair= pgtbl_cfg->arm_lpae_s1_cfg.mair; > + > + ret = arm_smmu_write_ctx_desc(smmu_domain, 0, >cd); Hmm. This ends up calling arm_smmu_sync_cd() but I think that happens before we've added the master to the devices list of the domain. Does that mean we miss the new SSID during the invalidation? > + if (ret) > + goto out_free_tables; > + > return 0; > > +out_free_tables: nit: We have more tables in this driver than you can shake a stick at, so please rename the label "out_free_cd_tables" or something like that. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Add support for Substream IDs
On Thu, Dec 19, 2019 at 05:30:27PM +0100, Jean-Philippe Brucker wrote:
> At the moment, the SMMUv3 driver implements only one stage-1 or stage-2
> page directory per device. However SMMUv3 allows more than one address
> space for some devices, by providing multiple stage-1 page directories. In
> addition to the Stream ID (SID), that identifies a device, we can now have
> Substream IDs (SSID) identifying an address space. In PCIe, SID is called
> Requester ID (RID) and SSID is called Process Address-Space ID (PASID).
> A complete stage-1 walk goes through the context descriptor table:
>
> Stream tables Ctx. Desc. tables Page tables
> ++ ,--->+---+ ,--->+---+
> :: |: : |: :
> ++ |+---+ |+---+
>SID->| STE |---' SSID->| CD |---' IOVA->| PTE |--> IPA
> +++---++---+
> ::: :: :
> +++---++---+
>
> Rewrite arm_smmu_write_ctx_desc() to modify context descriptor table
> entries. To keep things simple we only implement one level of context
> descriptor tables here, but as with stream and page tables, an SSID can
> be split to index multiple levels of tables.
>
> Tested-by: Zhangfei Gao
> Reviewed-by: Eric Auger
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm-smmu-v3.c | 125 +---
> 1 file changed, 102 insertions(+), 23 deletions(-)
--->8
> @@ -1456,6 +1472,33 @@ static int arm_smmu_cmdq_issue_sync(struct
> arm_smmu_device *smmu)
> }
>
> /* Context descriptor manipulation functions */
> +static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> + int ssid, bool leaf)
> +{
> + size_t i;
> + unsigned long flags;
> + struct arm_smmu_master *master;
> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> + struct arm_smmu_cmdq_ent cmd = {
> + .opcode = CMDQ_OP_CFGI_CD,
> + .cfgi = {
> + .ssid = ssid,
> + .leaf = leaf,
> + },
> + };
> +
> + spin_lock_irqsave(_domain->devices_lock, flags);
> + list_for_each_entry(master, _domain->devices, domain_head) {
> + for (i = 0; i < master->num_sids; i++) {
> + cmd.cfgi.sid = master->sids[i];
> + arm_smmu_cmdq_issue_cmd(smmu, );
> + }
> + }
> + spin_unlock_irqrestore(_domain->devices_lock, flags);
> +
> + arm_smmu_cmdq_issue_sync(smmu);
Can you send a follow-up patch converting this to batch submission, please?
> +}
> +
> static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> struct arm_smmu_cd_table *table,
> size_t num_entries)
> @@ -1498,34 +1541,65 @@ static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr)
> return val;
> }
>
> -static void arm_smmu_write_ctx_desc(struct arm_smmu_device *smmu,
> - struct arm_smmu_s1_cfg *cfg)
> +static int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain,
> +int ssid, struct arm_smmu_ctx_desc *cd)
> {
> - u64 val;
> - __le64 *cdptr = cfg->table.ptr;
> -
> /*
> - * We don't need to issue any invalidation here, as we'll invalidate
> - * the STE when installing the new entry anyway.
> + * This function handles the following cases:
> + *
> + * (1) Install primary CD, for normal DMA traffic (SSID = 0).
> + * (2) Install a secondary CD, for SID+SSID traffic.
> + * (3) Update ASID of a CD. Atomically write the first 64 bits of the
> + * CD, then invalidate the old entry and mappings.
> + * (4) Remove a secondary CD.
>*/
> - val = arm_smmu_cpu_tcr_to_cd(cfg->cd.tcr) |
> -#ifdef __BIG_ENDIAN
> - CTXDESC_CD_0_ENDI |
> -#endif
> - CTXDESC_CD_0_R | CTXDESC_CD_0_A | CTXDESC_CD_0_ASET |
> - CTXDESC_CD_0_AA64 | FIELD_PREP(CTXDESC_CD_0_ASID, cfg->cd.asid) |
> - CTXDESC_CD_0_V;
> + u64 val;
> + bool cd_live;
> + struct arm_smmu_device *smmu = smmu_domain->smmu;
> + __le64 *cdptr = smmu_domain->s1_cfg.table.ptr + ssid *
> + CTXDESC_CD_DWORDS;
>
> - /* STALL_MODEL==0b10 && CD.S==0 is ILLEGAL */
> - if (smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
> - val |= CTXDESC_CD_0_S;
> + val = le64_to_cpu(cdptr[0]);
> + cd_live = !!(val & CTXDESC_CD_0_V);
>
> - cdptr[0] = cpu_to_le64(val);
> + if (!cd) { /* (4) */
> + val = 0;
> + } else if (cd_live) { /* (3) */
> + val &= ~CTXDESC_CD_0_ASID;
> + val |= FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid);
> + /*
> +
Re: [PATCH v4 06/13] iommu/arm-smmu-v3: Add context descriptor tables allocators
On Tue, Jan 14, 2020 at 12:52:30PM +0100, Jean-Philippe Brucker wrote:
> On Tue, Jan 14, 2020 at 11:06:52AM +, Will Deacon wrote:
> > > /* Context descriptor manipulation functions */
> > > +static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> > > + struct arm_smmu_cd_table *table,
> > > + size_t num_entries)
> > > +{
> > > + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> > > +
> > > + table->ptr = dmam_alloc_coherent(smmu->dev, size, >ptr_dma,
> > > + GFP_KERNEL);
> > > + if (!table->ptr) {
> > > + dev_warn(smmu->dev,
> > > + "failed to allocate context descriptor table\n");
> > > + return -ENOMEM;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static void arm_smmu_free_cd_leaf_table(struct arm_smmu_device *smmu,
> > > + struct arm_smmu_cd_table *table,
> > > + size_t num_entries)
> > > +{
> > > + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> > > +
> > > + dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma);
> > > +}
> >
> > I think we'd be better off taking the 'arm_smmu_s1_cfg' as a parameter here
> > instead of the table pointer and a num_entries value, since the code above
> > implies that we support partial freeing of the context descriptors.
> >
> > I can do that as a follow-up patch if you agree. Thoughts?
>
> Do you mean only changing the arguments of arm_smmu_free_cd_leaf_table(),
> or arm_smmu_alloc_cd_leaf_table() as well? For free() I agree, for alloc()
> I'm not sure it would look better.
Yeah, just for free(). I'll spin something on top after I've finished
reviewing the series.
> For my tests I have a debug patch that allocates PASIDs randomly which
> quickly consumes DMA for leaf tables. So I do have to free the leaves
> individually when they aren't used, but it will be easy for me to update.
Cool.
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 06/13] iommu/arm-smmu-v3: Add context descriptor tables allocators
On Tue, Jan 14, 2020 at 11:06:52AM +, Will Deacon wrote:
> > /* Context descriptor manipulation functions */
> > +static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> > + struct arm_smmu_cd_table *table,
> > + size_t num_entries)
> > +{
> > + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> > +
> > + table->ptr = dmam_alloc_coherent(smmu->dev, size, >ptr_dma,
> > +GFP_KERNEL);
> > + if (!table->ptr) {
> > + dev_warn(smmu->dev,
> > +"failed to allocate context descriptor table\n");
> > + return -ENOMEM;
> > + }
> > + return 0;
> > +}
> > +
> > +static void arm_smmu_free_cd_leaf_table(struct arm_smmu_device *smmu,
> > + struct arm_smmu_cd_table *table,
> > + size_t num_entries)
> > +{
> > + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> > +
> > + dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma);
> > +}
>
> I think we'd be better off taking the 'arm_smmu_s1_cfg' as a parameter here
> instead of the table pointer and a num_entries value, since the code above
> implies that we support partial freeing of the context descriptors.
>
> I can do that as a follow-up patch if you agree. Thoughts?
Do you mean only changing the arguments of arm_smmu_free_cd_leaf_table(),
or arm_smmu_alloc_cd_leaf_table() as well? For free() I agree, for alloc()
I'm not sure it would look better.
For my tests I have a debug patch that allocates PASIDs randomly which
quickly consumes DMA for leaf tables. So I do have to free the leaves
individually when they aren't used, but it will be easy for me to update.
Thanks,
Jean
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 06/13] iommu/arm-smmu-v3: Add context descriptor tables allocators
On Thu, Dec 19, 2019 at 05:30:26PM +0100, Jean-Philippe Brucker wrote:
> Support for SSID will require allocating context descriptor tables. Move
> the context descriptor allocation to separate functions.
>
> Reviewed-by: Eric Auger
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/iommu/arm-smmu-v3.c | 57 ++---
> 1 file changed, 46 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index b287e303b1d7..43d6a7ded6e4 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -568,6 +568,7 @@ struct arm_smmu_cd_table {
> struct arm_smmu_s1_cfg {
> struct arm_smmu_cd_tabletable;
> struct arm_smmu_ctx_desccd;
> + u8 s1cdmax;
> };
>
> struct arm_smmu_s2_cfg {
> @@ -1455,6 +1456,31 @@ static int arm_smmu_cmdq_issue_sync(struct
> arm_smmu_device *smmu)
> }
>
> /* Context descriptor manipulation functions */
> +static int arm_smmu_alloc_cd_leaf_table(struct arm_smmu_device *smmu,
> + struct arm_smmu_cd_table *table,
> + size_t num_entries)
> +{
> + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> +
> + table->ptr = dmam_alloc_coherent(smmu->dev, size, >ptr_dma,
> + GFP_KERNEL);
> + if (!table->ptr) {
> + dev_warn(smmu->dev,
> + "failed to allocate context descriptor table\n");
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +
> +static void arm_smmu_free_cd_leaf_table(struct arm_smmu_device *smmu,
> + struct arm_smmu_cd_table *table,
> + size_t num_entries)
> +{
> + size_t size = num_entries * (CTXDESC_CD_DWORDS << 3);
> +
> + dmam_free_coherent(smmu->dev, size, table->ptr, table->ptr_dma);
> +}
I think we'd be better off taking the 'arm_smmu_s1_cfg' as a parameter here
instead of the table pointer and a num_entries value, since the code above
implies that we support partial freeing of the context descriptors.
I can do that as a follow-up patch if you agree. Thoughts?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] arm: use swiotlb for bounce buffer on LPAE configs
Christoph, Robin,
On 09/01/2020 16.49, Christoph Hellwig wrote:
> On Wed, Jan 08, 2020 at 03:20:07PM +, Robin Murphy wrote:
>>> The problem - I think - is that the DMA_BIT_MASK(32) from
>>> dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)) is treated as physical
>>> address along the call path so the dma_pfn_offset is applied to it and
>>> the check will fail, saying that DMA_BIT_MASK(32) can not be supported.
>>
>> But that's the thing - in isolation, that is entirely correct. Considering
>> ZONE_DMA32 for simplicity, in general the zone is expected to cover the
>> physical address range 0x_ - 0x_ (because DMA offsets are
>> relatively rare), and a device with a dma_pfn_offset of more than
>> (0x1__ >> PAGE_SHIFT) *cannot* support that range with any mask,
>> because the DMA address itself would have to be negative.
>
> Note that ZONE_DMA32 is irrelevant in this particular case, as we are
> talking about arm32. But with ZONE_DMA instead this roughly makes sense.
>
>> The problem is that platforms with esoteric memory maps have no right thing
>> to do. If the base of RAM is at at 0x1__ or higher, the "correct"
>> ZONE_DMA32 would be empty while ZONE_NORMAL above it would not, and last
>> time I looked that makes the page allocator break badly. So the standard
>> bodge on such platforms is to make ZONE_DMA32 cover not the first 4GB of
>> *PA space*, but the first 4GB of *RAM*, wherever that happens to be. That
>> then brings different problems - now the page allocator is happy and
>> successfully returns GFP_DMA32 allocations from the range 0x8__ -
>> 0x8__ that are utterly useless to 32-bit devices with zero
>> dma_pfn_offset - see the AMD Seattle SoC for the prime example of that. If
>> on the other hand all devices are guaranteed to have a dma_pfn_offset that
>> puts the base of RAM at DMA address 0 then GFP_DMA32 allocations do end up
>> working as expected, but now the original assumption of where ZONE_DMA32
>> actually is is broken, so generic code unaware of the
>> platform/architecture-specific bodge will be misled - that's the case
>> you're running into.
>>
>> Having thought this far, if there's a non-hacky way to reach in and grab
>> ZONE_DMA{32} such that dma_direct_supported() could use zone_end_pfn()
>> instead of trying to assume either way, that might be the most robust
>> general solution.
>
> zone_dma_bits is our somewhat ugly way to try to poke into this
> information, although the way it is done right now sucks pretty badly.
In my view the handling of dma_pfn_offset is just incorrect as it is applied to
_any_ address.
According to DT specification dma-ranges:
"Value type: or encoded as an arbitrary
number of (child-bus-address, parent-bus-address, length) triplets."
Yet in drivers/of/ we only take the _first_ triplet and ignore the rest.
The dma_pfn_offset should be only applied to paddr in the range:
parent-bus-address to parent-bus-address+length
for anything outside of this the dma_pfn_offset is 0.
conversion back from dma to paddr should consider the offset in range:
child-bus-address to child-bus-address+length
and 0 for everything outside of this.
To correctly handle the dma-ranges we would need something like this in
device.h:
+struct dma_ranges {
+ u64 paddr;
+ u64 dma_addr;
+ u64 size;
+ unsigned long pfn_offset;
+};
+
struct device {
...
- unsigned long dma_pfn_offset;
+ struct dma_ranges *dma_ranges;
int dma_ranges_cnt;
...
};
Then when we currently use dma_pfn_offset we would have:
unsigned long __phys_to_dma_pfn_offset(struct device *dev, phys_addr_t paddr)
{
int i;
if (!dev->dma_ranges)
return 0;
for (i = 0; i < dev->dma_ranges_cnt; i++) {
struct dma_ranges *range = >dma_ranges[i];
if (paddr >= range->paddr &&
paddr <= (range->paddr + range->size))
return range->pfn_offset;
}
return 0;
}
unsigned long __dma_to_phys_pfn_offset(struct device *dev, dma_addr_t dma_addr)
{
int i;
for (i = 0; i < dev->dma_ranges_cnt; i++) {
struct dma_ranges *range = >dma_ranges[i];
if (dma_addr >= range->dma_addr &&
dma_addr <= (range->dma_addr + range->size))
return range->pfn_offset;
}
return 0;
}
For existing drivers/archs setting dma_pfn_offset we can:
if (dev->dma_ranges_cnt == 1 && dev->dma_ranges[0].pfn_offset &&
!dev->dma_ranges[0].size)
return dev->dma_ranges[0].pfn_offset;
and they would have to set up one struct dma_ranges.
One of the issue with this is that the struct dma_ranges would need to be
allocated for
all devices, so there should be a some clever way need to be invented to use
pointers
as much as we can.
> The patch I sent to Peter in December was trying to convey that
Re: [PATCH v3 4/5] PCI: vmd: Stop overriding dma_map_ops
On Fri, Jan 10, 2020 at 10:21:12AM -0700, Jon Derrick wrote:
> Devices on the VMD domain use the VMD endpoint's requester ID and have
> been relying on the VMD endpoint's DMA operations. The problem with this
> was that VMD domain devices would use the VMD endpoint's attributes when
> doing DMA and IOMMU mapping. We can be smarter about this by only using
> the VMD endpoint when mapping and providing the correct child device's
> attributes during DMA operations.
>
> This patch modifies Intel-IOMMU to check for a 'Direct DMA Alias' and
> refer to it for mapping.
>
> Signed-off-by: Jon Derrick
> ---
> drivers/iommu/intel-iommu.c| 18 +++--
> drivers/pci/controller/Kconfig | 1 -
> drivers/pci/controller/vmd.c | 150
> -
> 3 files changed, 13 insertions(+), 156 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 716347e2..7ca807a 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -804,14 +804,14 @@ static struct intel_iommu *device_to_iommu(struct
> device *dev, u8 *bus, u8 *devf
>
> if (dev_is_pci(dev)) {
> struct pci_dev *pf_pdev;
> + struct pci_dev *dma_alias;
>
> pdev = to_pci_dev(dev);
>
> -#ifdef CONFIG_X86
> - /* VMD child devices currently cannot be handled individually */
> - if (is_vmd(pdev->bus))
> - return NULL;
> -#endif
Don't we need this sanity check to prevent assingning vmd subdevices?
> + /* DMA aliased devices use the DMA alias's IOMMU */
> + dma_alias = pci_direct_dma_alias(pdev);
> + if (dma_alias)
> + pdev = dma_alias;
>
> /* VFs aren't listed in scope tables; we need to look up
>* the PF instead to find the IOMMU. */
> @@ -2521,6 +2521,14 @@ struct dmar_domain *find_domain(struct device *dev)
>dev->archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO))
> return NULL;
>
> + if (dev_is_pci(dev)) {
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct pci_dev *dma_alias = pci_direct_dma_alias(pdev);
> +
> + if (dma_alias)
> + dev = _alias->dev;
Instead of all these duplicate calls, shouldn't pci_direct_dma_alias be
replaced with a pci_real_dma_dev helper that either returns the
dma parent if it exiѕts, or the actual device?
Also I think this patch should be split - one for intel-iommu that
just adds the real device checks, and then one that wires up vmd to
the new mechanism and then removes all the cruft.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/5] PCI: Introduce pci_direct_dma_alias()
On Fri, Jan 10, 2020 at 10:21:11AM -0700, Jon Derrick wrote:
> The current DMA alias implementation requires the aliased device be on
> the same PCI bus as the requester ID. This introduces an arch-specific
> mechanism to point to another PCI device when doing mapping and
> PCI DMA alias search.
>
> Signed-off-by: Jon Derrick
> ---
> arch/x86/pci/common.c | 7 +++
> drivers/pci/pci.c | 19 ++-
> drivers/pci/search.c | 7 +++
> include/linux/pci.h | 1 +
> 4 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 1e59df0..83334a5 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -736,3 +736,10 @@ int pci_ext_cfg_avail(void)
> else
> return 0;
> }
> +
> +#if IS_ENABLED(CONFIG_VMD)
> +struct pci_dev *pci_direct_dma_alias(struct pci_dev *dev)
> +{
> + return to_pci_sysdata(dev->bus)->vmd_dev;
> +}
> +#endif
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ad746d9..1362694 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6034,7 +6034,9 @@ bool pci_devs_are_dma_aliases(struct pci_dev *dev1,
> struct pci_dev *dev2)
> return (dev1->dma_alias_mask &&
> test_bit(dev2->devfn, dev1->dma_alias_mask)) ||
> (dev2->dma_alias_mask &&
> - test_bit(dev1->devfn, dev2->dma_alias_mask));
> + test_bit(dev1->devfn, dev2->dma_alias_mask)) ||
> +(pci_direct_dma_alias(dev1) == dev2) ||
> +(pci_direct_dma_alias(dev2) == dev1);
No need for the inner braces here.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/5] x86/PCI: Expose VMD's PCI Device in pci_sysdata
On Fri, Jan 10, 2020 at 10:21:10AM -0700, Jon Derrick wrote: > To be used by Intel-IOMMU code to find the correct domain. > > CC: Christoph Hellwig > Signed-off-by: Jon Derrick Looks good, modulo the brace bisection issue already pointed out: Reviewed-by: Christoph Hellwig ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] x86/pci: Add a to_pci_sysdata helper
On Fri, Jan 10, 2020 at 10:21:09AM -0700, Jon Derrick wrote: > From: Christoph Hellwig > > Various helpers need the pci_sysdata just to dereference a single field > in it. Add a little helper that returns the properly typed sysdata > pointer to require a little less boilerplate code. > > Signed-off-by: Christoph Hellwig > [jonathan.derrick: added un-const cast] > + return to_pci_sysdata((struct pci_bus *) bus)->node; Instead of this case I think we'd be better off marking the bus argument to to_pci_sysdata const. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
