Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On Wed, Jul 22, 2020 at 12:34:57PM +, Sironi, Filippo wrote: > On Wed, 2020-07-22 at 14:19 +0200, j...@8bytes.org wrote: > I wouldn't be surprised if a PCIe device raises a PCIe SERR if it is > asked to do DMA beyond its abilities. Yeah, but that would also make it impossible to safely assign the device to any untrusted entity, like a guest of user-space driver. > I think that this discussion is orthogonal wrt the spirit of the > patches. They are actually adding a few bits to the AMD IOMMU driver > (and propagating them to the right places) to implement a portion of the > specification that wasn't implemented, i.e., relying on the IVRS table. > These patches are valuable independently on the content of the IVRS > table, be it 32, 48, or 64 bits. You are right from a technical point of view, and the patches are as well. The problem I see is that there are a lot of systems out there with an AMD IOMMU and possibly broken ACPI tables. And if the driver starts checking this field now it is possible that it breaks formerly working setups. So doing this needs a strong reason, like upcoming hardware that has lower limits in the supported address space size than before. The use-case you have described is not a strong enough reason to take the risk. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On Wed, 2020-07-22 at 14:19 +0200, j...@8bytes.org wrote: > > On Fri, Jul 17, 2020 at 03:15:43PM +, Sironi, Filippo wrote: > > I don't believe that we want to trust a userspace driver here, this > > may > > result in hosts becoming unstable because devices are asked to do > > things > > they aren't meant to do (e.g., DMA beyond 48 bits). > > How is the hosts stability affected when a device is programmed with > handles outside of its DMA mask? I wouldn't be surprised if a PCIe device raises a PCIe SERR if it is asked to do DMA beyond its abilities. > Anyway, someone needs to know how to use the device in the end, and > this > entity also needs to know the DMA mask of the device and pass it down > to > path to the dma-iommu code. > > Regards, > > Joerg I agree, device drivers should do the right thing and if we have generic device drivers they may need "quirks" based on VID:DID to do the right thing. Still, I believe that the VFIO case is special because the device driver is effectively in userspace I really think that trusting userspace isn't quite correct (and we can keep discussing on this front). I think that this discussion is orthogonal wrt the spirit of the patches. They are actually adding a few bits to the AMD IOMMU driver (and propagating them to the right places) to implement a portion of the specification that wasn't implemented, i.e., relying on the IVRS table. These patches are valuable independently on the content of the IVRS table, be it 32, 48, or 64 bits. Filippo Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On Fri, Jul 17, 2020 at 03:15:43PM +, Sironi, Filippo wrote: > I don't believe that we want to trust a userspace driver here, this may > result in hosts becoming unstable because devices are asked to do things > they aren't meant to do (e.g., DMA beyond 48 bits). How is the hosts stability affected when a device is programmed with handles outside of its DMA mask? Anyway, someone needs to know how to use the device in the end, and this entity also needs to know the DMA mask of the device and pass it down to path to the dma-iommu code. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On Fri, 2020-07-17 at 15:36 +0100, Robin Murphy wrote: > On 2020-07-17 14:22, Sironi, Filippo wrote: > > On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote: > > > > > > On 2020-07-17 10:20, Sebastian Ott via iommu wrote: > > > > Hello Joerg, > > > > > > > > On 2020-07-10 14:31, Joerg Roedel wrote: > > > > > On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: > > > > > > The IVRS ACPI table specifies maximum address sizes for I/O > > > > > > virtual > > > > > > addresses that can be handled by the IOMMUs in the system. > > > > > > Parse > > > > > > that > > > > > > data from the IVRS header to provide aperture information > > > > > > for > > > > > > DMA > > > > > > mappings and users of the iommu API. > > > > > > > > > > > > Changes for V2: > > > > > >- use limits in iommu_setup_dma_ops() > > > > > >- rebased to current upstream > > > > > > > > > > > > Sebastian Ott (3): > > > > > > iommu/amd: Parse supported address sizes from IVRS > > > > > > iommu/amd: Restrict aperture for domains to conform with > > > > > > IVRS > > > > > > iommu/amd: Actually enforce geometry aperture > > > > > > > > > > Thanks for the changes. May I ask what the reason for those > > > > > changes are? > > > > > AFAIK all AMD IOMMU implementations (in hardware) support full > > > > > 64bit > > > > > address spaces, and the IVRS table might actually be wrong, > > > > > limiting the > > > > > address space in the worst case to only 32 bit. > > > > > > > > It's not the IOMMU, but we've encountered devices that are > > > > capable > > > > of > > > > more than > > > > 32- but less than 64- bit IOVA, and there's no way to express > > > > that > > > > to > > > > the IOVA > > > > allocator in the PCIe spec. Our solution was to have our > > > > platforms > > > > express an > > > > IVRS entry that says the IOMMU is capable of 48-bit, which these > > > > devices > > > > can generate. > > > > 48 bits is plenty of address space in this generation for the > > > > application we have. > > > > > > Hmm, for constraints of individual devices, it should really be > > > their > > > drivers' job to call dma_set_mask*() with the appropriate value in > > > the > > > first place rather than just assuming that 64 means anything >32. > > > If > > > it's a case where the device itself technically is 64-bit capable, > > > but > > > an upstream bridge is constraining it, then that limit can also be > > > described either by dedicated firmware properties (e.g. ACPI _DMA) > > > or > > > with a quirk like via_no_dac(). > > > > > > Robin. > > > > You cannot rely on the device driver only because the device driver > > attach might be a generic one like vfio-pci, for instance, that > > doesn't > > have any device specific knowledge. > > Indeed, but on the other hand a generic driver that doesn't know the > device is highly unlikely to set up any DMA transactions by itself > either. In the case of VFIO, it would then be the guest/userspace > driver's responsibility to take the equivalent action to avoid > allocating addresses the hardware can't actually use. I don't believe that we want to trust a userspace driver here, this may result in hosts becoming unstable because devices are asked to do things they aren't meant to do (e.g., DMA beyond 48 bits). > I'm mostly just wary that trying to fake up a per-device restriction > as > a global one is a bit crude, and has the inherent problem that > whatever > you think the lowest common denominator is, there's the potential for > some device to be hotplugged in later and break the assumption you've > already had to commit to. I agree, if the BIOS sets up an IVRS table with aperture of 48 bits and all of a sudden we hotplug a device that only support 36 bits we're in a bad place. > And of course I am taking a bit of a DMA-API-centric viewpoint here - > I > think exposing per-device properties like bus_dma_limit that aren't > easily identifiable for VFIO users to take into account is still > rather > an open problem. > > Robin. The use of ACPI _DMA that you suggest looks interesting and would allow the kernel to prevent a dumb userspace driver using VFIO to make damage, I think. It doesn't look like there's much support for ACPI _DMA though. Are you aware of existing efforts on this front? Filippo Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On 2020-07-17 14:22, Sironi, Filippo wrote: On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote: On 2020-07-17 10:20, Sebastian Ott via iommu wrote: Hello Joerg, On 2020-07-10 14:31, Joerg Roedel wrote: On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: The IVRS ACPI table specifies maximum address sizes for I/O virtual addresses that can be handled by the IOMMUs in the system. Parse that data from the IVRS header to provide aperture information for DMA mappings and users of the iommu API. Changes for V2: - use limits in iommu_setup_dma_ops() - rebased to current upstream Sebastian Ott (3): iommu/amd: Parse supported address sizes from IVRS iommu/amd: Restrict aperture for domains to conform with IVRS iommu/amd: Actually enforce geometry aperture Thanks for the changes. May I ask what the reason for those changes are? AFAIK all AMD IOMMU implementations (in hardware) support full 64bit address spaces, and the IVRS table might actually be wrong, limiting the address space in the worst case to only 32 bit. It's not the IOMMU, but we've encountered devices that are capable of more than 32- but less than 64- bit IOVA, and there's no way to express that to the IOVA allocator in the PCIe spec. Our solution was to have our platforms express an IVRS entry that says the IOMMU is capable of 48-bit, which these devices can generate. 48 bits is plenty of address space in this generation for the application we have. Hmm, for constraints of individual devices, it should really be their drivers' job to call dma_set_mask*() with the appropriate value in the first place rather than just assuming that 64 means anything >32. If it's a case where the device itself technically is 64-bit capable, but an upstream bridge is constraining it, then that limit can also be described either by dedicated firmware properties (e.g. ACPI _DMA) or with a quirk like via_no_dac(). Robin. You cannot rely on the device driver only because the device driver attach might be a generic one like vfio-pci, for instance, that doesn't have any device specific knowledge. Indeed, but on the other hand a generic driver that doesn't know the device is highly unlikely to set up any DMA transactions by itself either. In the case of VFIO, it would then be the guest/userspace driver's responsibility to take the equivalent action to avoid allocating addresses the hardware can't actually use. I'm mostly just wary that trying to fake up a per-device restriction as a global one is a bit crude, and has the inherent problem that whatever you think the lowest common denominator is, there's the potential for some device to be hotplugged in later and break the assumption you've already had to commit to. And of course I am taking a bit of a DMA-API-centric viewpoint here - I think exposing per-device properties like bus_dma_limit that aren't easily identifiable for VFIO users to take into account is still rather an open problem. Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote: > > On 2020-07-17 10:20, Sebastian Ott via iommu wrote: > > Hello Joerg, > > > > On 2020-07-10 14:31, Joerg Roedel wrote: > > > On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: > > > > The IVRS ACPI table specifies maximum address sizes for I/O > > > > virtual > > > > addresses that can be handled by the IOMMUs in the system. Parse > > > > that > > > > data from the IVRS header to provide aperture information for > > > > DMA > > > > mappings and users of the iommu API. > > > > > > > > Changes for V2: > > > > - use limits in iommu_setup_dma_ops() > > > > - rebased to current upstream > > > > > > > > Sebastian Ott (3): > > > >iommu/amd: Parse supported address sizes from IVRS > > > >iommu/amd: Restrict aperture for domains to conform with IVRS > > > >iommu/amd: Actually enforce geometry aperture > > > > > > Thanks for the changes. May I ask what the reason for those > > > changes are? > > > AFAIK all AMD IOMMU implementations (in hardware) support full > > > 64bit > > > address spaces, and the IVRS table might actually be wrong, > > > limiting the > > > address space in the worst case to only 32 bit. > > > > It's not the IOMMU, but we've encountered devices that are capable > > of > > more than > > 32- but less than 64- bit IOVA, and there's no way to express that > > to > > the IOVA > > allocator in the PCIe spec. Our solution was to have our platforms > > express an > > IVRS entry that says the IOMMU is capable of 48-bit, which these > > devices > > can generate. > > 48 bits is plenty of address space in this generation for the > > application we have. > > Hmm, for constraints of individual devices, it should really be their > drivers' job to call dma_set_mask*() with the appropriate value in the > first place rather than just assuming that 64 means anything >32. If > it's a case where the device itself technically is 64-bit capable, but > an upstream bridge is constraining it, then that limit can also be > described either by dedicated firmware properties (e.g. ACPI _DMA) or > with a quirk like via_no_dac(). > > Robin. You cannot rely on the device driver only because the device driver attach might be a generic one like vfio-pci, for instance, that doesn't have any device specific knowledge. Filippo Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
On 2020-07-17 10:20, Sebastian Ott via iommu wrote: Hello Joerg, On 2020-07-10 14:31, Joerg Roedel wrote: On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: The IVRS ACPI table specifies maximum address sizes for I/O virtual addresses that can be handled by the IOMMUs in the system. Parse that data from the IVRS header to provide aperture information for DMA mappings and users of the iommu API. Changes for V2: - use limits in iommu_setup_dma_ops() - rebased to current upstream Sebastian Ott (3): iommu/amd: Parse supported address sizes from IVRS iommu/amd: Restrict aperture for domains to conform with IVRS iommu/amd: Actually enforce geometry aperture Thanks for the changes. May I ask what the reason for those changes are? AFAIK all AMD IOMMU implementations (in hardware) support full 64bit address spaces, and the IVRS table might actually be wrong, limiting the address space in the worst case to only 32 bit. It's not the IOMMU, but we've encountered devices that are capable of more than 32- but less than 64- bit IOVA, and there's no way to express that to the IOVA allocator in the PCIe spec. Our solution was to have our platforms express an IVRS entry that says the IOMMU is capable of 48-bit, which these devices can generate. 48 bits is plenty of address space in this generation for the application we have. Hmm, for constraints of individual devices, it should really be their drivers' job to call dma_set_mask*() with the appropriate value in the first place rather than just assuming that 64 means anything >32. If it's a case where the device itself technically is 64-bit capable, but an upstream bridge is constraining it, then that limit can also be described either by dedicated firmware properties (e.g. ACPI _DMA) or with a quirk like via_no_dac(). Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
Hello Joerg, On 2020-07-10 14:31, Joerg Roedel wrote: On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: The IVRS ACPI table specifies maximum address sizes for I/O virtual addresses that can be handled by the IOMMUs in the system. Parse that data from the IVRS header to provide aperture information for DMA mappings and users of the iommu API. Changes for V2: - use limits in iommu_setup_dma_ops() - rebased to current upstream Sebastian Ott (3): iommu/amd: Parse supported address sizes from IVRS iommu/amd: Restrict aperture for domains to conform with IVRS iommu/amd: Actually enforce geometry aperture Thanks for the changes. May I ask what the reason for those changes are? AFAIK all AMD IOMMU implementations (in hardware) support full 64bit address spaces, and the IVRS table might actually be wrong, limiting the address space in the worst case to only 32 bit. It's not the IOMMU, but we've encountered devices that are capable of more than 32- but less than 64- bit IOVA, and there's no way to express that to the IOVA allocator in the PCIe spec. Our solution was to have our platforms express an IVRS entry that says the IOMMU is capable of 48-bit, which these devices can generate. 48 bits is plenty of address space in this generation for the application we have. Sebastian Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits
Hi Sebastian, On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote: > The IVRS ACPI table specifies maximum address sizes for I/O virtual > addresses that can be handled by the IOMMUs in the system. Parse that > data from the IVRS header to provide aperture information for DMA > mappings and users of the iommu API. > > Changes for V2: > - use limits in iommu_setup_dma_ops() > - rebased to current upstream > > Sebastian Ott (3): > iommu/amd: Parse supported address sizes from IVRS > iommu/amd: Restrict aperture for domains to conform with IVRS > iommu/amd: Actually enforce geometry aperture Thanks for the changes. May I ask what the reason for those changes are? AFAIK all AMD IOMMU implementations (in hardware) support full 64bit address spaces, and the IVRS table might actually be wrong, limiting the address space in the worst case to only 32 bit. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/3] iommu/amd: I/O VA address limits
The IVRS ACPI table specifies maximum address sizes for I/O virtual addresses that can be handled by the IOMMUs in the system. Parse that data from the IVRS header to provide aperture information for DMA mappings and users of the iommu API. Changes for V2: - use limits in iommu_setup_dma_ops() - rebased to current upstream Sebastian Ott (3): iommu/amd: Parse supported address sizes from IVRS iommu/amd: Restrict aperture for domains to conform with IVRS iommu/amd: Actually enforce geometry aperture drivers/iommu/amd/amd_iommu_types.h | 3 +++ drivers/iommu/amd/init.c| 26 ++ drivers/iommu/amd/iommu.c | 12 ++-- 3 files changed, 39 insertions(+), 2 deletions(-) -- 2.17.1 Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
