Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

2020-07-22 Thread j...@8bytes.org
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

2020-07-22 Thread Sironi, Filippo via iommu
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

2020-07-22 Thread j...@8bytes.org
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

2020-07-17 Thread Sironi, Filippo via iommu
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

2020-07-17 Thread Robin Murphy

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

2020-07-17 Thread Sironi, Filippo via iommu
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

2020-07-17 Thread Robin Murphy

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

2020-07-17 Thread Sebastian Ott via iommu

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

2020-07-10 Thread Joerg Roedel
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