PCI: vmd: add PCH rootbus #2

sdurawa · 2024-10-08T13:44:05Z

Introducing 2 commits to support PCH rootbus for Intel VMD enhancements.

This function is too long and needs to be shortened to make it more readable. Certain VMD devices like 0xAD0B has two bus ranges, current implementation supports only one range. This clean up is a prework for enablement additional VMD bus range. Signed-off-by: Szymon Durawa <[email protected]> Suggested-by: Nirmal Patel <[email protected]>

Starting from 0xAD0B VMD enhacement introduces separate rootbus 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. Signed-off-by: Szymon Durawa <[email protected]> Suggested-by: Nirmal Patel <[email protected]>

mtkaczyk · 2024-10-10T06:28:15Z

drivers/pci/controller/vmd.c

 	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
+
+	/*
+	 * Some client VMD devices have their VMD rootports connected to both


Nie wspominaj tutaj, że mówimy tu o "client" ponieważ nikogo to nie obchodzi. Część ludzi nie ogarnie nawet co masz na myśli.
To co jest ważne to fakty. a fakt jest taki:

"Starting from Intel Lunar Lake, VMD driver might be connected to PCH rootbus" (jeżeli dobrze pamiętam, to Lunar lake).

mtkaczyk · 2024-10-10T06:34:13Z

drivers/pci/controller/vmd.c

+	 * PCI_VMD_PRIMARY_PCH_BUS but original value is 0xE1 which is stored
+	 * in vmd->busn_start_pch.
+	 */
+	if (vmd_has_pch_rootbus(vmd) && bus->number == PCI_VMD_PRIMARY_PCH_BUS)


wydaje mi się, że te warunki są jednoznaczne w tym miejscu i możesz sprawdzić tylko jeden z nich.

mtkaczyk · 2024-10-10T06:37:37Z

drivers/pci/controller/vmd.c

+	if (vmd_has_pch_rootbus(vmd) && bus->number == PCI_VMD_PRIMARY_PCH_BUS)
+		busnr_ecam = vmd->busn_start_pch - vmd->busn_start;
+	else
+		busnr_ecam = bus->number - vmd->busn_start;


jeżeli zachodzi koniecznośc takiego WA zdecydowanie czytelniejsze, będzie, jeżeli stworzysz pole (jeżeli dobrze rozumiem) vmd->busn_offset[2] i będziesz tam przypisywał wartość:

vmd->busn_offset[0] = bus->number; vmd->busn_offset[1] = 0xE1;

Chodzi mi o to że nazywanie pól pod pch ogranicza rozszerzalność tego rozwiązania (jeżeli ktoś za parę lat doda inny pcie bus ciężko będzie utrzymać odpowiednie nazewnictwo).

mtkaczyk · 2024-10-10T06:41:47Z

drivers/pci/controller/vmd.c

+	struct resource		resources[6];
 	struct irq_domain	*irq_domain;
 	struct pci_bus		*bus;
+	struct pci_bus		*bus_pch;


nie da rady zmergować busów w tablice?

dodac define na ilosc busow

mtkaczyk · 2024-10-10T06:51:35Z

drivers/pci/controller/vmd.c

