On 16/06/26 07:46PM, Jonathan Cameron wrote:
On Tue, 9 Jun 2026 16:28:34 +0530
Shrihari E S <[email protected]> wrote:
From: Dongjoo Seo <[email protected]>
Implement the PCIe Streamlined Virtual Channel (SVC) Extended
Capability by adding support of capability, control and status
registers per PCIe 6.4 section 7.9.29. This capability is one
of the main requisites for UIO support in both PCIe and CXL ports.
Key changes include:
- New pcie_svc.c file for SVC capability management.
- Updated pcie_cap_fill_lnk() to handle flitmode signaling.
- Implement Lifecycle hooks (reset and config_write) to manage
SVC state.
Signed-off-by: Dongjoo Seo <[email protected]>
Signed-off-by: Shrihari E S <[email protected]>
A very quick review on this one as I'm more or less out of time for today
(and not listening to a call ;)
pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKCAP,
@@ -160,8 +161,14 @@ static void pcie_cap_fill_lnk(uint8_t *exp_cap,
PCIExpLinkWidth width,
}
if (flitmode) {
- pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA2,
+ uint32_t pos = dev->exp.exp_cap;
+
+ pci_word_test_and_set_mask(exp_cap + PCI_EXP_FLAGS,
PCI_EXP_LNKSTA2_FLIT);
Why is this writing a field from LNKSTA2 into FLAGS?
Hi Jonathan,
Sorry, it's a typo. It should be PCI_EXP_LNKSTA2 instead of PCI_EXP_FLAGS.
will correct it in the next version of patch.
static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
{
PCIESlot *s = (PCIESlot *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
@@ -217,7 +259,8 @@ static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
/* the PCI_EXP_LNKSTA_DLLLA will be set in the hotplug function */
}
- pcie_cap_fill_lnk(exp_cap, s->width, s->speed, s->parent_obj.flitmode);
+ pcie_cap_fill_lnk(dev, exp_cap, s->width, s->speed,
+ s->parent_obj.flitmode);
As previously I think that should be a PCIE_PORT(s)->flitmode
}
Yeah, will correct it.
+int pcie_config_uio_svc(PCIDevice *d, Error **errp)
+{
+ PCIEPort *p = PCIE_PORT(d);
+
+ if (!get_uio_mandatory_svc(p)
+ || pcie_svc_cap_init(d, PCI_EXT_CAP_BASE_OFFSET, errp) < 0) {
|| on the line above
Got it, thanks for pointing out. Will correct this.
+int pcie_svc_cap_init(PCIDevice *dev, uint16_t offset, Error **errp)
+{
+ uint32_t hdr;
+
+ if (!pci_is_express(dev)) {
+ error_setg(errp, "SVC ECAP requires PCIe");
+ return -EINVAL;
+ }
+
+ /*
+ * If no other ECAPs are present, make SVC the first at 0x100.
+ * This avoids pcie_add_capability() asserting on a non-0x100 offset.
+ */
+ hdr = pci_get_long(dev->config + PCI_CONFIG_SPACE_SIZE);
Is there precedence for this? Seems like most cases hand code
an offset. I think letting this run has the risk that an ordering
change might end up with this where something else wants to go.
Yeah, will check the precedence and change it accordingly.
+ if (hdr == 0) {
+ offset = PCI_CONFIG_SPACE_SIZE;
+ }
void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp);
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 33a22229fe..644da744b2 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -81,6 +81,7 @@ typedef enum PCIExpLinkWidth {
#define PCI_EXP_DEVCAP2_EETLPP 0x200000
#define PCI_EXP_DEVCTL2_EETLPPB 0x8000
+#define PCI_EXP_LNKCTL_FLIT_DIS 0x2000
This extra indent is supposed to associate the field with the register
but the register isn't defined here so it makes little sense.
I see there is some precedence in here for registers that are also defined
in the linux header that is included via hw/pci/pci_regs.h
Maybe we should clean that up a t somepoint.
/* ARI */
#define PCI_ARI_VER 1
diff --git a/include/hw/pci/pcie_svc.h b/include/hw/pci/pcie_svc.h
new file mode 100644
index 0000000000..4872905501
--- /dev/null
+++ b/include/hw/pci/pcie_svc.h
@@ -0,0 +1,91 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * PCIe Streamlined Virtual Channel (SVC) Extended Capability
+ *
+ * Copyright (c) 2026 Samsung Electronics Co., Ltd.
+ */
+
+#ifndef HW_PCIE_SVC_H
+#define HW_PCIE_SVC_H
Much of this feels like it will end up in pci_regs.h
so maybe just do that from the start.
We thought that we should follow the same pattern that PCIe SRIOV did
"pcie_sriov.h", so we added pcie_svc.h. I agree that all the register
declaration and definition should go in pci_regs.h. Will do that.
For this series you'd have to have a patch adding them with
a note on when you expect them to be in the linux header.
Yeah sure.
+#define PCI_EXT_CAP_BASE_OFFSET 0x200
+#define PCI_EXT_CAP_ID_SVC 0x35
+#define PCI_EXT_CAP_SVC_SIZE 0x74
+
+/* PCIe 6.4 section 7.9.29 */
+#define PCIE_SVC_CAP_HEAD_OFFSET 0x00
+#define PCIE_SVC_CAP_OFFSET 0x04
+#define SVC_TC_VC_MAP(n) ((n & 0xff) << 0)
+
+/* 7.9.27.8 SVC Resource Status Register */
+#define SVC_RES_STATUS_BASE 0x1c
+#define SVC_RES_STATUS(n) (SVC_RES_STATUS_BASE + \
+ (n) * 0x0c)
That's not a nice line break for readabilty. If you have two, move the whole
thing to next line.
Sure, will rectify it.
