PCI: Extend ACS configurability

For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early in the boot process, either via the BIOS or via the kernel disable_acs_redir parameter.

disable_acs_redir allows clearing the RR|CR|EC ACS flags, but the PCIe spec Rev3.0 already defines 7 different ACS related flags with many more useful combinations depending on the fabric design.

For backward compatibility, leave the 'disable_acs_redir' as is and add a new parameter 'config_acs'so that the user can directly specify the ACS flags to set on a per-device basis. Use a similar syntax to the existing 'resource_alignment'  parameter by using the @ character and have the user specify the ACS flags using a bit encoding. If both 'disable_acs_redir' and 'config_acs' are specified for a particular device, configuration specified through 'config_acs' takes precedence over the other.

Signed-off-by Vidya Sagar <[email protected]>

---
v3:
* Fixed a documentation issue reported by kernel test bot

v2:
* Refactored the code as per Jason's suggestion

 .../admin-guide/kernel-parameters.txt         |  22 +++
 drivers/pci/pci.c                             | 148 +++++++++++-------
 2 files changed, 112 insertions(+), 58 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +22 -0

index 41644336e..b4a8207eb 100644
@@ -4456,6 +4456,28 @@
 				bridges without forcing it upstream. Note:
 				this removes isolation between devices and
 				may put more devices in an IOMMU group.
+		config_acs=
+				Format:
+				=<ACS flags>@<pci_dev>[; ...]
+				Specify one or more PCI devices (in the format
+				specified above) optionally prepended with flags
+				and separated by semicolons. The respective
+				capabilities will be enabled, disabled or unchanged
+				based on what is specified in flags.
+				ACS Flags is defined as follows
+				bit-0 : ACS Source Validation
+				bit-1 : ACS Translation Blocking
+				bit-2 : ACS P2P Request Redirect
+				bit-3 : ACS P2P Completion Redirect
+				bit-4 : ACS Upstream Forwarding
+				bit-5 : ACS P2P Egress Control
+				bit-6 : ACS Direct Translated P2P
+				Each bit can be marked as
+				‘0‘ – force disabled
+				‘1’ – force enabled
+				‘x’ – unchanged.
+				Note: this may remove isolation between devices
+				and may put more devices in an IOMMU group.
 		force_floating	[S390] Force usage of floating interrupts.
 		nomio		[S390] Do not use MIO instructions.
 		norid		[S390] ignore the RID field and force use of

drivers/pci/pci.c +90 -58

index a607f277c..a46264f83 100644
@@ -887,30 +887,67 @@ void pci_request_acs(void)
 }
 
 static const char *disable_acs_redir_param;
+static const char *config_acs_param;
 
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
+struct pci_acs {
+	u16 cap;
+	u16 ctrl;
+	u16 fw_ctrl;
+};
+
+static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
+			     const char *p, u16 mask, u16 flags)
 {
+	char *delimit;
 	int ret = 0;
-	const char *p;
-	int pos;
-	u16 ctrl;
 
-	if (!disable_acs_redir_param)
+	if (!p)
 		return;
 
-	p = disable_acs_redir_param;
 	while (*p) {
+		if (!mask) {
+			/* Check for ACS flags */
+			delimit = strstr(p, "@");
+			if (delimit) {
+				int end;
+				u32 shift = 0;
+
+				end = delimit - p - 1;
+
+				while (end > -1) {
+					if (*(p + end) == '0') {
+						mask |= 1 << shift;
+						shift++;
+						end--;
+					} else if (*(p + end) == '1') {
+						mask |= 1 << shift;
+						flags |= 1 << shift;
+						shift++;
+						end--;
+					} else if ((*(p + end) == 'x') || (*(p + end) == 'X')) {
+						shift++;
+						end--;
+					} else {
+						pci_err(dev, "Invalid ACS flags... Ignoring\n");
+						return;
+					}
+				}
+				p = delimit + 1;
+			} else {
+				pci_err(dev, "ACS Flags missing\n");
+				return;
+			}
+		}
+
+		if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
+			    PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
+			pci_err(dev, "Invalid ACS flags specified\n");
+			return;
+		}
+
 		ret = pci_dev_str_match(dev, p, &p);
 		if (ret < 0) {
-			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
-				     disable_acs_redir_param);
-
+			pr_info_once("PCI: Can't parse acs command line parameter\n");
 			break;
 		} else if (ret == 1) {
 			/* Found a match */

@@ -930,56 +967,38 @@ static void pci_disable_acs_redir(struct pci_dev *dev)
 	if (!pci_dev_specific_disable_acs_redir(dev))
 		return;
 
-	pos = dev->acs_cap;
-	if (!pos) {
-		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
-		return;
-	}
-
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+	pci_dbg(dev, "ACS mask  = 0x%X\n", mask);
+	pci_dbg(dev, "ACS flags = 0x%X\n", flags);
 
-	/* P2P Request & Completion Redirect */
-	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+	/* If mask is 0 then we copy the bit from the firmware setting. */
+	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
+	caps->ctrl |= flags;
 
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
-
-	pci_info(dev, "disabled ACS redirect\n");
+	pci_info(dev, "Configured ACS to 0x%x\n", caps->ctrl);
 }
 
 /**
  * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
  * @dev: the PCI device
+ * @caps: default ACS controls
  */
