Vmd pch rootbus by sdurawa · Pull Request #3 · sdurawa/linux

sdurawa · 2024-10-21T16:04:46Z

VMD PCH rootbus implementation

mtkaczyk

patch 1 review
for title I suggest to remove reference to Intel CPU code number (not needed here):

PCI: vmd: Clean up vmd_enable_domain function
This function is too long and needs to be shortened to make it more readable.

This clean up is a prework for enablement additional VMD bus range. It doesn't change
Functional behavior of vmd_enable_domain().

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

typo beahvior

mtkaczyk · 2024-10-22T09:23:36Z

+	VMD_RESOURCE_COUNT
+};
+
+enum vmd_rootbus { VMD_ROOTBUS_0 = 0, VMD_ROOTBUS_COUNT };


I've never seen something like that before, probably visual formatting.

mtkaczyk · 2024-10-22T09:25:34Z

 {
-	u16 bus, max_buses = resource_size(&vmd->resources[0]);
+	u16 bus,
+		max_buses = resource_size(&vmd->resources[VMD_RESOURCE_CFGBAR]);


not acceptable:

Suggested change

max_buses = resource_size(&vmd->resources[VMD_RESOURCE_CFGBAR]);

u16 bus;

u16 max_buses = resource_size(&vmd->resources[VMD_RESOURCE_CFGBAR]);

mtkaczyk · 2024-10-22T09:30:03Z

-	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
-	vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[2];
+	vmd->dev->resource[VMD_MEMBAR1].child =
+		&vmd->resources[VMD_RESOURCE_MEMBAR_1];


For sure, it is badly formatted.

Suggested change

&vmd->resources[VMD_RESOURCE_MEMBAR_1];

struct x *vmd_membar1_res=&vmd->dev->resource[VMD_MEMBAR1];

struct x *vmd_membar2_res =&vmd->dev->resource[VMD_MEMBAR2];

vmd_membar1_res->child =&vmd->dev->resource[VMD_MEMBAR1];

vmd_membar2_res->child =&vmd->dev->resource[VMD_MEMBAR2];

mtkaczyk · 2024-10-22T09:31:02Z

-static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
+static void vmd_configure_cfgbar(struct vmd_dev *vmd)
+{
+	struct resource *res;


Suggested change

struct resource *res;

struct resource *res = &vmd->dev->resource[VMD_CFGBAR];

mtkaczyk · 2024-10-22T09:32:33Z

+ * @membar_number: type of the MemBAR
+ * @start_offset: 4K aligned offset applied to start of VMD’s MEMBAR MMIO space
+ * @end_offset: 4K aligned offset applied to end of VMD’s MEMBAR MMIO space
+ * @parent: parent resource assigned to resource to be filled in


Highlight that it can be NULL.

I mean @parent lol :)

mtkaczyk · 2024-10-22T09:33:24Z

+ * to MMIO address assigned by the OS or BIOS.
+ * @vmd: the VMD device
+ * @resource_number: resource buffer number to be filled in
+ * @membar_number: type of the MemBAR


you say type but the name has number

mtkaczyk · 2024-10-22T09:34:39Z

+	snprintf(name, sizeof(name), "VMD MEMBAR%d", membar_number/2);
+
+	if (!parent)
+		parent = res;


generally overwriting income parameter is bad practice, maybe better. use inline if?

.parent = parent ? parent : res;

mtkaczyk · 2024-10-22T09:36:31Z

-	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
+	pci_add_resource(&resources, &vmd->resources[VMD_RESOURCE_CFGBAR]);
+	pci_add_resource_offset(
+		&resources, &vmd->resources[VMD_RESOURCE_MEMBAR_1], offset[0]);


looks like bad formatting to me, please look into kernel, how they solve this :)

mtkaczyk · 2024-10-22T09:36:52Z

