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 ;) > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 5996229c81..0469250f42 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > /***********************************************************/ > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index d452199d85..7c7d948e00 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -112,8 +112,9 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t > type, uint8_t version) > } > > /* Includes setting the target speed default */ > -static void pcie_cap_fill_lnk(uint8_t *exp_cap, PCIExpLinkWidth width, > - PCIExpLinkSpeed speed, bool flitmode) > +static void pcie_cap_fill_lnk(PCIDevice *dev, uint8_t *exp_cap, > + PCIExpLinkWidth width, PCIExpLinkSpeed speed, > + bool flitmode) > { > /* Clear and fill LNKCAP from what was configured above */ > 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? > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_FLAGS, > + PCI_EXP_FLAGS_FLIT); > + pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_FLIT_DIS); > } > } > > 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 > } > > int pcie_cap_init(PCIDevice *dev, uint8_t offset, > diff --git a/hw/pci/pcie_svc.c b/hw/pci/pcie_svc.c > new file mode 100644 > index 0000000000..84db73de58 > --- /dev/null > +++ b/hw/pci/pcie_svc.c > @@ -0,0 +1,164 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * > + * PCIe Streamlined Virtual Channel (SVC) Extended Capability > + * > + * Copyright (c) 2026 Samsung Electronics Co., Ltd. > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/bitops.h" > +#include "hw/pci/pci_device.h" > +#include "hw/pci/pcie.h" > +#include "hw/pci/pcie_svc.h" > +#include "hw/pci/pcie_port.h" > + > +static void pcie_svc_update_map(PCIDevice *dev) > +{ > + uint32_t non_uio_ctrl = SVC_VC0_PROTOCOL | SVC_VC_ENABLE; > + uint32_t uio_ctrl = SVC_UIO_PROTOCOL_SELECTED | SVC_VC_ENABLE; > + int offset; > + > + if (!pci_is_express(dev) || !dev->exp.svc_cap) { > + return; > + } > + > + offset = pcie_find_capability(dev, PCI_EXT_CAP_ID_SVC); > + > + /* SVC VC0 & VC3 initialization */ > + pci_set_long(dev->config + offset + SVC_RES_CTRL(0), BIT(0) | > non_uio_ctrl); > + pci_set_long(dev->config + offset + SVC_RES_CTRL(3), BIT(3) | uio_ctrl); > + > + /* SVC VC4 initialization - optional */ > + if (dev->exp.svc.uio_opt_svc) { > + pci_set_long(dev->config + offset + SVC_RES_CTRL(4), BIT(4) | > uio_ctrl); > + } > +} > + > +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 > + return -1; > + } > + > + if (get_uio_optional_svc(p)) { > + pcie_svc_set_vc4(d, true); > + } > + > + return 0; > +} > + > +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. > + 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. 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. > + > +#include "hw/pci/pci.h" > +#include "qemu/bitops.h" > + > +/* > + * The PCIe config space starts from 0x00 till 0xFF. In that > + * extended capability starts from 0x100 and each extended capability > + * is of dword size (32 bit). So we have to give a proper base offset > + * value which should be >= 0x100 and <= 0xFF and in between other > + * capability will also be registerd. We might not know where exactly > + * SVC extended capability will sit, so to avoid the overlapping we have > + * to give a very high offset. > + */ > +#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. > +#define SVC_VC_NEGO BIT(1)