-static void pci_std_enable_acs(struct pci_dev *dev)
+static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
 {
-	int pos;
-	u16 cap;
-	u16 ctrl;
-
-	pos = dev->acs_cap;
-	if (!pos)
-		return;
-
-	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
 	/* Source Validation */
-	ctrl |= (cap & PCI_ACS_SV);
+	caps->ctrl |= (caps->cap & PCI_ACS_SV);
 
 	/* P2P Request Redirect */
-	ctrl |= (cap & PCI_ACS_RR);
+	caps->ctrl |= (caps->cap & PCI_ACS_RR);
 
 	/* P2P Completion Redirect */
-	ctrl |= (cap & PCI_ACS_CR);
+	caps->ctrl |= (caps->cap & PCI_ACS_CR);
 
 	/* Upstream Forwarding */
-	ctrl |= (cap & PCI_ACS_UF);
+	caps->ctrl |= (caps->cap & PCI_ACS_UF);
 
 	/* Enable Translation Blocking for external devices and noats */
 	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
-		ctrl |= (cap & PCI_ACS_TB);
-
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+		caps->ctrl |= (caps->cap & PCI_ACS_TB);
 }
 
 /**

@@ -988,23 +1007,33 @@ static void pci_std_enable_acs(struct pci_dev *dev)
  */
 static void pci_enable_acs(struct pci_dev *dev)
 {
-	if (!pci_acs_enable)
-		goto disable_acs_redir;
+	struct pci_acs caps;
+	int pos;
+
+	pos = dev->acs_cap;
+	if (!pos)
+		return;
 
-	if (!pci_dev_specific_enable_acs(dev))
-		goto disable_acs_redir;
+	pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+	caps.fw_ctrl = caps.ctrl;
 
-	pci_std_enable_acs(dev);
+	/* If an iommu is present we start with kernel default caps */
+	if (pci_acs_enable) {
+		if (pci_dev_specific_enable_acs(dev))
+			pci_std_enable_acs(dev, &caps);
+	}
 
-disable_acs_redir:
 	/*
-	 * Note: pci_disable_acs_redir() must be called even if ACS was not
-	 * enabled by the kernel because it may have been enabled by
-	 * platform firmware.  So if we are told to disable it, we should
-	 * always disable it after setting the kernel's default
-	 * preferences.
+	 * Always apply caps from the command line, even if there is no iommu.
+	 * Trust that the admin has a reason to change the ACS settings.
 	 */
-	pci_disable_acs_redir(dev);
+	__pci_config_acs(dev, &caps, disable_acs_redir_param,
+			 PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC,
+			 ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC));
+	__pci_config_acs(dev, &caps, config_acs_param, 0, 0);
+
+	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
 }
 
 /**

@@ -7023,6 +7052,8 @@ static int __init pci_setup(char *str)
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
 			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
 				disable_acs_redir_param = str + 18;
+			} else if (!strncmp(str, "config_acs=", 11)) {
+				config_acs_param = str + 11;
 			} else {
 				pr_err("PCI: Unknown option `%s'\n", str);
 			}

@@ -7047,6 +7078,7 @@ static int __init pci_realloc_setup_params(void)
 	resource_alignment_param = kstrdup(resource_alignment_param,
 					   GFP_KERNEL);
 	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
+	config_acs_param = kstrdup(config_acs_param, GFP_KERNEL);
 
 	return 0;
 }

Signature · 1 line

2.25.1

[+cc iommu folks]

Quoted 3 lines · Vidya Sagar

For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early in the boot process, either via the BIOS or via the kernel disable_acs_redir parameter.

Can you point to the iommu code that is involved here? It sounds like the iommu_groups are built at boot time and are immutable after that?

If we need per-device ACS config that depends on the workload, it seems kind of problematic to only be able to specify this at boot time. I guess we would need to reboot if we want to run a workload that needs a different config?

Is this the iommu usage model we want in the long term?

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
Sent: Thursday, May 23, 2024 8:46 PM
To: Bjorn Helgaas <[email protected]>
Cc: Vidya Sagar <[email protected]>; [email protected]; [email protected]; Gal Shalom <[email protected]>; Leon Romanovsky <[email protected]>; Thierry Reding <[email protected]>; Jon Hunter <[email protected]>; Masoud Moshref Javadi <[email protected]>; Shahaf Shuler <[email protected]>; Vikram Sethi <[email protected]>; Shanker Donthineni <[email protected]>; Jiandi An <[email protected]>; Tushar Dave <[email protected]>; linux- [email protected]; [email protected]; [email protected]; Krishna Thota <[email protected]>; Manikanta Maddireddy <[email protected]>; [email protected]; Joerg Roedel <[email protected]>; Will Deacon <[email protected]>; Robin Murphy <[email protected]>; [email protected]
Subject: Re: [PATCH V3] PCI: Extend ACS configurability

Quoted 9 lines · Bjorn Helgaas

[+cc iommu folks]

Quoted 3 lines · Vidya Sagar

For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early in the boot process, either via the BIOS or via the kernel disable_acs_redir parameter.

Can you point to the iommu code that is involved here? It sounds like the iommu_groups are built at boot time and are immutable after that?

They are created when the struct device is plugged in. pci_device_group() does the logic.

Notably groups can't/don't change if details like ACS change after the groups are setup.

There are alot of instructions out there telling people to boot their servers and then manually change the ACS flags with set_pci or something, and these are not good instructions since it defeats the VFIO group based security mechanisms.

Quoted 4 lines

If we need per-device ACS config that depends on the workload, it seems kind of problematic to only be able to specify this at boot time. I guess we would need to reboot if we want to run a workload that needs a different config?

Basically. The main difference I'd see is if the server is a VM host or running bare metal apps. You can get more efficicenty if you change things for the bare metal case, and often bare metal will want to turn the iommu off while a VM host often wants more of it turned on.

Quoted 1 line

Is this the iommu usage model we want in the long term?

There is some path to more dynamic behavior here, but it would require separating groups into two components - devices that are together because they are physically sharing translation (aliases and things) from devices that are together because they share a security boundary (ACS).

It is more believable we could dynamically change security group assigments for VFIO than translation group assignment. I don't know anyone interested in this right now - Alex and I have only talked about it as a possibility a while back.

FWIW I don't view patch as excluding more dynamisism in the future, but it is the best way to work with the current state of affairs, and definitely better than set_pci instructions.

Thanks,
Jason

Hi Bjorn,
Could you let me know if Jason's reply answers your question? Please let me know if you are looking for any more information.

Quoted 52 lines

-----Original Message-----
From: Jason Gunthorpe <[email protected]>
...

Quoted 9 lines · Bjorn Helgaas

[+cc iommu folks]

Quoted 3 lines · Vidya Sagar

For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early in the boot process, either via the BIOS or via the kernel disable_acs_redir parameter.

Can you point to the iommu code that is involved here? It sounds like the iommu_groups are built at boot time and are immutable after that?

They are created when the struct device is plugged in. pci_device_group() does the logic.

Notably groups can't/don't change if details like ACS change after the groups are setup.

There are alot of instructions out there telling people to boot their servers and then manually change the ACS flags with set_pci or something, and these are not good instructions since it defeats the VFIO group based security mechanisms.

Quoted 4 lines

If we need per-device ACS config that depends on the workload, it seems kind of problematic to only be able to specify this at boot time. I guess we would need to reboot if we want to run a workload that needs a different config?

Basically. The main difference I'd see is if the server is a VM host or running bare metal apps. You can get more efficicenty if you change things for the bare metal case, and often bare metal will want to turn the iommu off while a VM host often wants more of it turned on.

Quoted 1 line

Is this the iommu usage model we want in the long term?

There is some path to more dynamic behavior here, but it would require separating groups into two components - devices that are together because they are physically sharing translation (aliases and things) from devices that are together because they share a security boundary (ACS).

It is more believable we could dynamically change security group assigments for VFIO than translation group assignment. I don't know anyone interested in this right now - Alex and I have only talked about it as a possibility a while back.

FWIW I don't view patch as excluding more dynamisism in the future, but it is the best way to work with the current state of affairs, and definitely better than set_pci instructions.

Thanks,
Jason

"Correctly" is not quite the right word here; it's just a fact that the ACS settings determined at boot time result in certain IOMMU groups. If the user desires different groups, it's not that something is "incorrect"; it's just that the user may have to accept less isolation to get the desired IOMMU groups.

Quoted 5 lines · Bjorn Helgaas

"Correctly" is not quite the right word here; it's just a fact that the ACS settings determined at boot time result in certain IOMMU groups. If the user desires different groups, it's not that something is "incorrect"; it's just that the user may have to accept less isolation to get the desired IOMMU groups.

That is not quite accurate.. There are HW configurations where ACS needs to be a certain way for the HW to work with P2P at all. It isn't just an optimization or the user accepts something, if they want P2P at all they must get a ACS configuration appropriate for their system.

[+cc Alex since VFIO entered the conversation; thread at
https://lore.kernel.org/r/[email protected]]

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Bjorn Helgaas

"Correctly" is not quite the right word here; it's just a fact that the ACS settings determined at boot time result in certain IOMMU groups. If the user desires different groups, it's not that something is "incorrect"; it's just that the user may have to accept less isolation to get the desired IOMMU groups.

That is not quite accurate.. There are HW configurations where ACS needs to be a certain way for the HW to work with P2P at all. It isn't just an optimization or the user accepts something, if they want P2P at all they must get a ACS configuration appropriate for their system.

The current wording of "For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early" suggests that the way we currently configure ACS is incorrect in general, regardless of P2PDMA.

But my impression is that there's a trade-off between isolation and the ability to do P2PDMA, and users have different requirements, and the preference for less isolation/more P2PDMA is no more "correct" than a preference for more isolation/less P2PDMA.

Maybe something like this:

PCIe ACS settings determine how devices are put into iommu_groups. The iommu_groups in turn determine which devices can be passed through to VMs and whether P2PDMA between them is possible. The iommu_groups are built at enumeration-time and are currently static.

Quoted 16 lines · Bjorn Helgaas

[+cc Alex since VFIO entered the conversation; thread at
https://lore.kernel.org/r/[email protected]]

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Bjorn Helgaas

"Correctly" is not quite the right word here; it's just a fact that the ACS settings determined at boot time result in certain IOMMU groups. If the user desires different groups, it's not that something is "incorrect"; it's just that the user may have to accept less isolation to get the desired IOMMU groups.

That is not quite accurate.. There are HW configurations where ACS needs to be a certain way for the HW to work with P2P at all. It isn't just an optimization or the user accepts something, if they want P2P at all they must get a ACS configuration appropriate for their system.

Quoted 4 lines

The current wording of "For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early" suggests that the way we currently configure ACS is incorrect in general, regardless of P2PDMA.

Yes, I'd agree with this. We don't have enough information to configurate it properly in the kernel in an automatic way. We don't know if pairs of devices even have SW enablement to do P2P in the kernel and we don't accurately know what issues the root complex has. All of this information goes into choosing the right ACS bits.

Quoted 4 lines

But my impression is that there's a trade-off between isolation and the ability to do P2PDMA, and users have different requirements, and the preference for less isolation/more P2PDMA is no more "correct" than a preference for more isolation/less P2PDMA.

Sure, that makes sense

Quoted 6 lines

Maybe something like this:

PCIe ACS settings determine how devices are put into iommu_groups. The iommu_groups in turn determine which devices can be passed through to VMs and whether P2PDMA between them is possible. The iommu_groups are built at enumeration-time and are currently static.

Not quite, the iommu_groups don't have alot to do with the P2P. Even devices in the same kernel group can still have non working P2P.

Maybe:

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

It's unfortunate that this requires so much expert knowledge to use, but I guess we don't really have a good alternative. The only way I can think of to help would be some kind of white paper or examples in Documentation/PCI/.

Quoted 16 lines · Bjorn Helgaas

[+cc Alex since VFIO entered the conversation; thread at
https://lore.kernel.org/r/[email protected]]

Quoted 11 lines · Jason Gunthorpe

Quoted 5 lines · Bjorn Helgaas

"Correctly" is not quite the right word here; it's just a fact that the ACS settings determined at boot time result in certain IOMMU groups. If the user desires different groups, it's not that something is "incorrect"; it's just that the user may have to accept less isolation to get the desired IOMMU groups.

That is not quite accurate.. There are HW configurations where ACS needs to be a certain way for the HW to work with P2P at all. It isn't just an optimization or the user accepts something, if they want P2P at all they must get a ACS configuration appropriate for their system.

Quoted 4 lines

The current wording of "For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early" suggests that the way we currently configure ACS is incorrect in general, regardless of P2PDMA.

Yes, I'd agree with this. We don't have enough information to configurate it properly in the kernel in an automatic way. We don't know if pairs of devices even have SW enablement to do P2P in the kernel and we don't accurately know what issues the root complex has. All of this information goes into choosing the right ACS bits.

Quoted 4 lines

But my impression is that there's a trade-off between isolation and the ability to do P2PDMA, and users have different requirements, and the preference for less isolation/more P2PDMA is no more "correct" than a preference for more isolation/less P2PDMA.

Sure, that makes sense

Quoted 6 lines

Maybe something like this:

PCIe ACS settings determine how devices are put into iommu_groups. The iommu_groups in turn determine which devices can be passed through to VMs and whether P2PDMA between them is possible. The iommu_groups are built at enumeration-time and are currently static.

Not quite, the iommu_groups don't have alot to do with the P2P. Even devices in the same kernel group can still have non working P2P.

Maybe:

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

For iommu_groups to form correctly, the ACS settings in the PCIe fabric need to be setup early in the boot process, either via the BIOS or via the kernel disable_acs_redir parameter.

disable_acs_redir allows clearing the RR|CR|EC ACS flags, but the PCIe spec Rev3.0 already defines 7 different ACS related flags with many more useful combinations depending on the fabric design.

For backward compatibility, leave the 'disable_acs_redir' as is and add a new parameter 'config_acs'so that the user can directly specify the ACS flags to set on a per-device basis. Use a similar syntax to the existing 'resource_alignment'  parameter by using the @ character and have the user specify the ACS flags using a bit encoding. If both 'disable_acs_redir' and 'config_acs' are specified for a particular device, configuration specified through 'config_acs' takes precedence over the other.

Signed-off-by Vidya Sagar <[email protected]>

---
v3:
* Fixed a documentation issue reported by kernel test bot

v2:
* Refactored the code as per Jason's suggestion

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

Quoted 3 lines · Vidya Sagar

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration.

An example wouldn't hurt, here and in kernel-parameters.txt.

Quoted 7 lines

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

I'm wondering whether it would make more sense to let users choose between those "higher-level" options, instead of giving direct access to bits (and thus risking users to choose an incorrect setting).

Also, would it be possible to automatically change ACS settings when enabling or disabling P2PDMA?

The representation chosen here (as a command line option) seems questionable:

We're going to add more user-controllable options going forward. E.g., when introducing IDE, we'll have to let user space choose whether encryption should be enabled for certain PCIe devices. That's because encryption isn't for free, so can't be enabled opportunistically. (The number of crypto engines on a CPU is limited and enabling encryption consumes energy.)

What about exposing such user configurable settings with sysctl?

Quoted 3 lines · Vidya Sagar

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration.

An example wouldn't hurt, here and in kernel-parameters.txt.

Quoted 7 lines

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

I'm wondering whether it would make more sense to let users choose between those "higher-level" options, instead of giving direct access to bits (and thus risking users to choose an incorrect setting).

From: Vidya Sagar <[email protected]>
Sent: Tuesday, June 25, 2024 11:32 PM

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

Quoted 10 lines

From: Vidya Sagar <[email protected]>
Sent: Tuesday, June 25, 2024 11:32 PM

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

It'll be helpful to also call out the impact of losing other features (e.g. PASID) with less isolation:

pci_enable_pasid()

{
	...
	if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
		return -EINVAL;
	...
}

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options (ex:- disable_acs_redir) allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

Signed-off-by Vidya Sagar <[email protected]>

---
v4:
* Changed commit message (Courtesy: Jason) to provide more details

v3:
* Fixed a documentation issue reported by kernel test bot

v2:
* Refactored the code as per Jason's suggestion

 .../admin-guide/kernel-parameters.txt         |  22 +++
 drivers/pci/pci.c                             | 148 +++++++++++-------
 2 files changed, 112 insertions(+), 58 deletions(-)

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options (ex:- disable_acs_redir) allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

Signed-off-by Vidya Sagar <[email protected]>

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options (ex:- disable_acs_redir) allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

"Error starting domain: internal error: QEMU unexpectedly closed the monitor (vm=‘win10’): qxl_send_events: spice-server bug: guest stopped, ignoring

2024-08-08T01:45:51.824474Z qemu-system-x86_64: -device {“driver”:“vfio-pci”,“host”:“0000:0b:00.0”,“id”:“hostdev0”,“bus”:“pci.2”,“addr”:“0x0”}: vfio 0000:0b:00.0: group 83 is not viable

Please ensure all devices within the iommu_group are bound to their vfio bus driver."

The good kernel 6.4.0-150600.23.14 log has this:

[    0.618918] pci 0000:00:1c.0: Intel PCH root port ACS workaround enabled
[    0.619153] pci 0000:00:1c.4: Intel PCH root port ACS workaround enabled

the bad one 6.4.0-150600.23.22 does not.

Signed-off-by Vidya Sagar <[email protected]>

---
v4:
* Changed commit message (Courtesy: Jason) to provide more details

v3:
* Fixed a documentation issue reported by kernel test bot

v2:
* Refactored the code as per Jason's suggestion

  .../admin-guide/kernel-parameters.txt         |  22 +++
  drivers/pci/pci.c                             | 148 +++++++++++-------
  2 files changed, 112 insertions(+), 58 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +22 -0

index 500cfa776225..42d0f6fd40d0 100644
@@ -4619,6 +4619,28 @@
  				bridges without forcing it upstream. Note:
  				this removes isolation between devices and
  				may put more devices in an IOMMU group.
+		config_acs=
+				Format:
+				=<ACS flags>@<pci_dev>[; ...]
+				Specify one or more PCI devices (in the format
+				specified above) optionally prepended with flags
+				and separated by semicolons. The respective
+				capabilities will be enabled, disabled or unchanged
+				based on what is specified in flags.
+				ACS Flags is defined as follows
+				bit-0 : ACS Source Validation
+				bit-1 : ACS Translation Blocking
+				bit-2 : ACS P2P Request Redirect
+				bit-3 : ACS P2P Completion Redirect
+				bit-4 : ACS Upstream Forwarding
+				bit-5 : ACS P2P Egress Control
+				bit-6 : ACS Direct Translated P2P
+				Each bit can be marked as
+				‘0‘ – force disabled
+				‘1’ – force enabled
+				‘x’ – unchanged.
+				Note: this may remove isolation between devices
+				and may put more devices in an IOMMU group.
  		force_floating	[S390] Force usage of floating interrupts.
  		nomio		[S390] Do not use MIO instructions.
  		norid		[S390] ignore the RID field and force use of

drivers/pci/pci.c +90 -58

index 55078c70a05b..6661932afe59 100644
@@ -946,30 +946,67 @@ void pci_request_acs(void)
  }
 
  static const char *disable_acs_redir_param;
+static const char *config_acs_param;
 
-/**
- * pci_disable_acs_redir - disable ACS redirect capabilities
- * @dev: the PCI device
- *
- * For only devices specified in the disable_acs_redir parameter.
- */
-static void pci_disable_acs_redir(struct pci_dev *dev)
+struct pci_acs {
+	u16 cap;
+	u16 ctrl;
+	u16 fw_ctrl;
+};
+
+static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
+			     const char *p, u16 mask, u16 flags)
  {
+	char *delimit;
  	int ret = 0;
-	const char *p;
-	int pos;
-	u16 ctrl;
 
-	if (!disable_acs_redir_param)
+	if (!p)
  		return;
 
-	p = disable_acs_redir_param;
  	while (*p) {
+		if (!mask) {
+			/* Check for ACS flags */
+			delimit = strstr(p, "@");
+			if (delimit) {
+				int end;
+				u32 shift = 0;
+
+				end = delimit - p - 1;
+
+				while (end > -1) {
+					if (*(p + end) == '0') {
+						mask |= 1 << shift;
+						shift++;
+						end--;
+					} else if (*(p + end) == '1') {
+						mask |= 1 << shift;
+						flags |= 1 << shift;
+						shift++;
+						end--;
+					} else if ((*(p + end) == 'x') || (*(p + end) == 'X')) {
+						shift++;
+						end--;
+					} else {
+						pci_err(dev, "Invalid ACS flags... Ignoring\n");
+						return;
+					}
+				}
+				p = delimit + 1;
+			} else {
+				pci_err(dev, "ACS Flags missing\n");
+				return;
+			}
+		}
+
+		if (mask & ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | PCI_ACS_CR |
+			    PCI_ACS_UF | PCI_ACS_EC | PCI_ACS_DT)) {
+			pci_err(dev, "Invalid ACS flags specified\n");
+			return;
+		}
+
  		ret = pci_dev_str_match(dev, p, &p);
  		if (ret < 0) {
-			pr_info_once("PCI: Can't parse disable_acs_redir parameter: %s\n",
-				     disable_acs_redir_param);
-
+			pr_info_once("PCI: Can't parse acs command line parameter\n");
  			break;
  		} else if (ret == 1) {
  			/* Found a match */

@@ -989,56 +1026,38 @@ static void pci_disable_acs_redir(struct pci_dev *dev)
  	if (!pci_dev_specific_disable_acs_redir(dev))
  		return;
 
-	pos = dev->acs_cap;
-	if (!pos) {
-		pci_warn(dev, "cannot disable ACS redirect for this hardware as it does not have ACS capabilities\n");
-		return;
-	}
-
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
+	pci_dbg(dev, "ACS mask  = 0x%X\n", mask);
+	pci_dbg(dev, "ACS flags = 0x%X\n", flags);
 
-	/* P2P Request & Completion Redirect */
-	ctrl &= ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC);
+	/* If mask is 0 then we copy the bit from the firmware setting. */
+	caps->ctrl = (caps->ctrl & ~mask) | (caps->fw_ctrl & mask);
+	caps->ctrl |= flags;
 
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
-
-	pci_info(dev, "disabled ACS redirect\n");
+	pci_info(dev, "Configured ACS to 0x%x\n", caps->ctrl);
  }
 
  /**
   * pci_std_enable_acs - enable ACS on devices using standard ACS capabilities
   * @dev: the PCI device
+ * @caps: default ACS controls
   */