-	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
-				       &vmd_ops, sd, &resources);
-	if (!vmd->bus) {
+	vmd->bus[VMD_ROOTBUS_0] = pci_create_root_bus(


bad formatting.

mtkaczyk · 2024-10-22T09:42:11Z

 	char __iomem *base;

+	if (vmd_has_pch_rootbus(vmd))
+		max_buses += resource_size(


bad formating

mtkaczyk · 2024-10-22T09:42:51Z

+		case 3:
+			if (!(features & VMD_FEAT_HAS_PCH_ROOTBUS)) {
+				pci_err(dev,
+					"VMD Bus Restriction detected type %d, but PCH Rootbus is not supported, aborting.\n",


You can move it up to previous line, lgtm anyway

mtkaczyk · 2024-10-22T09:44:48Z

+		 * PCI_REG_BUSRANGE0 register.
+		 */
+		vmd->resources[VMD_RESOURCE_CFGBAR].start =
+			VMD_ROOTBUS_RANGE_START(ioc_bus_range);


I'm not sure if this is correct in kernel coding style :(

mtkaczyk · 2024-10-22T09:46:21Z

+				     &vmd->resources[VMD_RESOURCE_MEMBAR_1]);
+		vmd_configure_membar(vmd, VMD_RESOURCE_PCH_MEMBAR_2,
+				     VMD_MEMBAR2,
+				     membar2_offset + pch_membar2_offset, 0,


consider shorter naming for variables.

mtkaczyk · 2024-10-22T09:48:56Z

+	 * which is stored in vmd->busn_start[VMD_ROOTBUS_1].
+	 */
+	bus_number = (bus->number == VMD_PRIMARY_PCH_BUS) ?
+			     vmd->busn_start[VMD_ROOTBUS_1] :


This function is too long and needs to be shortened to make it more readable. Certain VMD devices like Intel Arrow Lake has two bus ranges, current implementation supports only one range. This clean up is a prework for enablement additional VMD bus range. It doesn't change functional behavior of vmd_enable_domain(). Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

Starting from Intel Arrow Lake VMD enhacement introduces separate rotbus for PCH. It means that all 3 MMIO BARs exposed by VMD are shared now between CPU IOC and PCH. This patch adds PCH bus enumeration and MMIO management for devices with VMD enhancement support. Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

VMD PCH rootbus primary number is 0x80 and pci_scan_bridge_extend() cannot assign it as "hard-wired to 0" and marks setup as broken. To avoid this, PCH bus number has to be the same as PCH primary number. Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

mtkaczyk · 2024-10-22T15:57:03Z

+ * @membar_number: type of the MemBAR
+ * @start_offset: 4K aligned offset applied to start of VMD’s MEMBAR MMIO space
+ * @end_offset: 4K aligned offset applied to end of VMD’s MEMBAR MMIO space
+ * @parent: parent resource assigned to resource to be filled in


I mean @parent lol :)

mtkaczyk · 2024-10-23T06:07:05Z

+	pci_add_resource_offset(&resources_pch,
+				&vmd->resources[VMD_RES_PCH_MEMBAR_2], offset[1]);
+
+	vmd->bus[VMD_ROOTBUS_1] = pci_create_root_bus(


this formatting is bad.

sdurawa force-pushed the vmd_pch_rootbus branch from e6e2d3d to 893f31f Compare October 22, 2024 08:13

mtkaczyk reviewed Oct 22, 2024

View reviewed changes

sdurawa force-pushed the vmd_pch_rootbus branch from 893f31f to c4ede08 Compare October 22, 2024 14:33

Szymon Durawa added 3 commits October 22, 2024 17:31

mtkaczyk reviewed Oct 23, 2024

View reviewed changes

	max_buses = resource_size(&vmd->resources[VMD_RESOURCE_CFGBAR]);
	u16 bus;
	u16 max_buses = resource_size(&vmd->resources[VMD_RESOURCE_CFGBAR]);

-		&vmd->resources[VMD_RESOURCE_MEMBAR_1];
+struct x *vmd_membar1_res=&vmd->dev->resource[VMD_MEMBAR1];
+struct x *vmd_membar2_res =&vmd->dev->resource[VMD_MEMBAR2];
+vmd_membar1_res->child =&vmd->dev->resource[VMD_MEMBAR1];
+vmd_membar2_res->child =&vmd->dev->resource[VMD_MEMBAR2];

	struct resource *res;
	struct resource *res = &vmd->dev->resource[VMD_CFGBAR];

Conversation

sdurawa commented Oct 21, 2024

Uh oh!

mtkaczyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants