Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 20:15 +0100, David Woodhouse wrote: > On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote: > > I can only speak to the HP servers. We have been shipping devices > > 'for a while' that provide sensor-type data to the platform. The > > device does DMA writes to a range of memory (the RMRR) and > > iLO does DMA reads of that data. > > > > This works in general but not when the 'iommu=pt' boot option is > > used. This patch associates the RMRR with the devices when > > they are moved out of the "si" domain. > > That much makes sense, I think, because they're moved out of the > hardware SI domain *early*, when we realise they're actually only > capable of 32-bit DMA and we have >4GiB of RAM. Right? Correct. By default all devices are added to SI domain assuming that these devices are 64-bit devices. When we detect the device is a 32-bit device based on the requested dma-mask, it gets removed from SI domain, hence looses its RMRR association. In the meantime dma continues causing DMA errors. This patch is re-processing RMRRs for the device in question and doing re-assignment. > > It sounds like this isn't a case of the device being used by a native > driver or given to KVM, and subsequently released. This is just booting > up and losing the RMRR regions on a device which the OS *hasn't* really > touched. So that should be fixed. > > > Based on Alex's comments about moving RMRR devices between domains, > > it sounds like we also have a problem without the 'iommu=pt' boot > > option if someone assigns one of those devices to a guest. > > Yeah... but why *would* they? What possible reason would we have to > assign either the sensor device, or the iLO, to a KVM guest. Or to have > a native driver that attempts to do DMA from them? > > Obviously, in an ideal world we'd have proper native drivers for the > sensor device. But I'm guessing that's not the case here; it's used by > the firmware and we're not supposed to be touching it? > > And yes, obviously a better hardware design (from the OS/IOMMU point of > view) would be to have a path for the sensor data that *doesn't* go via > host RAM and thus via the IOMMU twice. But while that's a lesson that's > hopefully been learned and will be implemented in future, we have to > deal with the existing hardware and its (ab)use of RMRRs. > Right. We do have hardware that is relying on being able to do dma from devices to a system RAM via RMRR. -- Shuah ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
That is correct, David. It is moved out early on when it is found to be a 32bit DMA device and its RMRR info is lost. The movement of devices between the si domain and *others* only happens in iommu=pt mode. This si domain doesn't come into play when simply booting with intel_iommu=on. Thanks, Tom -Original Message- From: David Woodhouse [mailto:dw...@infradead.org] Sent: Friday, September 28, 2012 2:15 PM To: Knippers, Linda Cc: Alex Williamson; Joerg Roedel; iommu@lists.linux-foundation.org; Khan, Shuah; Mingarelli, Thomas Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote: > I can only speak to the HP servers. We have been shipping devices > 'for a while' that provide sensor-type data to the platform. The > device does DMA writes to a range of memory (the RMRR) and > iLO does DMA reads of that data. > > This works in general but not when the 'iommu=pt' boot option is > used. This patch associates the RMRR with the devices when > they are moved out of the "si" domain. That much makes sense, I think, because they're moved out of the hardware SI domain *early*, when we realise they're actually only capable of 32-bit DMA and we have >4GiB of RAM. Right? It sounds like this isn't a case of the device being used by a native driver or given to KVM, and subsequently released. This is just booting up and losing the RMRR regions on a device which the OS *hasn't* really touched. So that should be fixed. > Based on Alex's comments about moving RMRR devices between domains, > it sounds like we also have a problem without the 'iommu=pt' boot > option if someone assigns one of those devices to a guest. Yeah... but why *would* they? What possible reason would we have to assign either the sensor device, or the iLO, to a KVM guest. Or to have a native driver that attempts to do DMA from them? Obviously, in an ideal world we'd have proper native drivers for the sensor device. But I'm guessing that's not the case here; it's used by the firmware and we're not supposed to be touching it? And yes, obviously a better hardware design (from the OS/IOMMU point of view) would be to have a path for the sensor data that *doesn't* go via host RAM and thus via the IOMMU twice. But while that's a lesson that's hopefully been learned and will be implemented in future, we have to deal with the existing hardware and its (ab)use of RMRRs. -- dwmw2 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 12:36 -0400, Linda Knippers wrote: > I can only speak to the HP servers. We have been shipping devices > 'for a while' that provide sensor-type data to the platform. The > device does DMA writes to a range of memory (the RMRR) and > iLO does DMA reads of that data. > > This works in general but not when the 'iommu=pt' boot option is > used. This patch associates the RMRR with the devices when > they are moved out of the "si" domain. That much makes sense, I think, because they're moved out of the hardware SI domain *early*, when we realise they're actually only capable of 32-bit DMA and we have >4GiB of RAM. Right? It sounds like this isn't a case of the device being used by a native driver or given to KVM, and subsequently released. This is just booting up and losing the RMRR regions on a device which the OS *hasn't* really touched. So that should be fixed. > Based on Alex's comments about moving RMRR devices between domains, > it sounds like we also have a problem without the 'iommu=pt' boot > option if someone assigns one of those devices to a guest. Yeah... but why *would* they? What possible reason would we have to assign either the sensor device, or the iLO, to a KVM guest. Or to have a native driver that attempts to do DMA from them? Obviously, in an ideal world we'd have proper native drivers for the sensor device. But I'm guessing that's not the case here; it's used by the firmware and we're not supposed to be touching it? And yes, obviously a better hardware design (from the OS/IOMMU point of view) would be to have a path for the sensor data that *doesn't* go via host RAM and thus via the IOMMU twice. But while that's a lesson that's hopefully been learned and will be implemented in future, we have to deal with the existing hardware and its (ab)use of RMRRs. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 19:01 +0200, Joerg Roedel wrote: > On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote: > > I can only speak to the HP servers. We have been shipping devices > > 'for a while' that provide sensor-type data to the platform. The > > device does DMA writes to a range of memory (the RMRR) and > > iLO does DMA reads of that data. > > And what PCI request-ids are used for these DMA transfers? Are this > request-ids which belong to devices Linux handles on its own? > > > If we address Alex's comments and we make a change to disallow the > > devices (non-USB devices?) with RMRRs from being assigned to > > a guest, will those changes be considered? > > This is overkill in my eyes. It means that *any* device which has an > RMRR defined, whether it is on your platform or not, can not be assigned > to a guest. > > I think it is better to have the RMRR regions mapped in the domains used > for DMA-API mappings and disallow to assign these devices to guests. For > devices where this breaks we can implement some quirk-solution and > disallow guest assignment. But disallowing assignment of devices with > RMRR defined in general is pure overkill. On the other hand, a blanket blacklist of no assignment of RMRR devices deters creative uses of RMRRs, which seems like a good thing. USB probably needs to be an exception due to widespread usage of RMRRs that are known to not be needed. Anything else risks the assigned device doing autonomous writes to memory, potentially causing corruption. The only safe way to prevent that would be to insert a reserved memory range into the guest to match the RMRR, maybe even identity map it if we care about the RMRR continuing to work, but that makes hotplug of those devices nearly impossible. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, Sep 28, 2012 at 07:01:06PM +0200, Joerg Roedel wrote: > On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote: > > I can only speak to the HP servers. We have been shipping devices > > 'for a while' that provide sensor-type data to the platform. The > > device does DMA writes to a range of memory (the RMRR) and > > iLO does DMA reads of that data. > > And what PCI request-ids are used for these DMA transfers? Are this > request-ids which belong to devices Linux handles on its own? > > > If we address Alex's comments and we make a change to disallow the > > devices (non-USB devices?) with RMRRs from being assigned to > > a guest, will those changes be considered? > > This is overkill in my eyes. It means that *any* device which has an > RMRR defined, whether it is on your platform or not, can not be assigned > to a guest. > > I think it is better to have the RMRR regions mapped in the domains used > for DMA-API mappings and disallow to assign these devices to guests. Of course I meant "allow to assign these devices to guests." > For devices where this breaks we can implement some quirk-solution and > disallow guest assignment. But disallowing assignment of devices with > RMRR defined in general is pure overkill. > > > Joerg > > > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, Sep 28, 2012 at 12:36:05PM -0400, Linda Knippers wrote: > I can only speak to the HP servers. We have been shipping devices > 'for a while' that provide sensor-type data to the platform. The > device does DMA writes to a range of memory (the RMRR) and > iLO does DMA reads of that data. And what PCI request-ids are used for these DMA transfers? Are this request-ids which belong to devices Linux handles on its own? > If we address Alex's comments and we make a change to disallow the > devices (non-USB devices?) with RMRRs from being assigned to > a guest, will those changes be considered? This is overkill in my eyes. It means that *any* device which has an RMRR defined, whether it is on your platform or not, can not be assigned to a guest. I think it is better to have the RMRR regions mapped in the domains used for DMA-API mappings and disallow to assign these devices to guests. For devices where this breaks we can implement some quirk-solution and disallow guest assignment. But disallowing assignment of devices with RMRR defined in general is pure overkill. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
David Woodhouse wrote: > > HP may have been shipping such things 'for a while' but it's never > actually worked right, yes? This thread is about the patch that's > intended to *fix* that? I can only speak to the HP servers. We have been shipping devices 'for a while' that provide sensor-type data to the platform. The device does DMA writes to a range of memory (the RMRR) and iLO does DMA reads of that data. This works in general but not when the 'iommu=pt' boot option is used. This patch associates the RMRR with the devices when they are moved out of the "si" domain. Based on Alex's comments about moving RMRR devices between domains, it sounds like we also have a problem without the 'iommu=pt' boot option if someone assigns one of those devices to a guest. Tom, Shuah, Alex - please correct me if I've gotten any of this wrong. > If they could just manage to make their firmware-owned DMA appear to be > from a different PCIe device/function from the one the OS gets to own, > that would make things "just work", right? Hell, their laptops already > have Ricoh multi-function devices that do their DMA from the 'wrong' > function take that concept and make it useful... > > If the DMA could be hidden from the IOMMU altogether, all the better. > But at least coming from its own dedicated devfn would avoid the issues > this patch attempts to solve. That does sound like a good idea and may be something that we can suggest for the future but that won't help us today. If we address Alex's comments and we make a change to disallow the devices (non-USB devices?) with RMRRs from being assigned to a guest, will those changes be considered? -- ljk > > > > -- > dwmw2 > > (Apologies for HTML and top-posting; Android mailer is broken.) > > Alex Williamson wrote: > On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote: >> On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote: >> > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote: >> >> > > I don't think so. The concept of RMRR is just not defined well enough >> > > (like the concept of unity mappings on the AMD side which is > similar to >> > > RMRR). The definition says, that any memory region must be mapped at >> > > any time for the device. But that is not true (at least I have no >> > > counter-example yet). The right definition would be, that the RMRR >> > > regions are only necessary as long as the operating system does not >> > > control the particular device. And assigning a device to a guest also >> > > counts a 'taking control over the device'. >> > >> > I think HP folks would be very unhappy with that definition. As David >> > indicates, that's how things like USB use RMRR, but the actual >> > definition in the spec leaves much more room for abuse. Thanks, >> >> To my experience, for a hardware designer, existing software overrides >> any Spec because it is much worse to break existing software than it is >> to break a Spec :) So, unless we break existing hardware/firmware, I >> still suggest that we use the assumption that OS controlled devices do >> not need RMRR/unity-mapped regions anymore. > > HP has been shipping hardware that makes use of RMRRs for other purposes > for a while. > >> Is HP doing anything in their firmware which would not work with that? > > Yes, I'll let them fill in the details. > >> For the USB controlers, they only generate DMA to the RMRR/unity-mapped >> region until the OS takes over control from the firmware. After >> the USB driver is initialized the RMRR region should not be necessary >> anymore. > > I agree that was probably the intent, but vendors have found loopholes > as their opportunity to innovate. Thanks, > > Alex > > > > > > ___ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 16:52 +0100, David Woodhouse wrote: > HP may have been shipping such things 'for a while' but it's never > actually worked right, yes? This thread is about the patch that's > intended to *fix* that? I believe it currently works in non-passthrough mode since the RMRRs stick for that. It hasn't worked in passthrough mode since we disabled swiotlb and started kicking 32bit dma mask devices out of the si domain. It has probably never worked for VM usage if anyone has made the mistake of attempting that. > If they could just manage to make their firmware-owned DMA appear to > be from a different PCIe device/function from the one the OS gets to > own, that would make things "just work", right? Hell, their laptops > already have Ricoh multi-function devices that do their DMA from the > 'wrong' function take that concept and make it useful... > > If the DMA could be hidden from the IOMMU altogether, all the better. > But at least coming from its own dedicated devfn would avoid the > issues this patch attempts to solve. Hmm, an RMRR for a non-existent device would cause problems when we try to figure out the dma mask... if nothing else. The proposal seems to open pandora's box just a little further :-\ Maybe it would have to live outside of device ranges claimed by the DRHDs, but that implies physical topology changes. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
HP may have been shipping such things 'for a while' but it's never actually worked right, yes? This thread is about the patch that's intended to *fix* that? If they could just manage to make their firmware-owned DMA appear to be from a different PCIe device/function from the one the OS gets to own, that would make things "just work", right? Hell, their laptops already have Ricoh multi-function devices that do their DMA from the 'wrong' function take that concept and make it useful... If the DMA could be hidden from the IOMMU altogether, all the better. But at least coming from its own dedicated devfn would avoid the issues this patch attempts to solve. -- dwmw2 (Apologies for HTML and top-posting; Android mailer is broken.)Alex Williamson wrote:On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote: > On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote: > > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote: > > > > I don't think so. The concept of RMRR is just not defined well enough > > > (like the concept of unity mappings on the AMD side which is similar to > > > RMRR). The definition says, that any memory region must be mapped at > > > any time for the device. But that is not true (at least I have no > > > counter-example yet). The right definition would be, that the RMRR > > > regions are only necessary as long as the operating system does not > > > control the particular device. And assigning a device to a guest also > > > counts a 'taking control over the device'. > > > > I think HP folks would be very unhappy with that definition. As David > > indicates, that's how things like USB use RMRR, but the actual > > definition in the spec leaves much more room for abuse. Thanks, > > To my experience, for a hardware designer, existing software overrides > any Spec because it is much worse to break existing software than it is > to break a Spec :) So, unless we break existing hardware/firmware, I > still suggest that we use the assumption that OS controlled devices do > not need RMRR/unity-mapped regions anymore. HP has been shipping hardware that makes use of RMRRs for other purposes for a while. > Is HP doing anything in their firmware which would not work with that? Yes, I'll let them fill in the details. > For the USB controlers, they only generate DMA to the RMRR/unity-mapped > region until the OS takes over control from the firmware. After > the USB driver is initialized the RMRR region should not be necessary > anymore. I agree that was probably the intent, but vendors have found loopholes as their opportunity to innovate. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 14:52 +0200, Joerg Roedel wrote: > On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote: > > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote: > > > > I don't think so. The concept of RMRR is just not defined well enough > > > (like the concept of unity mappings on the AMD side which is similar to > > > RMRR). The definition says, that any memory region must be mapped at > > > any time for the device. But that is not true (at least I have no > > > counter-example yet). The right definition would be, that the RMRR > > > regions are only necessary as long as the operating system does not > > > control the particular device. And assigning a device to a guest also > > > counts a 'taking control over the device'. > > > > I think HP folks would be very unhappy with that definition. As David > > indicates, that's how things like USB use RMRR, but the actual > > definition in the spec leaves much more room for abuse. Thanks, > > To my experience, for a hardware designer, existing software overrides > any Spec because it is much worse to break existing software than it is > to break a Spec :) So, unless we break existing hardware/firmware, I > still suggest that we use the assumption that OS controlled devices do > not need RMRR/unity-mapped regions anymore. HP has been shipping hardware that makes use of RMRRs for other purposes for a while. > Is HP doing anything in their firmware which would not work with that? Yes, I'll let them fill in the details. > For the USB controlers, they only generate DMA to the RMRR/unity-mapped > region until the OS takes over control from the firmware. After > the USB driver is initialized the RMRR region should not be necessary > anymore. I agree that was probably the intent, but vendors have found loopholes as their opportunity to innovate. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, Sep 28, 2012 at 06:40:08AM -0600, Alex Williamson wrote: > On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote: > > I don't think so. The concept of RMRR is just not defined well enough > > (like the concept of unity mappings on the AMD side which is similar to > > RMRR). The definition says, that any memory region must be mapped at > > any time for the device. But that is not true (at least I have no > > counter-example yet). The right definition would be, that the RMRR > > regions are only necessary as long as the operating system does not > > control the particular device. And assigning a device to a guest also > > counts a 'taking control over the device'. > > I think HP folks would be very unhappy with that definition. As David > indicates, that's how things like USB use RMRR, but the actual > definition in the spec leaves much more room for abuse. Thanks, To my experience, for a hardware designer, existing software overrides any Spec because it is much worse to break existing software than it is to break a Spec :) So, unless we break existing hardware/firmware, I still suggest that we use the assumption that OS controlled devices do not need RMRR/unity-mapped regions anymore. Is HP doing anything in their firmware which would not work with that? For the USB controlers, they only generate DMA to the RMRR/unity-mapped region until the OS takes over control from the firmware. After the USB driver is initialized the RMRR region should not be necessary anymore. Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 11:43 +0200, Joerg Roedel wrote: > On Thu, Sep 27, 2012 at 03:34:07PM -0600, Alex Williamson wrote: > > > It really seems like RMRRs are incompatible with IOMMU API use > > though. > > I don't think so. The concept of RMRR is just not defined well enough > (like the concept of unity mappings on the AMD side which is similar to > RMRR). The definition says, that any memory region must be mapped at > any time for the device. But that is not true (at least I have no > counter-example yet). The right definition would be, that the RMRR > regions are only necessary as long as the operating system does not > control the particular device. And assigning a device to a guest also > counts a 'taking control over the device'. I think HP folks would be very unhappy with that definition. As David indicates, that's how things like USB use RMRR, but the actual definition in the spec leaves much more room for abuse. Thanks, Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, Sep 28, 2012 at 11:23:23AM +0100, David Woodhouse wrote: > On Fri, 2012-09-28 at 11:46 +0200, Joerg Roedel wrote: > > Even on modern hardware with modern (IOMMU aware) kernels there is still > > this small time window when the OS has enabled the IOMMU and the USB > > driver is not initialized yet. In this time window the RMRR memory > > region is still necessary, no? > > Yes but there's no *reason* for that. It wouldn't be that hard to ask > the firmware to quiesce all its own DMA *before* we enable the IOMMU. True. That would have been a better approach. Some kind of IOMMU handover from firmware to the OS. > > As I said already in another mail, I think it is safe to ignore any RMRR > > requirements when we start to use a device in the OS. > > I think the whole point in this patch is that there is some brain-dead > hardware out there (vendor 'value subtract' I think) on which that > common-sense observation isn't actually true. > > I'm all for handling that broken hardware with quirks, giving clear > messages to the user that the device(+firmware) in question is broken, > and refusing to let either the kernel *or* VM guests do any DMA with it. Well, it is probably mostly about southbridge devices and add-on USB controllers. Or is there any other way a given device can communicate RMRR requirements to the firmware? Anyway, I am fine with completly blocking DMA for those devices too. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Fri, 2012-09-28 at 11:46 +0200, Joerg Roedel wrote: > Even on modern hardware with modern (IOMMU aware) kernels there is still > this small time window when the OS has enabled the IOMMU and the USB > driver is not initialized yet. In this time window the RMRR memory > region is still necessary, no? Yes but there's no *reason* for that. It wouldn't be that hard to ask the firmware to quiesce all its own DMA *before* we enable the IOMMU. > As I said already in another mail, I think it is safe to ignore any RMRR > requirements when we start to use a device in the OS. I think the whole point in this patch is that there is some brain-dead hardware out there (vendor 'value subtract' I think) on which that common-sense observation isn't actually true. I'm all for handling that broken hardware with quirks, giving clear messages to the user that the device(+firmware) in question is broken, and refusing to let either the kernel *or* VM guests do any DMA with it. -- dwmw2 smime.p7s Description: S/MIME cryptographic signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Thu, Sep 27, 2012 at 10:50:05PM +0100, David Woodhouse wrote: > That would include fairly much any USB host controller. The whole RMRR > concept is completely broken and should never have been invented. The > idea that firmware-controlled DMA should continue to happen *after* the > operating system has been booted and taken control of the hardware is > just insane. > > The majority of RMRR use is for USB controllers, so that firmware can > emulate a legacy keyboard for the benefit of pre-USB operating systems. > But no operating system that old is ever going to support the IOMMU > anyway, so that's just mad. But that's why we've managed to get away > with setting up the RMRRs and then tearing them down when a native > driver has actually taken control of the hardware — because at that > point, it should have been reset and whatever the firmware had > configured to do has been abandoned. Even on modern hardware with modern (IOMMU aware) kernels there is still this small time window when the OS has enabled the IOMMU and the USB driver is not initialized yet. In this time window the RMRR memory region is still necessary, no? As I said already in another mail, I think it is safe to ignore any RMRR requirements when we start to use a device in the OS. Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Thu, Sep 27, 2012 at 03:34:07PM -0600, Alex Williamson wrote: > It really seems like RMRRs are incompatible with IOMMU API use > though. I don't think so. The concept of RMRR is just not defined well enough (like the concept of unity mappings on the AMD side which is similar to RMRR). The definition says, that any memory region must be mapped at any time for the device. But that is not true (at least I have no counter-example yet). The right definition would be, that the RMRR regions are only necessary as long as the operating system does not control the particular device. And assigning a device to a guest also counts a 'taking control over the device'. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
Alex Williamson wrote:
> On Thu, 2012-09-27 at 21:10 +, Mingarelli, Thomas wrote:
>> Alex,
>>
>> Are you suggesting that a solution is to prevent devices with RMRRs
>> from being placed in the SI Domain in the first place (when pt mode is
>> used)?
>
> No, it seems like it's preferable that devices with RMRRs stay in the si
> domain if the device supports 64bit DMA. They're likely to cause less
> problems there and we can ignore the RMRRs in the si domain. The
> problem I'm trying to address is that the domain you're setting up with
> RMRRs is only used for dma_ops (ie. host driver use). If the device is
> attached to a guest, that domain gets discarded and the device is added
> to yet another domain, potentially with other devices. That domain is
> missing RMRRs, so now your device is going to generate VT-d faults. The
> device can then get removed from that domain, in which case the RMRRs
> should be removed, and again a new dma_ops domain needs to get created
> with RMRRs.
>
> It really seems like RMRRs are incompatible with IOMMU API use though.
> If an RMRR is setup for a VM domain, that's bad because a) it gives the
> VM direct access to that range of host memory, and b) it interferes with
> the guest use of the address space. a) is also bad for isolating
> devices on the host, but the spec makes it available for abuse. For b),
> it's not hard to imagine an RMRR range on the host that overlaps with
> DMA'able space on the guest. Data is read or written to the host memory
> instead of the guest memory. So maybe the right answer is to make
> intel_iommu_attach_device return error if requested to act on a device
> with RMRR ranges.
That sounds like the right answer to me. Tom, can you make that
change when you address the rest of the comments? Or should it
be a separate patch?
-- ljk
> Thanks,
>
> Alex
>
>> -Original Message-
>> From: Alex Williamson [mailto:alex.william...@redhat.com]
>> Sent: Thursday, September 27, 2012 3:37 PM
>> To: Mingarelli, Thomas
>> Cc: iommu@lists.linux-foundation.org; Knippers, Linda; Khan, Shuah; Don
>> Dutile; David Woodhouse
>> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
>>
>> [adding David Woodhouse]
>>
>> On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
>>> When a 32bit PCI device is removed from the SI Domain, the RMRR information
>>> for this device becomes invalid and needs to be reprocessed to avoid DMA
>>> Read errors. These errors are evidenced by the Present bit being cleared in
>>> the device's context entry. Fixing this problem with an enhancement to
>>> process
>>> the RMRR info when the device is assigned to another domain. The Bus Master
>>> bit
>>> is cleared during the move to another domain and during the reprocessing of
>>> the RMRR info so no DMA can take place at this time.
>>>
>>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>>>
>>> drivers/iommu/intel-iommu.c | 47
>>> --
>>> 1 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> Signed-off-by: Thomas Mingarelli
>>>
>>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
>>> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889
>>> -0500
>>> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
>>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>>> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>> }
>>>
>>> +static int reprocess_rmrr(struct device *dev)
>>> +{
>>> + struct dmar_rmrr_unit *rmrr;
>>> + struct pci_dev *pdev;
>>> + int i, ret;
>>> +
>>> + pdev = to_pci_dev(dev);
>>> +
>>> + for_each_rmrr_units(rmrr) {
>>> + for (i = 0; i < rmrr->devices_cnt; i++) {
>>> + /*
>>> +* Here we are just concerned with
>>> +* finding the one device that was
>>> +* removed from the si_domain and
>>> +* re-evaluating its RMRR info.
>>> +*/
>> I'm still not sure why this comment is wrapped so tightly.
>>
>>> + if (rmrr->devices[i] != pdev)
>>> + continue;
>>> + pr_info("IOMMU: Reprocess RMRR information for device
>>> %s.\n",
>
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Thu, 2012-09-27 at 17:50 -0400, Linda Knippers wrote:
> Alex Williamson wrote:
> > On Thu, 2012-09-27 at 21:10 +, Mingarelli, Thomas wrote:
> >> Alex,
> >>
> >> Are you suggesting that a solution is to prevent devices with RMRRs
> >> from being placed in the SI Domain in the first place (when pt mode is
> >> used)?
> >
> > No, it seems like it's preferable that devices with RMRRs stay in the si
> > domain if the device supports 64bit DMA. They're likely to cause less
> > problems there and we can ignore the RMRRs in the si domain. The
> > problem I'm trying to address is that the domain you're setting up with
> > RMRRs is only used for dma_ops (ie. host driver use). If the device is
> > attached to a guest, that domain gets discarded and the device is added
> > to yet another domain, potentially with other devices. That domain is
> > missing RMRRs, so now your device is going to generate VT-d faults. The
> > device can then get removed from that domain, in which case the RMRRs
> > should be removed, and again a new dma_ops domain needs to get created
> > with RMRRs.
> >
> > It really seems like RMRRs are incompatible with IOMMU API use though.
> > If an RMRR is setup for a VM domain, that's bad because a) it gives the
> > VM direct access to that range of host memory, and b) it interferes with
> > the guest use of the address space. a) is also bad for isolating
> > devices on the host, but the spec makes it available for abuse. For b),
> > it's not hard to imagine an RMRR range on the host that overlaps with
> > DMA'able space on the guest. Data is read or written to the host memory
> > instead of the guest memory. So maybe the right answer is to make
> > intel_iommu_attach_device return error if requested to act on a device
> > with RMRR ranges.
>
> That sounds like the right answer to me. Tom, can you make that
> change when you address the rest of the comments? Or should it
> be a separate patch?
Please include a pr_info when intel_iommu_attach_device takes this path
or we're going to have a really hard time figuring out why a device
won't attach to a guest. Thanks,
Alex
> >> -Original Message-
> >> From: Alex Williamson [mailto:alex.william...@redhat.com]
> >> Sent: Thursday, September 27, 2012 3:37 PM
> >> To: Mingarelli, Thomas
> >> Cc: iommu@lists.linux-foundation.org; Knippers, Linda; Khan, Shuah; Don
> >> Dutile; David Woodhouse
> >> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
> >>
> >> [adding David Woodhouse]
> >>
> >> On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
> >>> When a 32bit PCI device is removed from the SI Domain, the RMRR
> >>> information
> >>> for this device becomes invalid and needs to be reprocessed to avoid DMA
> >>> Read errors. These errors are evidenced by the Present bit being cleared
> >>> in
> >>> the device's context entry. Fixing this problem with an enhancement to
> >>> process
> >>> the RMRR info when the device is assigned to another domain. The Bus
> >>> Master bit
> >>> is cleared during the move to another domain and during the reprocessing
> >>> of
> >>> the RMRR info so no DMA can take place at this time.
> >>>
> >>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> >>>
> >>> drivers/iommu/intel-iommu.c | 47
> >>> --
> >>> 1 files changed, 44 insertions(+), 3 deletions(-)
> >>>
> >>> Signed-off-by: Thomas Mingarelli
> >>>
> >>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> >>> --- ./drivers/iommu/intel-iommu.c.ORIG2012-09-18 09:58:25.147976889
> >>> -0500
> >>> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
> >>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> >>> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> >>> }
> >>>
> >>> +static int reprocess_rmrr(struct device *dev)
> >>> +{
> >>> + struct dmar_rmrr_unit *rmrr;
> >>> + struct pci_dev *pdev;
> >>> + int i, ret;
> >>> +
> >>> + pdev = to_pci_dev(dev);
> >>> +
> >>> + for_each_rmrr_units(rmrr) {
> >>> + for (i = 0; i < rmrr->devices_cnt; i++) {
> >>&
RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
I think this sounds like a separate patch.
Tom
-Original Message-
From: Knippers, Linda
Sent: Thursday, September 27, 2012 4:51 PM
To: Alex Williamson
Cc: Mingarelli, Thomas; iommu@lists.linux-foundation.org; Khan, Shuah; Don
Dutile; David Woodhouse
Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
Alex Williamson wrote:
> On Thu, 2012-09-27 at 21:10 +, Mingarelli, Thomas wrote:
>> Alex,
>>
>> Are you suggesting that a solution is to prevent devices with RMRRs
>> from being placed in the SI Domain in the first place (when pt mode is
>> used)?
>
> No, it seems like it's preferable that devices with RMRRs stay in the si
> domain if the device supports 64bit DMA. They're likely to cause less
> problems there and we can ignore the RMRRs in the si domain. The
> problem I'm trying to address is that the domain you're setting up with
> RMRRs is only used for dma_ops (ie. host driver use). If the device is
> attached to a guest, that domain gets discarded and the device is added
> to yet another domain, potentially with other devices. That domain is
> missing RMRRs, so now your device is going to generate VT-d faults. The
> device can then get removed from that domain, in which case the RMRRs
> should be removed, and again a new dma_ops domain needs to get created
> with RMRRs.
>
> It really seems like RMRRs are incompatible with IOMMU API use though.
> If an RMRR is setup for a VM domain, that's bad because a) it gives the
> VM direct access to that range of host memory, and b) it interferes with
> the guest use of the address space. a) is also bad for isolating
> devices on the host, but the spec makes it available for abuse. For b),
> it's not hard to imagine an RMRR range on the host that overlaps with
> DMA'able space on the guest. Data is read or written to the host memory
> instead of the guest memory. So maybe the right answer is to make
> intel_iommu_attach_device return error if requested to act on a device
> with RMRR ranges.
That sounds like the right answer to me. Tom, can you make that
change when you address the rest of the comments? Or should it
be a separate patch?
-- ljk
> Thanks,
>
> Alex
>
>> -Original Message-
>> From: Alex Williamson [mailto:alex.william...@redhat.com]
>> Sent: Thursday, September 27, 2012 3:37 PM
>> To: Mingarelli, Thomas
>> Cc: iommu@lists.linux-foundation.org; Knippers, Linda; Khan, Shuah; Don
>> Dutile; David Woodhouse
>> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
>>
>> [adding David Woodhouse]
>>
>> On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
>>> When a 32bit PCI device is removed from the SI Domain, the RMRR information
>>> for this device becomes invalid and needs to be reprocessed to avoid DMA
>>> Read errors. These errors are evidenced by the Present bit being cleared in
>>> the device's context entry. Fixing this problem with an enhancement to
>>> process
>>> the RMRR info when the device is assigned to another domain. The Bus Master
>>> bit
>>> is cleared during the move to another domain and during the reprocessing of
>>> the RMRR info so no DMA can take place at this time.
>>>
>>> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>>>
>>> drivers/iommu/intel-iommu.c | 47
>>> --
>>> 1 files changed, 44 insertions(+), 3 deletions(-)
>>>
>>> Signed-off-by: Thomas Mingarelli
>>>
>>> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
>>> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889
>>> -0500
>>> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
>>> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
>>> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
>>> }
>>>
>>> +static int reprocess_rmrr(struct device *dev)
>>> +{
>>> + struct dmar_rmrr_unit *rmrr;
>>> + struct pci_dev *pdev;
>>> + int i, ret;
>>> +
>>> + pdev = to_pci_dev(dev);
>>> +
>>> + for_each_rmrr_units(rmrr) {
>>> + for (i = 0; i < rmrr->devices_cnt; i++) {
>>> + /*
>>> +* Here we are just concerned with
>>> +* finding the one device that was
>>> +* removed from the si_domain and
>>> +* re-evaluating its RMRR info.
>>> +*/
&
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Thu, 2012-09-27 at 15:34 -0600, Alex Williamson wrote: > It really seems like RMRRs are incompatible with IOMMU API use though. > If an RMRR is setup for a VM domain, that's bad because a) it gives the > VM direct access to that range of host memory, and b) it interferes with > the guest use of the address space. a) is also bad for isolating > devices on the host, but the spec makes it available for abuse. For b), > it's not hard to imagine an RMRR range on the host that overlaps with > DMA'able space on the guest. Data is read or written to the host memory > instead of the guest memory. So maybe the right answer is to make > intel_iommu_attach_device return error if requested to act on a device > with RMRR ranges. That would include fairly much any USB host controller. The whole RMRR concept is completely broken and should never have been invented. The idea that firmware-controlled DMA should continue to happen *after* the operating system has been booted and taken control of the hardware is just insane. The majority of RMRR use is for USB controllers, so that firmware can emulate a legacy keyboard for the benefit of pre-USB operating systems. But no operating system that old is ever going to support the IOMMU anyway, so that's just mad. But that's why we've managed to get away with setting up the RMRRs and then tearing them down when a native driver has actually taken control of the hardware — because at that point, it should have been reset and whatever the firmware had configured to do has been abandoned. Perhaps we should have a special case for USB controllers, which are quite happy when you drop their RMRR regions when the OS takes over, and for any *other* device with RMRRs, have a TAINT_YOUR_FIRMWARE_IS_INSANE and just refuse to let a native driver do anything at all with them? -- dwmw2 smime.p7s Description: S/MIME cryptographic signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On Thu, 2012-09-27 at 21:10 +, Mingarelli, Thomas wrote:
> Alex,
>
> Are you suggesting that a solution is to prevent devices with RMRRs
> from being placed in the SI Domain in the first place (when pt mode is
> used)?
No, it seems like it's preferable that devices with RMRRs stay in the si
domain if the device supports 64bit DMA. They're likely to cause less
problems there and we can ignore the RMRRs in the si domain. The
problem I'm trying to address is that the domain you're setting up with
RMRRs is only used for dma_ops (ie. host driver use). If the device is
attached to a guest, that domain gets discarded and the device is added
to yet another domain, potentially with other devices. That domain is
missing RMRRs, so now your device is going to generate VT-d faults. The
device can then get removed from that domain, in which case the RMRRs
should be removed, and again a new dma_ops domain needs to get created
with RMRRs.
It really seems like RMRRs are incompatible with IOMMU API use though.
If an RMRR is setup for a VM domain, that's bad because a) it gives the
VM direct access to that range of host memory, and b) it interferes with
the guest use of the address space. a) is also bad for isolating
devices on the host, but the spec makes it available for abuse. For b),
it's not hard to imagine an RMRR range on the host that overlaps with
DMA'able space on the guest. Data is read or written to the host memory
instead of the guest memory. So maybe the right answer is to make
intel_iommu_attach_device return error if requested to act on a device
with RMRR ranges. Thanks,
Alex
> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Thursday, September 27, 2012 3:37 PM
> To: Mingarelli, Thomas
> Cc: iommu@lists.linux-foundation.org; Knippers, Linda; Khan, Shuah; Don
> Dutile; David Woodhouse
> Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
>
> [adding David Woodhouse]
>
> On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
> > When a 32bit PCI device is removed from the SI Domain, the RMRR information
> > for this device becomes invalid and needs to be reprocessed to avoid DMA
> > Read errors. These errors are evidenced by the Present bit being cleared in
> > the device's context entry. Fixing this problem with an enhancement to
> > process
> > the RMRR info when the device is assigned to another domain. The Bus Master
> > bit
> > is cleared during the move to another domain and during the reprocessing of
> > the RMRR info so no DMA can take place at this time.
> >
> > PATCH v1: https://lkml.org/lkml/2012/6/15/204
> >
> > drivers/iommu/intel-iommu.c | 47
> > --
> > 1 files changed, 44 insertions(+), 3 deletions(-)
> >
> > Signed-off-by: Thomas Mingarelli
> >
> > diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> > --- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889
> > -0500
> > +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
> > @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> > return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> > }
> >
> > +static int reprocess_rmrr(struct device *dev)
> > +{
> > + struct dmar_rmrr_unit *rmrr;
> > + struct pci_dev *pdev;
> > + int i, ret;
> > +
> > + pdev = to_pci_dev(dev);
> > +
> > + for_each_rmrr_units(rmrr) {
> > + for (i = 0; i < rmrr->devices_cnt; i++) {
> > + /*
> > +* Here we are just concerned with
> > +* finding the one device that was
> > +* removed from the si_domain and
> > +* re-evaluating its RMRR info.
> > +*/
>
> I'm still not sure why this comment is wrapped so tightly.
>
> > + if (rmrr->devices[i] != pdev)
> > + continue;
> > + pr_info("IOMMU: Reprocess RMRR information for device
> > %s.\n",
> > + pci_name(pdev));
> > + ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> > + if (ret)
> > + pr_err("IOMMU: Reprocessing RMRR reserved
> > region for device failed");
>
> This could be "if (iommu_prepare_rmrr...)" because...
>
> > + }
> > + }
> > +return 0;
>
> Why does return anything? Looks like it could be a void function since
> you'
RE: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
Alex,
Are you suggesting that a solution is to prevent devices with RMRRs from being
placed in the SI Domain in the first place (when pt mode is used)?
Tom
-Original Message-
From: Alex Williamson [mailto:alex.william...@redhat.com]
Sent: Thursday, September 27, 2012 3:37 PM
To: Mingarelli, Thomas
Cc: iommu@lists.linux-foundation.org; Knippers, Linda; Khan, Shuah; Don Dutile;
David Woodhouse
Subject: Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
[adding David Woodhouse]
On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> for this device becomes invalid and needs to be reprocessed to avoid DMA
> Read errors. These errors are evidenced by the Present bit being cleared in
> the device's context entry. Fixing this problem with an enhancement to process
> the RMRR info when the device is assigned to another domain. The Bus Master
> bit
> is cleared during the move to another domain and during the reprocessing of
> the RMRR info so no DMA can take place at this time.
>
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>
> drivers/iommu/intel-iommu.c | 47 --
> 1 files changed, 44 insertions(+), 3 deletions(-)
>
> Signed-off-by: Thomas Mingarelli
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG2012-09-18 09:58:25.147976889
> -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> }
>
> +static int reprocess_rmrr(struct device *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + struct pci_dev *pdev;
> + int i, ret;
> +
> + pdev = to_pci_dev(dev);
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with
> + * finding the one device that was
> + * removed from the si_domain and
> + * re-evaluating its RMRR info.
> + */
I'm still not sure why this comment is wrapped so tightly.
> + if (rmrr->devices[i] != pdev)
> + continue;
> + pr_info("IOMMU: Reprocess RMRR information for device
> %s.\n",
> + pci_name(pdev));
> + ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> + if (ret)
> + pr_err("IOMMU: Reprocessing RMRR reserved
> region for device failed");
This could be "if (iommu_prepare_rmrr...)" because...
> + }
> + }
> +return 0;
Why does return anything? Looks like it could be a void function since
you're not returning the only possible error case above and not checking
the return value below. You're missing an indent here anyway.
> +}
> +
> /* Check if the pdev needs to go through non-identity map and unmap
> process.*/
> static int iommu_no_mapping(struct device *dev)
> {
> struct pci_dev *pdev;
> - int found;
> + int found, current_bus_master;
>
> if (unlikely(dev->bus != &pci_bus_type))
> return 1;
> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>* 32 bit DMA is removed from si_domain and fall back
>* to non-identity mapping.
>*/
> - domain_remove_one_dev_info(si_domain, pdev);
> printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> -pci_name(pdev));
> + pci_name(pdev));
White space damage? Change this to a pr_info if you really want to
touch it.
> + /*
> + * If a device gets this far we need to clear the Bus
> + * Master bit before we start moving devices from domain
> + * to domain. We will also reset the Bus Master bit
> + * after reprocessing the RMRR info. However, we only
> + * do both the clearing and setting if needed.
> + */
> + current_bus_master = pdev->is_busmaster;
> + if (current_bus_master)
> + pci_clear_master(pdev);
> + domain_remove_one_dev_info(si_domain, pdev);
> + reprocess_rmrr(dev);
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
[adding David Woodhouse]
On Tue, 2012-09-18 at 16:49 +, Tom Mingarelli wrote:
> When a 32bit PCI device is removed from the SI Domain, the RMRR information
> for this device becomes invalid and needs to be reprocessed to avoid DMA
> Read errors. These errors are evidenced by the Present bit being cleared in
> the device's context entry. Fixing this problem with an enhancement to process
> the RMRR info when the device is assigned to another domain. The Bus Master
> bit
> is cleared during the move to another domain and during the reprocessing of
> the RMRR info so no DMA can take place at this time.
>
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
>
> drivers/iommu/intel-iommu.c | 47 --
> 1 files changed, 44 insertions(+), 3 deletions(-)
>
> Signed-off-by: Thomas Mingarelli
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG2012-09-18 09:58:25.147976889
> -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
> @@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
> return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
> }
>
> +static int reprocess_rmrr(struct device *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + struct pci_dev *pdev;
> + int i, ret;
> +
> + pdev = to_pci_dev(dev);
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with
> + * finding the one device that was
> + * removed from the si_domain and
> + * re-evaluating its RMRR info.
> + */
I'm still not sure why this comment is wrapped so tightly.
> + if (rmrr->devices[i] != pdev)
> + continue;
> + pr_info("IOMMU: Reprocess RMRR information for device
> %s.\n",
> + pci_name(pdev));
> + ret = iommu_prepare_rmrr_dev(rmrr, pdev);
> + if (ret)
> + pr_err("IOMMU: Reprocessing RMRR reserved
> region for device failed");
This could be "if (iommu_prepare_rmrr...)" because...
> + }
> + }
> +return 0;
Why does return anything? Looks like it could be a void function since
you're not returning the only possible error case above and not checking
the return value below. You're missing an indent here anyway.
> +}
> +
> /* Check if the pdev needs to go through non-identity map and unmap
> process.*/
> static int iommu_no_mapping(struct device *dev)
> {
> struct pci_dev *pdev;
> - int found;
> + int found, current_bus_master;
>
> if (unlikely(dev->bus != &pci_bus_type))
> return 1;
> @@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
>* 32 bit DMA is removed from si_domain and fall back
>* to non-identity mapping.
>*/
> - domain_remove_one_dev_info(si_domain, pdev);
> printk(KERN_INFO "32bit %s uses non-identity mapping\n",
> -pci_name(pdev));
> + pci_name(pdev));
White space damage? Change this to a pr_info if you really want to
touch it.
> + /*
> + * If a device gets this far we need to clear the Bus
> + * Master bit before we start moving devices from domain
> + * to domain. We will also reset the Bus Master bit
> + * after reprocessing the RMRR info. However, we only
> + * do both the clearing and setting if needed.
> + */
> + current_bus_master = pdev->is_busmaster;
> + if (current_bus_master)
> + pci_clear_master(pdev);
> + domain_remove_one_dev_info(si_domain, pdev);
> + reprocess_rmrr(dev);
> + if (current_bus_master)
> + pci_set_master(pdev);
I don't know any better way to halt DMA since we can't move the device
to a new domain atomically, but what about the other cases where we
switch domains? For instance, what if some unsuspecting user tries to
assign the device to a guest? I think it's generally inadvisable to
assign a devices with RMRRs to a guest, but if they do, they're going to
run into the same problem. The RMRRs aren't reprocessed for the VM
domain and again aren't reprocessed when returned to a standard device
domain. Even more fun, if assigned to a VM domain, RMRRs should be
added with the device and removed if the device is later detached from
the domain.
The lazy solution might be to disallow devices with RMRRs from be
Re: [PATCH v2] Intel IOMMU patch to reprocess RMRR info
On 09/18/2012 12:49 PM, Tom Mingarelli wrote:
When a 32bit PCI device is removed from the SI Domain, the RMRR information
for this device becomes invalid and needs to be reprocessed to avoid DMA
Read errors. These errors are evidenced by the Present bit being cleared in
the device's context entry. Fixing this problem with an enhancement to process
the RMRR info when the device is assigned to another domain. The Bus Master bit
is cleared during the move to another domain and during the reprocessing of
the RMRR info so no DMA can take place at this time.
PATCH v1: https://lkml.org/lkml/2012/6/15/204
drivers/iommu/intel-iommu.c | 47 --
1 files changed, 44 insertions(+), 3 deletions(-)
Signed-off-by: Thomas Mingarelli
diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889 -0500
+++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
@@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
}
+static int reprocess_rmrr(struct device *dev)
+{
+ struct dmar_rmrr_unit *rmrr;
+ struct pci_dev *pdev;
+ int i, ret;
+
+ pdev = to_pci_dev(dev);
+
+ for_each_rmrr_units(rmrr) {
+ for (i = 0; i< rmrr->devices_cnt; i++) {
+ /*
+* Here we are just concerned with
+* finding the one device that was
+* removed from the si_domain and
+* re-evaluating its RMRR info.
+*/
+ if (rmrr->devices[i] != pdev)
+ continue;
+ pr_info("IOMMU: Reprocess RMRR information for device
%s.\n",
+ pci_name(pdev));
+ ret = iommu_prepare_rmrr_dev(rmrr, pdev);
+ if (ret)
+ pr_err("IOMMU: Reprocessing RMRR reserved region for
device failed");
+ }
+ }
+return 0;
+}
+
/* Check if the pdev needs to go through non-identity map and unmap process.*/
static int iommu_no_mapping(struct device *dev)
{
struct pci_dev *pdev;
- int found;
+ int found, current_bus_master;
if (unlikely(dev->bus !=&pci_bus_type))
return 1;
@@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
* 32 bit DMA is removed from si_domain and fall back
* to non-identity mapping.
*/
- domain_remove_one_dev_info(si_domain, pdev);
printk(KERN_INFO "32bit %s uses non-identity mapping\n",
- pci_name(pdev));
+ pci_name(pdev));
+ /*
+* If a device gets this far we need to clear the Bus
+* Master bit before we start moving devices from domain
+* to domain. We will also reset the Bus Master bit
+* after reprocessing the RMRR info. However, we only
+* do both the clearing and setting if needed.
+*/
+ current_bus_master = pdev->is_busmaster;
+ if (current_bus_master)
+ pci_clear_master(pdev);
+ domain_remove_one_dev_info(si_domain, pdev);
+ reprocess_rmrr(dev);
+ if (current_bus_master)
+ pci_set_master(pdev);
return 0;
}
} else {
appears to have recommended changes from v1, so looks good wrt handling devices
w/rmrr.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] Intel IOMMU patch to reprocess RMRR info
When a 32bit PCI device is removed from the SI Domain, the RMRR information
for this device becomes invalid and needs to be reprocessed to avoid DMA
Read errors. These errors are evidenced by the Present bit being cleared in
the device's context entry. Fixing this problem with an enhancement to process
the RMRR info when the device is assigned to another domain. The Bus Master bit
is cleared during the move to another domain and during the reprocessing of
the RMRR info so no DMA can take place at this time.
PATCH v1: https://lkml.org/lkml/2012/6/15/204
drivers/iommu/intel-iommu.c | 47 --
1 files changed, 44 insertions(+), 3 deletions(-)
Signed-off-by: Thomas Mingarelli
diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889 -0500
+++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
@@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
}
+static int reprocess_rmrr(struct device *dev)
+{
+ struct dmar_rmrr_unit *rmrr;
+ struct pci_dev *pdev;
+ int i, ret;
+
+ pdev = to_pci_dev(dev);
+
+ for_each_rmrr_units(rmrr) {
+ for (i = 0; i < rmrr->devices_cnt; i++) {
+ /*
+* Here we are just concerned with
+* finding the one device that was
+* removed from the si_domain and
+* re-evaluating its RMRR info.
+*/
+ if (rmrr->devices[i] != pdev)
+ continue;
+ pr_info("IOMMU: Reprocess RMRR information for device
%s.\n",
+ pci_name(pdev));
+ ret = iommu_prepare_rmrr_dev(rmrr, pdev);
+ if (ret)
+ pr_err("IOMMU: Reprocessing RMRR reserved
region for device failed");
+ }
+ }
+return 0;
+}
+
/* Check if the pdev needs to go through non-identity map and unmap process.*/
static int iommu_no_mapping(struct device *dev)
{
struct pci_dev *pdev;
- int found;
+ int found, current_bus_master;
if (unlikely(dev->bus != &pci_bus_type))
return 1;
@@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
* 32 bit DMA is removed from si_domain and fall back
* to non-identity mapping.
*/
- domain_remove_one_dev_info(si_domain, pdev);
printk(KERN_INFO "32bit %s uses non-identity mapping\n",
- pci_name(pdev));
+ pci_name(pdev));
+ /*
+* If a device gets this far we need to clear the Bus
+* Master bit before we start moving devices from domain
+* to domain. We will also reset the Bus Master bit
+* after reprocessing the RMRR info. However, we only
+* do both the clearing and setting if needed.
+*/
+ current_bus_master = pdev->is_busmaster;
+ if (current_bus_master)
+ pci_clear_master(pdev);
+ domain_remove_one_dev_info(si_domain, pdev);
+ reprocess_rmrr(dev);
+ if (current_bus_master)
+ pci_set_master(pdev);
return 0;
}
} else {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] Intel IOMMU patch to reprocess RMRR info
When a 32bit PCI device is removed from the SI Domain, the RMRR information
for this device becomes invalid and needs to be reprocessed to avoid DMA
Read errors. These errors are evidenced by the Present bit being cleared in
the device's context entry. Fixing this problem with an enhancement to process
the RMRR info when the device is assigned to another domain. The Bus Master bit
is cleared during the move to another domain and during the reprocessing of
the RMRR info so no DMA can take place at this time.
PATCH v1: https://lkml.org/lkml/2012/6/15/204
drivers/iommu/intel-iommu.c | 47 --
1 files changed, 44 insertions(+), 3 deletions(-)
Signed-off-by: Thomas Mingarelli
diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG 2012-09-18 09:58:25.147976889 -0500
+++ ./drivers/iommu/intel-iommu.c 2012-09-18 10:39:43.286672765 -0500
@@ -2706,11 +2706,39 @@ static int iommu_dummy(struct pci_dev *p
return pdev->dev.archdata.iommu == DUMMY_DEVICE_DOMAIN_INFO;
}
+static int reprocess_rmrr(struct device *dev)
+{
+ struct dmar_rmrr_unit *rmrr;
+ struct pci_dev *pdev;
+ int i, ret;
+
+ pdev = to_pci_dev(dev);
+
+ for_each_rmrr_units(rmrr) {
+ for (i = 0; i < rmrr->devices_cnt; i++) {
+ /*
+* Here we are just concerned with
+* finding the one device that was
+* removed from the si_domain and
+* re-evaluating its RMRR info.
+*/
+ if (rmrr->devices[i] != pdev)
+ continue;
+ pr_info("IOMMU: Reprocess RMRR information for device
%s.\n",
+ pci_name(pdev));
+ ret = iommu_prepare_rmrr_dev(rmrr, pdev);
+ if (ret)
+ pr_err("IOMMU: Reprocessing RMRR reserved
region for device failed");
+ }
+ }
+return 0;
+}
+
/* Check if the pdev needs to go through non-identity map and unmap process.*/
static int iommu_no_mapping(struct device *dev)
{
struct pci_dev *pdev;
- int found;
+ int found, current_bus_master;
if (unlikely(dev->bus != &pci_bus_type))
return 1;
@@ -2731,9 +2759,22 @@ static int iommu_no_mapping(struct devic
* 32 bit DMA is removed from si_domain and fall back
* to non-identity mapping.
*/
- domain_remove_one_dev_info(si_domain, pdev);
printk(KERN_INFO "32bit %s uses non-identity mapping\n",
- pci_name(pdev));
+ pci_name(pdev));
+ /*
+* If a device gets this far we need to clear the Bus
+* Master bit before we start moving devices from domain
+* to domain. We will also reset the Bus Master bit
+* after reprocessing the RMRR info. However, we only
+* do both the clearing and setting if needed.
+*/
+ current_bus_master = pdev->is_busmaster;
+ if (current_bus_master)
+ pci_clear_master(pdev);
+ domain_remove_one_dev_info(si_domain, pdev);
+ reprocess_rmrr(dev);
+ if (current_bus_master)
+ pci_set_master(pdev);
return 0;
}
} else {
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
