Re: [PATCH v2] swiotlb: Adjust SWIOTBL bounce buffer size for SEV guests.
On Tue, Jan 21, 2020 at 03:54:03PM -0500, Konrad Rzeszutek Wilk wrote: > > > > Additional memory calculations based on # of PCI devices and > > their memory ranges will make it more complicated with so > > many other permutations and combinations to explore, it is > > essential to keep this patch as simple as possible by > > adjusting the bounce buffer size simply by determining it > > from the amount of provisioned guest memory. >> >> Please rework the patch to: >> >> - Use a log solution instead of the multiplication. >>Feel free to cap it at a sensible value. Ok. >> >> - Also the code depends on SWIOTLB calling in to the >>adjust_swiotlb_default_size which looks wrong. >> >>You should not adjust io_tlb_nslabs from swiotlb_size_or_default. >>That function's purpose is to report a value. >> >> - Make io_tlb_nslabs be visible outside of the SWIOTLB code. >> >> - Can you utilize the IOMMU_INIT APIs and have your own detect which would >>modify the io_tlb_nslabs (and set swiotbl=1?). This seems to be a nice option, but then IOMMU_INIT APIs are x86-specific and this swiotlb buffer size adjustment is also needed for other memory encryption architectures like Power, S390, etc. >> >>Actually you seem to be piggybacking on pci_swiotlb_detect_4gb - so >>perhaps add in this code ? Albeit it really should be in it's own >>file, not in arch/x86/kernel/pci-swiotlb.c Actually, we piggyback on pci_swiotlb_detect_override which sets swiotlb=1 as x86_64_start_kernel() and invocation of sme_early_init() forces swiotlb on, but again this is all x86 architecture specific. Thanks, Ashish ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 0/7] Clean up VMD DMA Map Ops
On Tue, Jan 21, 2020 at 06:37:44AM -0700, Jon Derrick wrote: > v4 Set: > https://lore.kernel.org/linux-pci/20200120110220.gb17...@e121166-lin.cambridge.arm.com/T/#t > v3 Set: > https://lore.kernel.org/linux-iommu/20200113181742.ga27...@e121166-lin.cambridge.arm.com/T/#t > v2 Set: > https://lore.kernel.org/linux-iommu/1578580256-3483-1-git-send-email-jonathan.derr...@intel.com/T/#t > v1 Set: > https://lore.kernel.org/linux-iommu/20200107134125.gd30...@8bytes.org/T/#t > > VMD currently works with VT-d enabled by pointing DMA and IOMMU actions at the > VMD endpoint. The problem with this approach is that the VMD endpoint's > device-specific attributes, such as the DMA Mask Bits, are used instead of the > child device's attributes. > > This set cleans up VMD by removing the override that redirects DMA map > operations to the VMD endpoint. Instead it introduces a new DMA alias > mechanism > into the existing DMA alias infrastructure. This new DMA alias mechanism > allows > an architecture-specific pci_real_dma_dev() function to provide a pointer from > a pci_dev to its PCI DMA device, where by default it returns the original > pci_dev. > > In addition, this set removes the sanity check that was added to prevent > assigning VMD child devices. By using the DMA alias mechanism, all child > devices are assigned the same IOMMU group as the VMD endpoint. This removes > the > need for restricting VMD child devices from assignment, as the whole group > would have to be assigned, requiring unbinding the VMD driver and removing the > child device domain. > > v1 added a pointer in struct pci_dev that pointed to the DMA alias' struct > pci_dev and did the necessary DMA alias and IOMMU modifications. > > v2 introduced a new weak function to reference the 'Direct DMA Alias', and > removed the need to add a pointer in struct device or pci_dev. Weak functions > are generally frowned upon when it's a single architecture implementation, so > I > am open to alternatives. > > v3 referenced the pci_dev rather than the struct device for the PCI > 'Direct DMA Alias' (pci_direct_dma_alias()). This revision also allowed > pci_for_each_dma_alias() to call any DMA aliases for the Direct DMA alias > device, though I don't expect the VMD endpoint to need intra-bus DMA aliases. > > v4 changes the 'Direct DMA Alias' to instead refer to the 'Real DMA Dev', > which > either returns the PCI device itself or the PCI DMA device. > > v5 Fixes a bad call argument to pci_real_dma_dev that would have broken > bisection. This revision also changes one of the calls to a one-liner, and > assembles the same on my system. > > > Changes from v4: > Fix pci_real_dma_dev() call in 4/7. > Change other pci_real_dma_dev() call in 4/7 to one-liner. > > Changes from v3: > Uses pci_real_dma_dev() instead of pci_direct_dma_alias() > Split IOMMU enabling, IOMMU VMD sanity check and VMD dma_map_ops cleanup into > three patches > > Changes from v2: > Uses struct pci_dev for PCI Device 'Direct DMA aliasing' > (pci_direct_dma_alias) > Allows pci_for_each_dma_alias to iterate over the alias mask of the 'Direct > DMA alias' > > Changes from v1: > Removed 1/5 & 2/5 misc fix patches that were merged > Uses Christoph's staging/cleanup patches > Introduce weak function rather than including pointer in struct device or > pci_dev. > > Based on Bjorn's next: > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=next > > Christoph Hellwig (2): > x86/PCI: Add a to_pci_sysdata helper > x86/PCI: Remove X86_DEV_DMA_OPS > > Jon Derrick (5): > x86/PCI: Expose VMD's PCI Device in pci_sysdata > PCI: Introduce pci_real_dma_dev() > iommu/vt-d: Use pci_real_dma_dev() for mapping > iommu/vt-d: Remove VMD child device sanity check > PCI: vmd: Stop overriding dma_map_ops > > arch/x86/Kconfig | 3 - > arch/x86/include/asm/device.h | 10 --- > arch/x86/include/asm/pci.h | 31 - > arch/x86/pci/common.c | 48 +++-- > drivers/iommu/intel-iommu.c| 11 ++- > drivers/pci/controller/Kconfig | 1 - > drivers/pci/controller/vmd.c | 152 > + > drivers/pci/pci.c | 19 +- > drivers/pci/search.c | 6 ++ > include/linux/pci.h| 1 + > 10 files changed, 54 insertions(+), 228 deletions(-) Applied with acks/reviewed-by from Lu, Keith, and Lorenzo to pci/host-vmd for v5.6, thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [git pull] IOMMU Fixes for Linux v5.5-rc7
The pull request you sent on Fri, 24 Jan 2020 18:29:44 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git > tags/iommu-fixes-v5.5-rc7 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/6381b442836ea3c52eae630b10be8c27c7a17af2 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
On 1/23/20 11:43 PM, Suravee Suthikulpanit wrote:
On 1/24/20 5:32 AM, Shuah Khan wrote:
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
---
Changes since v1:
-- Fix bug in sucessful return path. Add a return instead of
fall through to pc_false error case
drivers/iommu/amd_iommu_init.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c
b/drivers/iommu/amd_iommu_init.c
index 568c52317757..483f7bc379fa 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1655,27 +1655,39 @@ 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);
+
+ return;
+
Good catch. Sorry, I missed this part as well :(
+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,
As for your question in v1:
> I see 2 banks and 4 counters on my system. Is it sufficient to check
> the first bank and first counter? In other words, if the first one
> isn't writable, are all counters non-writable?
We currently assume all counters have the same write-ability. So, it
should be sufficient
to just check the first one.
> Should we read the config first and then, try to see if any of the
> counters are writable? I have a patch that does that, I can send it
> out for review.
Which config are you referring to? Not sure what you mean.
I mean reading MMIO_CNTR_CONF_OFFSET to get max banks and counters.
Also what is the reason to check writable?
I tried a couplf og things on my
AMD Ryzen 5 PRO 2400GE w/ Radeon Vega Graphics
I changed the logic to read config to get max banks and counters
before checking if counters are writable and tried writing to all.
The result is the same and all of them aren't writable. However,
when disable the writable check and assume they are, I can run
perf stat -e 'amd_iommu_0 on all events and get data.
perf stat -e 'amd_iommu_0/cmd_processed/' sleep 10
Performance counter stats for 'system wide':
56 amd_iommu_0/cmd_processed/
10.001525171 seconds time elapsed
perf stat -a -e amd_iommu/mem_trans_total/ sleep 10
Performance counter stats for 'system wide':
2,696 amd_iommu/mem_trans_total/
10.001465115 seconds time elapsed
I tried all possible events listed under amd_iommu_0 and I can get
data on all of them. No problems in dmesg.
Is this inline with what you expect? I am curious what this write
tell us and can we enable read only mode on these counters?
By the way, here is the output from booting the system with this patch
(+ debug messages).
[ 14.408834] pci :60:00.2: AMD-Vi: IOMMU performance counters
supported
[ 14.416526] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[ 14.429602] pci :40:00.2: AMD-Vi: IOMMU performance counters
supported
[ 14.437275] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[ 14.450320] pci :20:00.2: AMD-Vi: IOMMU performance counters
supported
[ 14.457991] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
[ 14.471049] pci :00:00.2: AMD-Vi: IOMMU performance counters
supported
[ 14.478722] DEBUG: init_iommu_perf_ctr: amd_iommu_pc_present=0x1
Also, here is the perf amd_iommu testing.
# perf stat -e 'amd_iommu_0/cmd_processed/,\
amd_iommu_1/cmd_processed/,\
amd_iommu_2/cmd_processed/,\
amd_iommu_3/cmd_processed/'
Performance counter stats for 'system wide':
204 amd_iommu_0/cmd_processed/
0 amd_iommu_1/cmd_processed/
472
[git pull] IOMMU Fixes for Linux v5.5-rc7
Hi Linus, The following changes since commit def9d2780727cec3313ed3522d0123158d87224d: Linux 5.5-rc7 (2020-01-19 16:02:49 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git tags/iommu-fixes-v5.5-rc7 for you to fetch changes up to 8c17bbf6c8f70058a66305f2e1982552e6ea7f47: iommu/amd: Fix IOMMU perf counter clobbering during init (2020-01-24 15:28:40 +0100) IOMMU Fixes for Linux v5.5-rc7 Two Fixes: - Fix NULL-ptr dereference bug in Intel IOMMU driver - Properly safe and restore AMD IOMMU performance counter registers when testing if they are writable. Jerry Snitselaar (1): iommu/vt-d: Call __dmar_remove_one_dev_info with valid pointer Shuah Khan (1): iommu/amd: Fix IOMMU perf counter clobbering during init drivers/iommu/amd_iommu_init.c | 24 ++-- drivers/iommu/intel-iommu.c| 3 ++- 2 files changed, 20 insertions(+), 7 deletions(-) Please pull. Thanks, Joerg signature.asc Description: Digital signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/1] iommu/vt-d: Remove unnecessary WARN_ON_ONCE()
On Sat, Jan 18, 2020 at 10:14:11AM +0800, Lu Baolu wrote: > Hi Joerg, > > On 1/17/20 5:59 PM, Joerg Roedel wrote: > > On Thu, Jan 16, 2020 at 09:52:36AM +0800, Lu Baolu wrote: > > > Address field in device TLB invalidation descriptor is qualified > > > by the S field. If S field is zero, a single page at page address > > > specified by address [63:12] is requested to be invalidated. If S > > > field is set, the least significant bit in the address field with > > > value 0b (say bit N) indicates the invalidation address range. The > > > spec doesn't require the address [N - 1, 0] to be cleared, hence > > > remove the unnecessary WARN_ON_ONCE(). > > > > > > Otherwise, the caller might set "mask = MAX_AGAW_PFN_WIDTH" in order > > > to invalidating all the cached mappings on an endpoint, and below > > > overflow error will be triggered. > > > > > > [...] > > > UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1354:3 > > > shift exponent 64 is too large for 64-bit type 'long long unsigned int' > > > [...] > > > > > > Reported-and-tested-by: Frank > > > Signed-off-by: Lu Baolu > > > > Does this need a Fixes and/or stable tag? > > > > This doesn't cause any errors, just an unnecessary checking of > > "0 & ((1UL << 64) - 1)" > > in some cases. Okay, applied for v5.6. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: amd: Fix IOMMU perf counter clobbering during init
On Thu, Jan 23, 2020 at 03:32:14PM -0700, Shuah Khan wrote: > 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 > --- > Changes since v1: > -- Fix bug in sucessful return path. Add a return instead of >fall through to pc_false error case > > drivers/iommu/amd_iommu_init.c | 24 ++-- > 1 file changed, 18 insertions(+), 6 deletions(-) Applied for v5.5, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