-static void pci_std_enable_acs(struct pci_dev *dev)
+static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
  {
-	int pos;
-	u16 cap;
-	u16 ctrl;
-
-	pos = dev->acs_cap;
-	if (!pos)
-		return;
-
-	pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
-	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
-
  	/* Source Validation */
-	ctrl |= (cap & PCI_ACS_SV);
+	caps->ctrl |= (caps->cap & PCI_ACS_SV);
 
  	/* P2P Request Redirect */
-	ctrl |= (cap & PCI_ACS_RR);
+	caps->ctrl |= (caps->cap & PCI_ACS_RR);
 
  	/* P2P Completion Redirect */
-	ctrl |= (cap & PCI_ACS_CR);
+	caps->ctrl |= (caps->cap & PCI_ACS_CR);
 
  	/* Upstream Forwarding */
-	ctrl |= (cap & PCI_ACS_UF);
+	caps->ctrl |= (caps->cap & PCI_ACS_UF);
 
  	/* Enable Translation Blocking for external devices and noats */
  	if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
-		ctrl |= (cap & PCI_ACS_TB);
-
-	pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
+		caps->ctrl |= (caps->cap & PCI_ACS_TB);
  }
 
  /**

@@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct pci_dev *dev)
   */
  static void pci_enable_acs(struct pci_dev *dev)
  {
-	if (!pci_acs_enable)
-		goto disable_acs_redir;
+	struct pci_acs caps;
+	int pos;
+
+	pos = dev->acs_cap;
+	if (!pos)
+		return;
 
-	if (!pci_dev_specific_enable_acs(dev))
-		goto disable_acs_redir;
+	pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+	caps.fw_ctrl = caps.ctrl;
 
-	pci_std_enable_acs(dev);
+	/* If an iommu is present we start with kernel default caps */
+	if (pci_acs_enable) {
+		if (pci_dev_specific_enable_acs(dev))
+			pci_std_enable_acs(dev, &caps);
+	}
 
-disable_acs_redir:
  	/*
-	 * Note: pci_disable_acs_redir() must be called even if ACS was not
-	 * enabled by the kernel because it may have been enabled by
-	 * platform firmware.  So if we are told to disable it, we should
-	 * always disable it after setting the kernel's default
-	 * preferences.
+	 * Always apply caps from the command line, even if there is no iommu.
+	 * Trust that the admin has a reason to change the ACS settings.
  	 */
-	pci_disable_acs_redir(dev);
+	__pci_config_acs(dev, &caps, disable_acs_redir_param,
+			 PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC,
+			 ~(PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_EC));
+	__pci_config_acs(dev, &caps, config_acs_param, 0, 0);
+
+	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
  }
 
  /**

@@ -6740,6 +6769,8 @@ static int __init pci_setup(char *str)
  				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
  			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
  				disable_acs_redir_param = str + 18;
+			} else if (!strncmp(str, "config_acs=", 11)) {
+				config_acs_param = str + 11;
  			} else {
  				pr_err("PCI: Unknown option `%s'\n", str);
  			}

@@ -6764,6 +6795,7 @@ static int __init pci_realloc_setup_params(void)
  	resource_alignment_param = kstrdup(resource_alignment_param,
  					   GFP_KERNEL);
  	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
+	config_acs_param = kstrdup(config_acs_param, GFP_KERNEL);
 
  	return 0;
  }

Quoted 13 lines

drivers/pci/pci.c +6 -2

@@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct pci_dev 
 *dev)
    */
   static void pci_enable_acs(struct pci_dev *dev)
   {
-    if (!pci_acs_enable)
-        goto disable_acs_redir;
+    struct pci_acs caps;
+    int pos;
+
+    pos = dev->acs_cap;
+    if (!pos)
+        return;

drivers/pci/pci.c static void pci_std_enable_acs(struct pci_dev *dev) · line 1081

Quoted 8 lines

drivers/pci/pci.c +5 -3

-    if (!pci_dev_specific_enable_acs(dev))
-        goto disable_acs_redir;
+    pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+    pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+    caps.fw_ctrl = caps.ctrl;
-    pci_std_enable_acs(dev);
+    /* If an iommu is present we start with kernel default caps */
+    if (pci_acs_enable) {

drivers/pci/pci.c static void pci_std_enable_acs(struct pci_dev *dev) · line 1083

Quoted 2 lines

drivers/pci/pci.c +2 -0

+        if (pci_dev_specific_enable_acs(dev))
+            pci_std_enable_acs(dev, &caps);

Review static void pci_std_enable_acs(struct pci_dev · line 1083

Quoted 1 line

(quoted diff) +1 -0

+    }

Quoted 5 lines · Jiri Slaby

Quoted 5 lines

drivers/pci/pci.c +0 -0

@@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct pci_dev 
 *dev)
    */
   static void pci_enable_acs(struct pci_dev *dev)
   {

Quoted 8 lines

drivers/pci/pci.c static void pci_std_enable_acs(struct pci_dev *dev) · line 1074

Quoted 8 lines

drivers/pci/pci.c +6 -2

-    if (!pci_acs_enable)
-        goto disable_acs_redir;
+    struct pci_acs caps;
+    int pos;
+
+    pos = dev->acs_cap;
+    if (!pos)
+        return;

Quoted 8 lines

drivers/pci/pci.c static void pci_std_enable_acs(struct pci_dev *dev) · line 1081

Quoted 8 lines

drivers/pci/pci.c +5 -3

-    if (!pci_dev_specific_enable_acs(dev))
-        goto disable_acs_redir;
+    pci_read_config_word(dev, pos + PCI_ACS_CAP, &caps.cap);
+    pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
+    caps.fw_ctrl = caps.ctrl;
-    pci_std_enable_acs(dev);
+    /* If an iommu is present we start with kernel default caps */
+    if (pci_acs_enable) {

AFAIU pci_acs_enable is set from iommus' code via pci_request_acs(). Which is much later than when bridges are initialized here, right?

Quoted 14 lines · Jiri Slaby

Quoted 5 lines · Jiri Slaby

Quoted 5 lines

(quoted diff) +0 -0

@@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct
 pci_dev *dev)
    */
   static void pci_enable_acs(struct pci_dev *dev)
   {

Quoted 8 lines

Review static void pci_std_enable_acs(struct · line 1075

Quoted 8 lines

(quoted diff) +6 -2

-    if (!pci_acs_enable)
-        goto disable_acs_redir;
+    struct pci_acs caps;
+    int pos;
+
+    pos = dev->acs_cap;
+    if (!pos)
+        return;

Ignore the previous post.

The bridge has no ACS (see lspci below). So it used to be enabled by pci_quirk_enable_intel_pch_acs() by another registers.

I am not sure how to fix this as we cannot have "caps" from these quirks, so that whole idea of __pci_config_acs() is nonworking for these quirks.

Quoted 18 lines · Jiri Slaby

Quoted 14 lines · Jiri Slaby

Quoted 5 lines · Jiri Slaby

Quoted 5 lines

drivers/pci/pci.c +0 -0

@@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct
 pci_dev *dev)
    */
   static void pci_enable_acs(struct pci_dev *dev)
   {

Quoted 8 lines

Review static void pci_std_enable_acs(struct · line 1075

Quoted 8 lines

(quoted diff) +6 -2

-    if (!pci_acs_enable)
-        goto disable_acs_redir;
+    struct pci_acs caps;
+    int pos;
+
+    pos = dev->acs_cap;
+    if (!pos)
+        return;

Ignore the previous post.

Quoted 3 lines

The bridge has no ACS (see lspci below). So it used to be enabled by
pci_quirk_enable_intel_pch_acs() by another registers.

Er, Ok, so it overrides the whole thing with
pci_dev_specific_acs_enabled() too..

Quoted 4 lines

I am not sure how to fix this as we cannot have "caps" from these quirks, so
that whole idea of __pci_config_acs() is nonworking for these quirks.

We just need to allow the quirk to run before we try to do anything with the cap, which has probably always been a NOP anyhow.

Maybe like this?

drivers/pci/pci.c +1 -0

index 7d85c04fbba2ae..225a6cd2e9ca3b 100644
@@ -1067,8 +1067,15 @@ static void pci_std_enable_acs(struct pci_dev
 *dev, struct pci_acs *caps)
  static void pci_enable_acs(struct pci_dev *dev)
  {
  	struct pci_acs caps;
+	bool enable_acs = false;
  	int pos;

drivers/pci/pci.c +1 -0

+	/* If an iommu is present we start with kernel default caps

*/

drivers/pci/pci.c +5 -0

+	if (pci_acs_enable) {
+		if (pci_dev_specific_enable_acs(dev))
+			enable_acs = true;
+	}
+

  pos = dev->acs_cap;
  if (!pos)
  return;

drivers/pci/pci.c +0 -0

@@ -1077,11 +1084,8 @@ static void pci_enable_acs(struct pci_dev
 *dev)
  	pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
  	caps.fw_ctrl = caps.ctrl;

drivers/pci/pci.c +0 -1

-	/* If an iommu is present we start with kernel default caps

*/

drivers/pci/pci.c +2 -4

-	if (pci_acs_enable) {
-		if (pci_dev_specific_enable_acs(dev))
-			pci_std_enable_acs(dev, &caps);
-	}
+	if (enable_acs)
+		pci_std_enable_acs(dev, &caps);

  /*
  * Always apply caps from the command line, even if there is no iommu.

Quoted 38 lines · Vidya Sagar

PCIe ACS settings control the level of isolation and the possible P2P paths between devices. With greater isolation the kernel will create smaller iommu_groups and with less isolation there is more HW that can achieve P2P transfers. From a virtualization perspective all devices in the same iommu_group must be assigned to the same VM as they lack security isolation.

There is no way for the kernel to automatically know the correct ACS settings for any given system and workload. Existing command line options (ex:- disable_acs_redir) allow only for large scale change, disabling all isolation, but this is not sufficient for more complex cases.

Add a kernel command-line option 'config_acs' to directly control all the ACS bits for specific devices, which allows the operator to setup the right level of isolation to achieve the desired P2P configuration. The definition is future proof, when new ACS bits are added to the spec the open syntax can be extended.

ACS needs to be setup early in the kernel boot as the ACS settings effect how iommu_groups are formed. iommu_group formation is a one time event during initial device discovery, changing ACS bits after kernel boot can result in an inaccurate view of the iommu_groups compared to the current isolation configuration.

ACS applies to PCIe Downstream Ports and multi-function devices. The default ACS settings are strict and deny any direct traffic between two functions. This results in the smallest iommu_group the HW can support. Frequently these values result in slow or non-working P2PDMA.

ACS offers a range of security choices controlling how traffic is allowed to go directly between two devices. Some popular choices:
- Full prevention
- Translated requests can be direct, with various options
- Asymmetric direct traffic, A can reach B but not the reverse
- All traffic can be direct
Along with some other less common ones for special topologies.

Quoted 3 lines

The intention is that this option would be used with expert knowledge of the HW capability and workload to achieve the desired configuration.

Hi,

this breaks ACS on some platforms (in 6.11). See:
https://bugzilla.suse.com/show_bug.cgi?id=1229019