2012-10-16 16:50:35

by Tom Mingarelli

[permalink] [raw]
Subject: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

This patch is to prevent devices that have RMRRs associated with them
from getting placed into the SI Domain during init. We don't put USB devices
into this category, however. This fixes the issue where the RMRR info
for devices being placed in and out of the SI Domain gets lost.

Signed-off-by: Thomas Mingarelli <[email protected]>
----
PATCH v1: https://lkml.org/lkml/2012/6/15/204
PATCH v2: https://lkml.org/lkml/2012/9/18/354

drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
1 files changed, 33 insertions(+), 0 deletions(-)

diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
--- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
+++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
@@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
return 0;
}

+static bool device_has_rmrr(struct pci_dev *dev)
+{
+ struct dmar_rmrr_unit *rmrr;
+ int i;
+
+ for_each_rmrr_units(rmrr) {
+ for (i = 0; i < rmrr->devices_cnt; i++) {
+ /*
+ * Here we are just concerned with checking each device
+ * that has an RMRR associated with it and not allow it
+ * to be placed into the SI Domain during startup.
+ */
+ if (rmrr->devices[i] == dev)
+ return true;
+ }
+ }
+ return false;
+}
+
static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
{
+
+ if (startup) {
+ /*
+ * This is where we will refuse any device that has an
+ * RMRR associated with it and is not a USB device and
+ * NOT allow it to be placed into the SI Domain. We
+ * only do this on startup. We don't need a separate bit
+ * for this because it could be ANY device.
+ */
+ if (device_has_rmrr(pdev) &&
+ (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)
+ return 0;
+ }
+
if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
return 1;


2012-10-16 16:59:52

by Alex Williamson

[permalink] [raw]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +
> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup.

Why only on startup?

> We don't need a separate bit
> + * for this because it could be ANY device.
> + */

Note the white space here doesn't match existing comment blocks in
intel-iommu.

> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)
> + return 0;
> + }
> +
> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>

Thanks,
Alex

2012-10-16 17:26:07

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +

Will you use the same routine to deny device assignment request for
devices with RMRR? Is that going to be another patch?

> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup. We don't need a separate bit
> + * for this because it could be ANY device.
> + */
> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)
> + return 0;
> + }
> +

Is there a reason to not group this with the other pci device checks
below. Don't you need this done whenever iommu_should_identity_map() get
called as opposed just at startup?

> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>

2012-10-16 17:28:16

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +
> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup. We don't need a separate bit
> + * for this because it could be ANY device.
> + */
> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)

Forgot to ask in my last response. Is it sufficient to check _USB. Are
we missing any other devices that use RMRR that would qualify?

> + return 0;
> + }
> +
> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>

2012-10-16 17:40:02

by Tom Mingarelli

[permalink] [raw]
Subject: RE: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

I only perform this action on startup because that is the first time the devices are placed into the SI Domain when passthrough mode is used. If they never make it into the SI Domain then there is no RMRR confusion to worry about.


Tom

-----Original Message-----
From: Alex Williamson [mailto:[email protected]]
Sent: Tuesday, October 16, 2012 12:00 PM
To: Mingarelli, Thomas
Cc: David Woodhouse; Don Dutile; Khan, Shuah; Knippers, Linda; [email protected]; [email protected]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +
> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup.

Why only on startup?

> We don't need a separate bit
> + * for this because it could be ANY device.
> + */

Note the white space here doesn't match existing comment blocks in
intel-iommu.

> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)
> + return 0;
> + }
> +
> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>

Thanks,
Alex

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2012-10-16 17:40:55

by Tom Mingarelli

[permalink] [raw]
Subject: RE: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

I was under the impression this was ONLY a concern with devices placed in and out of the SI Domain when passthrough mode is used. If those devices never make it into the SI Domain in the first place then there should be no concern.


Tom

-----Original Message-----
From: Khan, Shuah
Sent: Tuesday, October 16, 2012 12:26 PM
To: Mingarelli, Thomas
Cc: Alex Williamson; David Woodhouse; Don Dutile; Knippers, Linda; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +

Will you use the same routine to deny device assignment request for
devices with RMRR? Is that going to be another patch?

> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup. We don't need a separate bit
> + * for this because it could be ANY device.
> + */
> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)
> + return 0;
> + }
> +

Is there a reason to not group this with the other pci device checks
below. Don't you need this done whenever iommu_should_identity_map() get
called as opposed just at startup?

> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2012-10-16 17:42:03

by Tom Mingarelli

[permalink] [raw]
Subject: RE: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

I imagine there could be other devices that get a "free ride". Any help or suggestions in that area are greatly appreciated.


Tom

-----Original Message-----
From: Khan, Shuah
Sent: Tuesday, October 16, 2012 12:28 PM
To: Mingarelli, Thomas
Cc: Alex Williamson; David Woodhouse; Don Dutile; Knippers, Linda; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v3] Prevent devices with RMRRs from being placed into SI Domain during startup

On Tue, 2012-10-16 at 16:50 +0000, Tom Mingarelli wrote:
> This patch is to prevent devices that have RMRRs associated with them
> from getting placed into the SI Domain during init. We don't put USB devices
> into this category, however. This fixes the issue where the RMRR info
> for devices being placed in and out of the SI Domain gets lost.
>
> Signed-off-by: Thomas Mingarelli <[email protected]>
> ----
> PATCH v1: https://lkml.org/lkml/2012/6/15/204
> PATCH v2: https://lkml.org/lkml/2012/9/18/354
>
> drivers/iommu/intel-iommu.c | 33 +++++++++++++++++++++++++++++++++
> 1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff -up ./drivers/iommu/intel-iommu.c.ORIG ./drivers/iommu/intel-iommu.c
> --- ./drivers/iommu/intel-iommu.c.ORIG 2012-10-16 09:34:23.148089944 -0500
> +++ ./drivers/iommu/intel-iommu.c 2012-10-16 09:56:56.905932861 -0500
> @@ -2320,8 +2320,41 @@ static int domain_add_dev_info(struct dm
> return 0;
> }
>
> +static bool device_has_rmrr(struct pci_dev *dev)
> +{
> + struct dmar_rmrr_unit *rmrr;
> + int i;
> +
> + for_each_rmrr_units(rmrr) {
> + for (i = 0; i < rmrr->devices_cnt; i++) {
> + /*
> + * Here we are just concerned with checking each device
> + * that has an RMRR associated with it and not allow it
> + * to be placed into the SI Domain during startup.
> + */
> + if (rmrr->devices[i] == dev)
> + return true;
> + }
> + }
> + return false;
> +}
> +
> static int iommu_should_identity_map(struct pci_dev *pdev, int startup)
> {
> +
> + if (startup) {
> + /*
> + * This is where we will refuse any device that has an
> + * RMRR associated with it and is not a USB device and
> + * NOT allow it to be placed into the SI Domain. We
> + * only do this on startup. We don't need a separate bit
> + * for this because it could be ANY device.
> + */
> + if (device_has_rmrr(pdev) &&
> + (pdev->class >> 8) != PCI_CLASS_SERIAL_USB)

Forgot to ask in my last response. Is it sufficient to check _USB. Are
we missing any other devices that use RMRR that would qualify?

> + return 0;
> + }
> +
> if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev))
> return 1;
>


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?