+		};
+
+		/*
+		 * Update first cfgbar range (IOC) with value stored


nie musisz tłumaczyć kodu, każdy ogarnięty developer widzi co robisz. To co nas interesuje to dlaczego jest taka potrzeba. Najelpszy byłby link do specyfikacji jeżeli jest to PCIe spek. Pewnie nie jest.

mtkaczyk · 2024-10-10T06:55:27Z

drivers/pci/controller/vmd.c

+ *
+ * The only way we could use a 64-bit non-prefetchable MEMBAR is
+ * if its address is <4GB so that we can convert it to a 32-bit
+ * resource.  To be visible to the host OS, all VMD endpoints must


Double space.

mtkaczyk · 2024-10-10T06:56:20Z

drivers/pci/controller/vmd.c

+}
+
+/*
+ * Align resource information the same as for vmd_configure_membar1


Ten komentarz nic nie wnosi.

mtkaczyk · 2024-10-10T06:59:08Z

drivers/pci/controller/vmd.c

+ * Align resource information the same as for vmd_configure_membar1
+ * function.
+ */
+static void vmd_configure_membar2(struct vmd_dev *vmd,


te dwie funkcje vmd_configure_membar1 i vmd_configure_membar2 wydaje mi się, że można połączyć w jedną ponieważ kod jest niemal indentyczny (poza resorces, które updatujesz) oczywiście są one sobie odpowiadające więc jesteś w stanie odnosić się po indexach.

no i wtedy możesz ładnie opisać co się znajduje w poszczególnych resources (jeżeli oczywiscie merge tych funkcji jest możliwy).

mtkaczyk · 2024-10-10T07:00:15Z

drivers/pci/controller/vmd.c

+	struct resource *res;
+	u32 upper_bits, reg;
+	unsigned long flags;
+	u64 vmd_enh_membar2_offset = 0;


podobnie jak wyżej, proszę zmień vmd_membar2_pch_offset

mtkaczyk · 2024-10-10T07:11:29Z

drivers/pci/controller/vmd.c

+	}
+
+	/*
+	 * VMD enhacement specific: pci_scan_bridge_extend() in probe.c


skróciłbym to trochę, i na razie podkreślił że to jest te miejsce przez które robimy RFC

This is a workaround for pci_scan_bridge_extend() code. It assigns setup as broken when primary != bus->number. For PCH rootbus primary is not "hard-wired to 0". To avoid this, vmd->bus_pch->number and vmd->bus_pch->primary are updated to same value.

sdurawa · 2024-10-10T08:29:07Z

drivers/pci/controller/vmd.c

 #define VMCONFIG_MSI_REMAP	0x2
 #define PCI_REG_VMLOCK		0x70
 #define MB2_SHADOW_EN(vmlock)	(vmlock & 0x2)



VMD enhacement nie zmienia prezentacji urzadzen

Starting from 0xAD0B VMD enhacement introduces separate rootbus 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. Signed-off-by: Szymon Durawa <[email protected]> Suggested-by: Nirmal Patel <[email protected]>

This function is too long and needs to be shortened to make it more readable. Certain VMD devices like 0xAD0B has two bus ranges, current implementation supports only one range. This clean up is a prework for enablement additional VMD bus range. Signed-off-by: Szymon Durawa <[email protected]> Suggested-by: Nirmal Patel <[email protected]>

…tbus 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. Signed-off-by: Szymon Durawa <[email protected]> Suggested-by: Nirmal Patel <[email protected]>

mtkaczyk · 2024-10-17T06:33:15Z

drivers/pci/controller/vmd.c

 #define VMD_MEMBAR1	2
 #define VMD_MEMBAR2	4

+#define VMD_ROOTBUS_SIZE 2


imo VMD_ROOTBUS_COUNT

mtkaczyk · 2024-10-17T06:38:27Z

drivers/pci/controller/vmd.c

 	struct irq_domain	*irq_domain;
-	struct pci_bus		*bus;
-	u8			busn_start;
+	struct pci_bus		*bus[VMD_ROOTBUS_SIZE];


you can eventually consider to group it into struct but it is fine for me.

mtkaczyk · 2024-10-17T06:42:53Z

drivers/pci/controller/vmd.c

+};
+
+enum vmd_rootbus {
+	VMD_ROOTBUS0,


imo VMD_ROOTBUS_0, but is is fine anyway

mtkaczyk · 2024-10-17T06:45:23Z

drivers/pci/controller/vmd.c

+	 * PCI_VMD_PRIMARY_PCH_BUS but original value is 0xE1 which is stored
+	 * in vmd->busn_start[VMD_ROOTBUS1].
+	 */
+	if (vmd_has_pch_rootbus(vmd) && bus->number == PCI_VMD_PRIMARY_PCH_BUS)


you can detect it by:
if (&(vmd->bus[VMD_ROOTBUS1]) == bus)

and I think it is better readable, because you are not reaying on hack but on adress.
Eventually, you can consider passing enum vmd_rootbus instead of bus, and deference it here.

If you passing struct vmd_dev *vmd and passing struct pci_bus *bus placed inside vmd the struct is not mandatory because you can obtain it easy.

Condition is rewritten using ? operand.

mtkaczyk · 2024-10-17T06:49:29Z

drivers/pci/controller/vmd.c

