Re: [RFC 6/8] hw/pci: Add PCIe Streamlined Virtual Channel (SVC) capability.

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  9 Jun 2026 16:28:34 +0530
Shrihari E S <shrihari.s@samsung.com> wrote:

> From: Dongjoo Seo <dongjoo.seo1@samsung.com>
> 
> 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 <dongjoo.seo1@samsung.com>
> Signed-off-by: Shrihari E S <shrihari.s@samsung.com>
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)