+	unsigned char bus_number = 0;
+
+	/*
+	 * VMD enhacement specific: for PCH rootbus, bus number is set to


Please remove VMD enhancement specyfic. It has nothing to do with VMD and it is not specific, it is a hack for kernel code elsewhere.

Please add reference to vmd_create_pch_bus. Simple

See comment in vmd_create_pch_bus().

Should be fine.

drivers/pci/controller/vmd.c

mtkaczyk · 2024-10-17T06:59:47Z

drivers/pci/controller/vmd.c

+				vmd->busn_start[VMD_ROOTBUS1] = 225;
+			} else {
+				pci_err(dev,
+					"Unknown Bus Offset Setting (%d)\n",


"Unknown Bus Offset Setting (%d)\n" is not really helpful.

What about:
Determined 3 vmd configs but VMD in PCH bus is not supported, aborting.

Probably you can find better wording, I didn't check it.

mtkaczyk · 2024-10-17T07:15:26Z

drivers/pci/controller/vmd.c

+	}
+}
+
+static void vmd_configure_membar(struct vmd_dev *vmd,


Description upon the function would be recommended and helpful.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

Description added.

mtkaczyk · 2024-10-17T07:17:14Z

drivers/pci/controller/vmd.c

+		 */
+		vmd_configure_membar(vmd, VMD_MEMBAR1, VMD_RESOURCE_MEMBAR1, 0,
+				     pch_membar1_offset, NULL);
+		vmd_configure_membar(vmd, VMD_MEMBAR2, VMD_RESOURCE_MEMBAR2,


What I don' like here is repeated initialization please consider reverting order. or use if/else

mtkaczyk · 2024-10-17T07:20:38Z

drivers/pci/controller/vmd.c

+				pci_err(dev,
+					"Unknown Bus Offset Setting (%d)\n",
+					BUS_RESTRICT_CFG(reg));
+				return -ENODEV;


Please consider other error codes (in each case now -ENODEV is returned from various functions)

mtkaczyk

Please update module version
Please update patches author (currently it is sdurawa)

mtkaczyk · 2024-10-18T08:17:25Z

drivers/pci/controller/vmd.c

 #define MB2_SHADOW_EN(vmlock)	(vmlock & 0x2)

+#define PCI_VMD_PRIMARY_PCH_BUS 0x80
+#define PCI_REG_BUSRANGE0 0xC8


Probably we should make all defines starting with PCI_VMD or just VMD. We should avoid name confusion with different components.

If you would like to stay with PCI_* then it is not right place for those defines because they are probably global and should be placed in include/linux/pci_regs.h - we don't want that.

mtkaczyk · 2024-10-18T08:20:38Z

drivers/pci/controller/vmd.c

 #define MB2_SHADOW_SIZE		16

+enum vmd_resource {
+	VMD_RESOURCE_CFGBAR,


VMD_RESOURCE_CFGBAR = 0 to force initialization from 0

Suggested change

VMD_RESOURCE_CFGBAR,

VMD_RESOURCE_CFGBAR = 0,

mtkaczyk · 2024-10-18T08:31:26Z

drivers/pci/controller/vmd.c

 				 VMD_FEAT_HAS_BUS_RESTRICTIONS |	\
 				 VMD_FEAT_OFFSET_FIRST_VECTOR |		\
-				 VMD_FEAT_BIOS_PM_QUIRK)
+				 VMD_FEAT_BIOS_PM_QUIRK |	\


Looks like to alligned same way as above but it could be presentation issue:

Suggested change

VMD_FEAT_BIOS_PM_QUIRK | \

VMD_FEAT_BIOS_PM_QUIRK | \

mtkaczyk · 2024-10-18T08:31:53Z

drivers/pci/controller/vmd.c

+};
+
+enum vmd_rootbus {
+	VMD_ROOTBUS_0,


Suggested change

VMD_ROOTBUS_0,

VMD_ROOTBUS_0 = 0,

mtkaczyk · 2024-10-18T08:37:04Z

drivers/pci/controller/vmd.c

-	struct pci_bus		*bus;
-	u8			busn_start;
+	struct pci_bus		*bus[VMD_ROOTBUS_COUNT];
+	u8			busn_start[VMD_ROOTBUS_COUNT];


It is asking to be placed in structure.

struct vmd_pci_bus { struct pci_bus *bus; u8 busn_start;

Suggested change

u8 busn_start[VMD_ROOTBUS_COUNT];

struct vmd_pci_bus vmd_bus[VMD_ROOTBUS_COUNT];

but I think it is fine either way

No changes here.

mtkaczyk · 2024-10-18T08:47:12Z

drivers/pci/controller/vmd.c

-	pci_scan_child_bus(vmd->bus);
-	vmd_domain_reset(vmd);
+	WARN(sysfs_create_link(&vmd->dev->dev.kobj,
+			       &vmd->bus[VMD_ROOTBUS_0]->dev.kobj, "domain"),


Shouldn't we create domain link for pch attached ROOTBUS_1?

No, there is only required for VMD domain. When creating sys link for ROOTBUS 1 there is an error "cannot create duplicate filename '/devices/pci0000:00/0000:00:0e.0/domain'".

mtkaczyk · 2024-10-18T11:10:09Z

drivers/pci/controller/vmd.c

+				vmd->busn_start[VMD_ROOTBUS_1] = 225;
+			} else {
+				pci_err(dev,
+					"VMD Bus Restriction detected type %d,",


Suggested change

"VMD Bus Restriction detected type %d,",

pci_err(dev, "VMD Bus Restriction detected type %d, but PCH Rootbus is not supported, aborting.",

BUS_RESTRICT_CFG(reg));

This messages cannot be divided because kernel is not guarantying that they will be flushed together, one by one.

mtkaczyk · 2024-10-18T12:17:32Z

drivers/pci/controller/vmd.c

+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+
+	snprintf(name, sizeof(name), "VMD MEMBAR%d", membar_number/2);


strncat should be avoided :)

Suggested change

snprintf(name, sizeof(name), "VMD MEMBAR%d", membar_number/2);

snprintf(name, sizeof(name), "VMD MEMBAR%d %s", membar_number / 2,

resource_number > VMD_RESOURCE_MEMBAR_2 : "PCH", "");

Fixed with resource_number > VMD_RESOURCE_MEMBAR_2 ? "PCH":""

mtkaczyk · 2024-10-18T12:20:50Z

drivers/pci/controller/vmd.c

+ */
+static void vmd_configure_membar(struct vmd_dev *vmd,
+				 enum vmd_resource resource_number,
+				 u8 membar_number, resource_size_t start_offset,


Suggested change

u8 membar_number, resource_size_t start_offset,

enum vmd_resource membar_number, resource_size_t start_offset,

membar number does not have an enum, it is a standalone define.

mtkaczyk · 2024-10-18T12:32:15Z

drivers/pci/controller/vmd.c

+
+	if (!vmd->bus[VMD_ROOTBUS_1]) {
+		pci_free_resource_list(&resources_pch);
+		vmd_remove_irq_domain(vmd);


Should we? We didn't register PCH MSI yet.

Shoudn't we call pci_stop_root_bus and pci_remove_root_bus?

correct, irq domain cannot be removed here, I have added stopping and removig PCH bus instead.

Szymon Durawa added 2 commits October 8, 2024 15:17

sdurawa marked this pull request as ready for review October 8, 2024 13:44

sdurawa changed the title ~~Vmd add pch rootbus~~ PCI: vmd: add PCH rootbus Oct 8, 2024

mtkaczyk reviewed Oct 10, 2024

View reviewed changes

sdurawa commented Oct 10, 2024

View reviewed changes

sdurawa added 3 commits October 15, 2024 13:44

mtkaczyk reviewed Oct 17, 2024

View reviewed changes

Source code modifications according to review remarks.

0955c01

mtkaczyk reviewed Oct 18, 2024

View reviewed changes

Replace PCI with VMD prefix for variables, align code indentation.

40a6497

	u8 busn_start[VMD_ROOTBUS_COUNT];
	struct vmd_pci_bus vmd_bus[VMD_ROOTBUS_COUNT];

	snprintf(name, sizeof(name), "VMD MEMBAR%d", membar_number/2);
	snprintf(name, sizeof(name), "VMD MEMBAR%d %s", membar_number / 2,
	resource_number > VMD_RESOURCE_MEMBAR_2 : "PCH", "");

	u8 membar_number, resource_size_t start_offset,
	enum vmd_resource membar_number, resource_size_t start_offset,

PCI: vmd: add PCH rootbus #2

Are you sure you want to change the base?

PCI: vmd: add PCH rootbus #2

Uh oh!

Conversation

sdurawa commented Oct 8, 2024

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!

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!

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!