[PATCH 0/3] FM-API Physical switch command set update

[PATCH 0/3] FM-API Physical switch command set update

[Arpit Kumar]

The main purpose of this patch series is to enhance existing support
(Identify Switch Device & Get Physical Port State)
as well as add new support (Physical Port Control)
of FM-API based physical switch command set
as per CXL spec r3.2 Table 8-230:Physical Switch.

[Patch 1/3] stores port info during enumeration for both mailbox
and mctp based component command interface (cci).
This not only simplifies command support of Identify Switch Device (Opcode 5100h)
and Get Physical Port State (Opcode 5101h) but also provides necessary
physical port information for future additions hence [Patch 2/3].

[Patch 3/3] and final patch adds command support of Physical Port Control(Opcode 5102h).
This includes assert-deassert PERST and reset PPB based on the
requested physical port as response payload.

The patches are generated against the Johnathan's tree
https://gitlab.com/jic23/qemu.git and branch cxl-2025-03-20.

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>

Arpit Kumar (3):
  hw/cxl: Storing physical ports info during enumeration
  hw/cxl: Simplified Identify Switch Device & Get Physical Port State
  hw/cxl: Add Physical Port Control (Opcode 5102h)

 hw/cxl/cxl-mailbox-utils.c  | 423 +++++++++++++++++++++++++++---------
 include/hw/cxl/cxl_device.h |  36 +++
 2 files changed, 352 insertions(+), 107 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw/cxl: Storing physical ports info during enumeration

[Arpit Kumar]

Physical ports info is stored for both mailbox cci &
mctp based cci type as per spec CXL r3.2 Table 8-230: Physical Switch

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c  | 166 ++++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h |  28 ++++++
 2 files changed, 194 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index a02d130926..680055c6c0 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -124,6 +124,64 @@ enum {
         #define GET_MHD_INFO 0x0
 };
 
+/* link state flags */
+#define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)
+#define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
+#define LINK_STATE_FLAG_PRSNT            (1 << 2)
+#define LINK_STATE_FLAG_POWER_OFF        (1 << 3)
+
+/* physical port control info - CXL r3.2 table 7-19 */
+typedef enum {
+    PORT_DISABLED = 0,
+    BIND_IN_PROGRESS = 1,
+    UNBIND_IN_PROGRESS = 2,
+    DSP = 3,
+    USP = 4,
+    FABRIC_PORT = 5,
+    INVALID_PORT_ID = 15
+} current_port_config_state;
+
+typedef enum {
+    NOT_CXL_OR_DISCONNECTED = 0x00,
+    RCD_MODE = 0x01,
+    CXL_68B_FLIT_AND_VH_MODE = 0x02,
+    STANDARD_256B_FLIT_MODE = 0x03,
+    CXL_LATENCY_OPTIMIZED_256B_FLIT_MODE = 0x04,
+    PBR_MODE = 0x05
+} connected_device_mode;
+
+typedef enum {
+    NO_DEVICE_DETECTED = 0,
+    PCIE_DEVICE = 1,
+    CXL_TYPE_1_DEVICE = 2,
+    CXL_TYPE_2_DEVICE_OR_HBR_SWITCH = 3,
+    CXL_TYPE_3_SLD = 4,
+    CXL_TYPE_3_MLD = 5,
+    PBR_COMPONENT = 6
+} connected_device_type;
+
+typedef enum {
+    CXL_RCD_MODE = 0x00,
+    CXL_68B_FLIT_AND_VH_CAPABLE = 0x01,
+    CXL_256B_FLIT_CAPABLE = 0x02,
+    CXL_LATENCY_OPTIMIZED_256B_FLIT = 0x03,
+    CXL_PBR_CAPABLE = 0x04
+} supported_cxl_modes;
+
+typedef enum {
+    LTSSM_DETECT = 0x00,
+    LTSSM_POLLING = 0x01,
+    LTSSM_CONFIGURATION = 0x02,
+    LTSSM_RECOVERY = 0x03,
+    LTSSM_L0 = 0x04,
+    LTSSM_L0S = 0x05,
+    LTSSM_L1 = 0x06,
+    LTSSM_L2 = 0x07,
+    LTSSM_DISABLED = 0x08,
+    LTSSM_LOOPBACK = 0x09,
+    LTSSM_HOT_RESET = 0x0A
+} LTSSM_State;
+
 /* CCI Message Format CXL r3.1 Figure 7-19 */
 typedef struct CXLCCIMessage {
     uint8_t category;
@@ -3686,6 +3744,112 @@ void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
     cxl_rebuild_cel(cci);
 }
 
+static CXLRetCode cxl_set_port_type(struct phy_port *ports, int pnum,
+                                    CXLCCI *cci)
+{
+    PCIDevice *port_dev;
+    uint16_t lnkcap, lnkcap2, lnksta;
+    int i = pnum;
+    if (!cci) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+    PCIEPort *usp = PCIE_PORT(cci->d);
+    port_dev = pcie_find_port_by_pn(bus, i);
+
+    if (port_dev) { /* DSP */
+        PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
+            ->devices[0];
+        ports->pport_info[i].port_id = i;
+        ports->pport_info[i].current_port_config_state = DSP;
+        ports->active_port_bitmask[i / 8] |= (1 << i % 8);
+        if (ds_dev) {
+            if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
+                ports->pport_info[i].connected_device_type = CXL_TYPE_3_MLD;
+            } else {
+                ports->pport_info[i].connected_device_type = PCIE_DEVICE;
+            }
+        } else {
+            ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
+        }
+        ports->pport_info[i].supported_ld_count = 3;
+    } else if (usp->port == i) { /* USP */
+        port_dev = PCI_DEVICE(usp);
+        ports->pport_info[i].port_id = i;
+        ports->pport_info[i].current_port_config_state = USP;
+        ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
+        ports->active_port_bitmask[i / 8] |= (1 << i % 8);
+    } else {
+        return CXL_MBOX_INVALID_INPUT;
+    }
+
+    if (!port_dev->exp.exp_cap) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    lnksta = port_dev->config_read(port_dev,
+                                   port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
+                                   sizeof(lnksta));
+    lnkcap = port_dev->config_read(port_dev,
+                                   port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
+                                   sizeof(lnkcap));
+    lnkcap2 = port_dev->config_read(port_dev,
+                                    port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
+                                    sizeof(lnkcap2));
+
+    ports->pport_info[i].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
+    ports->pport_info[i].negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
+    ports->pport_info[i].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
+    ports->pport_info[i].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
+    ports->pport_info[i].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
+
+    ports->pport_info[i].ltssm_state = LTSSM_L2;
+    ports->pport_info[i].first_negotiated_lane_num = 0;
+    ports->pport_info[i].link_state_flags = 0;
+    ports->pport_info[i].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
+    ports->pport_info[i].connected_device_mode = STANDARD_256B_FLIT_MODE;
+
+    return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
+{
+    uint8_t phy_port_num;
+    if (!cci) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    PCIEPort *usp = PCIE_PORT(cci->d);
+    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+    struct phy_port *ports = &cci->pports;
+    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
+    if (num_phys_ports < 0) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    ports->num_ports = num_phys_ports;
+    phy_port_num =  usp->port;
+
+    cxl_set_port_type(ports, phy_port_num, cci); /* usp */
+
+    for (int devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
+        PCIDevice *dev = bus->devices[devfn];
+
+        if (dev) {
+            phy_port_num = PCIE_PORT(dev)->port;
+            const char *typename = object_get_typename(OBJECT(dev));
+
+            if ((strcmp(typename, "cxl-downstream") == 0)) {
+                cxl_set_port_type(ports, phy_port_num, cci);
+            } else {
+                return CXL_MBOX_INTERNAL_ERROR;
+            }
+        }
+    }
+    return CXL_MBOX_SUCCESS;
+}
+
 void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
                                   DeviceState *d, size_t payload_max)
 {
@@ -3693,6 +3857,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
     cci->d = d;
     cci->intf = intf;
     cxl_init_cci(cci, payload_max);
+    cxl_set_phy_port_info(cci); /* store port info */
 }
 
 void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
@@ -3797,4 +3962,5 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
     cci->d = d;
     cci->intf = intf;
     cxl_init_cci(cci, payload_max);
+    cxl_set_phy_port_info(cci); /* store port info */
 }
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index ca515cab13..9eb128a1e8 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -127,6 +127,31 @@
                   CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
                   (1 << 16))
 
+/* CXL r3.2 Table 7-19: Port Info */
+struct cxl_phy_port_info {
+    uint8_t port_id;
+    uint8_t current_port_config_state;
+    uint8_t connected_device_mode;
+    uint8_t rsv1;
+    uint8_t connected_device_type;
+    uint8_t supported_cxl_modes;
+    uint8_t max_link_width;
+    uint8_t negotiated_link_width;
+    uint8_t supported_link_speeds_vector;
+    uint8_t max_link_speed;
+    uint8_t current_link_speed;
+    uint8_t ltssm_state;
+    uint8_t first_negotiated_lane_num;
+    uint16_t link_state_flags;
+    uint8_t supported_ld_count;
+} QEMU_PACKED;
+
+struct phy_port {
+    uint8_t num_ports;
+    uint8_t active_port_bitmask[0x20];
+    struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
+};
+
 /* CXL r3.1 Table 8-34: Command Return Codes */
 typedef enum {
     CXL_MBOX_SUCCESS = 0x0,
@@ -223,6 +248,9 @@ typedef struct CXLCCI {
     /* get log capabilities */
     const CXLLogCapabilities *supported_log_cap;
 
+    /*physical ports information */
+    struct phy_port pports;
+
     /* background command handling (times in ms) */
     struct {
         uint16_t opcode;
-- 
2.34.1

Re: [PATCH 1/3] hw/cxl: Storing physical ports info during enumeration

[Jonathan Cameron via]

On Mon,  2 Jun 2025 19:29:40 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

> Physical ports info is stored for both mailbox cci &
> mctp based cci type as per spec CXL r3.2 Table 8-230: Physical Switch
> 
> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Hi Arpit,

Sorry for slow response. I got behind on reviews in general and missed
this in the backlog!

Anyhow, main comment is that this is mostly a refactor that only makes
sense in a single patch with what you have in patch 2.

Don't introduce duplicate infrastructure for so few usecases.  It means
we can't just look at he diff and see what was added vs what was removed.
Much cleaner to just have a single refactoring patch.
> ---
>  hw/cxl/cxl-mailbox-utils.c  | 166 ++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |  28 ++++++
>  2 files changed, 194 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index a02d130926..680055c6c0 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -124,6 +124,64 @@ enum {
>          #define GET_MHD_INFO 0x0
>  };
>  
> +/* link state flags */
Naming makes that clear (which is great) so I'd drop the comment.

> +#define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)

BIT(0) from qemu/bitops.h

> +#define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
> +#define LINK_STATE_FLAG_PRSNT            (1 << 2)
> +#define LINK_STATE_FLAG_POWER_OFF        (1 << 3)
> +
> +/* physical port control info - CXL r3.2 table 7-19 */
> +typedef enum {
> +    PORT_DISABLED = 0,
> +    BIND_IN_PROGRESS = 1,
> +    UNBIND_IN_PROGRESS = 2,
> +    DSP = 3,
> +    USP = 4,
> +    FABRIC_PORT = 5,
> +    INVALID_PORT_ID = 15
> +} current_port_config_state;

These aren't used as types really but more as defines.
Hence I'd be tempted to make them defines. Also provide
defines for the masks.

Namespace the defines so it is obvious which field
they are in.

> +
> +typedef enum {
> +    NOT_CXL_OR_DISCONNECTED = 0x00,
> +    RCD_MODE = 0x01,
> +    CXL_68B_FLIT_AND_VH_MODE = 0x02,
> +    STANDARD_256B_FLIT_MODE = 0x03,
> +    CXL_LATENCY_OPTIMIZED_256B_FLIT_MODE = 0x04,
> +    PBR_MODE = 0x05
> +} connected_device_mode;
> +
> +typedef enum {
> +    NO_DEVICE_DETECTED = 0,
> +    PCIE_DEVICE = 1,
> +    CXL_TYPE_1_DEVICE = 2,
> +    CXL_TYPE_2_DEVICE_OR_HBR_SWITCH = 3,
> +    CXL_TYPE_3_SLD = 4,
> +    CXL_TYPE_3_MLD = 5,
> +    PBR_COMPONENT = 6
> +} connected_device_type;
> +
> +typedef enum {
> +    CXL_RCD_MODE = 0x00,
> +    CXL_68B_FLIT_AND_VH_CAPABLE = 0x01,
> +    CXL_256B_FLIT_CAPABLE = 0x02,
> +    CXL_LATENCY_OPTIMIZED_256B_FLIT = 0x03,
> +    CXL_PBR_CAPABLE = 0x04
> +} supported_cxl_modes;
> +
> +typedef enum {
> +    LTSSM_DETECT = 0x00,
> +    LTSSM_POLLING = 0x01,
> +    LTSSM_CONFIGURATION = 0x02,
> +    LTSSM_RECOVERY = 0x03,
> +    LTSSM_L0 = 0x04,
> +    LTSSM_L0S = 0x05,
> +    LTSSM_L1 = 0x06,
> +    LTSSM_L2 = 0x07,
> +    LTSSM_DISABLED = 0x08,
> +    LTSSM_LOOPBACK = 0x09,
> +    LTSSM_HOT_RESET = 0x0A
> +} LTSSM_State;
> +
>  /* CCI Message Format CXL r3.1 Figure 7-19 */
>  typedef struct CXLCCIMessage {
>      uint8_t category;
> @@ -3686,6 +3744,112 @@ void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
>      cxl_rebuild_cel(cci);
>  }
>  
> +static CXLRetCode cxl_set_port_type(struct phy_port *ports, int pnum,
> +                                    CXLCCI *cci)
> +{
> +    PCIDevice *port_dev;
> +    uint16_t lnkcap, lnkcap2, lnksta;
> +    int i = pnum;
I'd just use pnum for all the indexes.  This local variable doesn't add
much readability.

> +    if (!cci) {

As below. No need to defend against bugs so only check if there is a
chance cci might not be set.

> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +

Don't interleave declarations and cod.e

> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +    PCIEPort *usp = PCIE_PORT(cci->d);

blank line after declarations - of include this
in the declarations as
    PCIEDevice *port_dev = pcie_find_port_by_pn(bus, i);

However that is reasonable expensive, so maybe do the usp case first?


> +    port_dev = pcie_find_port_by_pn(bus, i);
> +
> +    if (port_dev) { /* DSP */
> +        PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
> +            ->devices[0];
> +        ports->pport_info[i].port_id = i;

This is not affected by USP or DSP, so drop it out of this if / else.

> +        ports->pport_info[i].current_port_config_state = DSP;
> +        ports->active_port_bitmask[i / 8] |= (1 << i % 8);

As below - this is currently set but not read, so I'd bring it in as part of
the patch that uses it.

Independent of port type, so doesn't belong in the if/else.


> +        if (ds_dev) {
> +            if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
> +                ports->pport_info[i].connected_device_type = CXL_TYPE_3_MLD;

Hmm. We should make this controllable (vs SLD).  I made this up in the existing
code and probably shouldn't have done :(

> +            } else {
> +                ports->pport_info[i].connected_device_type = PCIE_DEVICE;
> +            }
> +        } else {
> +            ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
> +        }
> +        ports->pport_info[i].supported_ld_count = 3;
> +    } else if (usp->port == i) { /* USP */
> +        port_dev = PCI_DEVICE(usp);
> +        ports->pport_info[i].port_id = i;
> +        ports->pport_info[i].current_port_config_state = USP;
> +        ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
> +        ports->active_port_bitmask[i / 8] |= (1 << i % 8);
> +    } else {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +
> +    if (!port_dev->exp.exp_cap) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    lnksta = port_dev->config_read(port_dev,
> +                                   port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
> +                                   sizeof(lnksta));
> +    lnkcap = port_dev->config_read(port_dev,
> +                                   port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
> +                                   sizeof(lnkcap));
> +    lnkcap2 = port_dev->config_read(port_dev,
> +                                    port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
> +                                    sizeof(lnkcap2));
> +
> +    ports->pport_info[i].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
> +    ports->pport_info[i].negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
> +    ports->pport_info[i].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
> +    ports->pport_info[i].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
> +    ports->pport_info[i].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
> +
> +    ports->pport_info[i].ltssm_state = LTSSM_L2;
> +    ports->pport_info[i].first_negotiated_lane_num = 0;
> +    ports->pport_info[i].link_state_flags = 0;
> +    ports->pport_info[i].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
> +    ports->pport_info[i].connected_device_mode = STANDARD_256B_FLIT_MODE;
> +
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
> +{
> +    uint8_t phy_port_num;
> +    if (!cci) {

What is this defending against?  At least for now the cci
is definitely not null where this function is used.

> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    PCIEPort *usp = PCIE_PORT(cci->d);

Don't mix declarations and code.  Declarations still typically
go at the top.

> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +    struct phy_port *ports = &cci->pports;
> +    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
> +    if (num_phys_ports < 0) {

Given add 1 this is always false and the check serve no
purpose.

> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    ports->num_ports = num_phys_ports;
> +    phy_port_num =  usp->port;
> +
> +    cxl_set_port_type(ports, phy_port_num, cci); /* usp */

USP for comment given it's an acronym.
 
> +
> +    for (int devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {

Maybe use pci_for_each_device_under_bus() ?

> +        PCIDevice *dev = bus->devices[devfn];
> +
> +        if (dev) {
> +            phy_port_num = PCIE_PORT(dev)->port;
> +            const char *typename = object_get_typename(OBJECT(dev));

Normally in qemu we just use a dynamic cast to identify types rather than
matching on names.  I'd prefer that approach here.

if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)


> +
> +            if ((strcmp(typename, "cxl-downstream") == 0)) {
> +                cxl_set_port_type(ports, phy_port_num, cci);
> +            } else {
> +                return CXL_MBOX_INTERNAL_ERROR;
> +            }
> +        }
> +    }
> +    return CXL_MBOX_SUCCESS;
> +}
> +
>  void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>                                    DeviceState *d, size_t payload_max)
>  {
> @@ -3693,6 +3857,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>      cci->d = d;
>      cci->intf = intf;
>      cxl_init_cci(cci, payload_max);
> +    cxl_set_phy_port_info(cci); /* store port info */
>  }
>  
>  void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
> @@ -3797,4 +3962,5 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>      cci->d = d;
>      cci->intf = intf;
>      cxl_init_cci(cci, payload_max);
> +    cxl_set_phy_port_info(cci); /* store port info */

I'd not bother with the comment.  The naming of the function is clear enough.

>  }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index ca515cab13..9eb128a1e8 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -127,6 +127,31 @@
>                    CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
>                    (1 << 16))
>  
> +/* CXL r3.2 Table 7-19: Port Info */
> +struct cxl_phy_port_info {

There is one of these in cxl-mailbox-utils.c already.  Pull it out
as part of this patch.  That may mean combining patches 1 and 2.


> +    uint8_t port_id;
> +    uint8_t current_port_config_state;

I'd pull the field defines and masks etc alongside the structure
definition.


> +    uint8_t connected_device_mode;
> +    uint8_t rsv1;
> +    uint8_t connected_device_type;
> +    uint8_t supported_cxl_modes;
> +    uint8_t max_link_width;
> +    uint8_t negotiated_link_width;
> +    uint8_t supported_link_speeds_vector;
> +    uint8_t max_link_speed;
> +    uint8_t current_link_speed;
> +    uint8_t ltssm_state;
> +    uint8_t first_negotiated_lane_num;
> +    uint16_t link_state_flags;
> +    uint8_t supported_ld_count;
> +} QEMU_PACKED;
> +
> +struct phy_port {
> +    uint8_t num_ports;
> +    uint8_t active_port_bitmask[0x20];

Why that size?  Also not used yet - so maybe bring this in where
you need it?

> +    struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];

I think there are lower limits than that on how many ports
we can have on a given bus + does this actually care about
the limits from one bus?  Request is per switch, not per
virtual heirarchy.  So we might be limited by number of unique
port numbers.

Today we only have one VCH so maybe this is fine for now.


> +};
> +
>  /* CXL r3.1 Table 8-34: Command Return Codes */
>  typedef enum {
>      CXL_MBOX_SUCCESS = 0x0,
> @@ -223,6 +248,9 @@ typedef struct CXLCCI {
>      /* get log capabilities */
>      const CXLLogCapabilities *supported_log_cap;
>  
> +    /*physical ports information */

Space after /* 

Run checkpatch.pl over qemu patches. I 'think' it would have
caught this trivial thing.

> +    struct phy_port pports;
Storing this in the CCI is a little odd seeing (as opposed to
in the USP - maybe DSP) as there is only one answer to this query whichever
CCI we come in on.

For similar things we do have firmware update in there which
is a mixture of device side stuff and CCI specific handling.
That might ideally be split up.  Obviously not something for
this patch, but maybe we can avoid making CCI more bloated? 

To me it seems reasonable to store this in the port and set
it up whether or not the cci is connected.

> +
>      /* background command handling (times in ms) */
>      struct {
>          uint16_t opcode;

Re: [PATCH 1/3] hw/cxl: Storing physical ports info during enumeration

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 13848 bytes --]

On 10/06/25 03:21PM, Jonathan Cameron wrote:
>On Mon,  2 Jun 2025 19:29:40 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> Physical ports info is stored for both mailbox cci &
>> mctp based cci type as per spec CXL r3.2 Table 8-230: Physical Switch
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>Hi Arpit,
>
>Sorry for slow response. I got behind on reviews in general and missed
>this in the backlog!
>
>Anyhow, main comment is that this is mostly a refactor that only makes
>sense in a single patch with what you have in patch 2.
>
>Don't introduce duplicate infrastructure for so few usecases.  It means
>we can't just look at he diff and see what was added vs what was removed.
>Much cleaner to just have a single refactoring patch.

Thanks Jonathan for the review. Will combine the first 2 patches and
apply changes as suggested in the next iteration(V2) of the patch
series.
>> ---
>>  hw/cxl/cxl-mailbox-utils.c  | 166 ++++++++++++++++++++++++++++++++++++
>>  include/hw/cxl/cxl_device.h |  28 ++++++
>>  2 files changed, 194 insertions(+)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index a02d130926..680055c6c0 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -124,6 +124,64 @@ enum {
>>          #define GET_MHD_INFO 0x0
>>  };
>>
>> +/* link state flags */
>Naming makes that clear (which is great) so I'd drop the comment.
>
okay
>> +#define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)
>
>BIT(0) from qemu/bitops.h
>
got it
>> +#define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
>> +#define LINK_STATE_FLAG_PRSNT            (1 << 2)
>> +#define LINK_STATE_FLAG_POWER_OFF        (1 << 3)
>> +
>> +/* physical port control info - CXL r3.2 table 7-19 */
>> +typedef enum {
>> +    PORT_DISABLED = 0,
>> +    BIND_IN_PROGRESS = 1,
>> +    UNBIND_IN_PROGRESS = 2,
>> +    DSP = 3,
>> +    USP = 4,
>> +    FABRIC_PORT = 5,
>> +    INVALID_PORT_ID = 15
>> +} current_port_config_state;
>
>These aren't used as types really but more as defines.
>Hence I'd be tempted to make them defines. Also provide
>defines for the masks.
>
>Namespace the defines so it is obvious which field
>they are in.
>
Will the below changes be the right way to proceed?
#define CXL_PORT_CONFIG_STATE_DISABLED           0x0
#define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1 etc.

>> +
>> +typedef enum {
>> +    NOT_CXL_OR_DISCONNECTED = 0x00,
>> +    RCD_MODE = 0x01,
>> +    CXL_68B_FLIT_AND_VH_MODE = 0x02,
>> +    STANDARD_256B_FLIT_MODE = 0x03,
>> +    CXL_LATENCY_OPTIMIZED_256B_FLIT_MODE = 0x04,
>> +    PBR_MODE = 0x05
>> +} connected_device_mode;
>> +
>> +typedef enum {
>> +    NO_DEVICE_DETECTED = 0,
>> +    PCIE_DEVICE = 1,
>> +    CXL_TYPE_1_DEVICE = 2,
>> +    CXL_TYPE_2_DEVICE_OR_HBR_SWITCH = 3,
>> +    CXL_TYPE_3_SLD = 4,
>> +    CXL_TYPE_3_MLD = 5,
>> +    PBR_COMPONENT = 6
>> +} connected_device_type;
>> +
>> +typedef enum {
>> +    CXL_RCD_MODE = 0x00,
>> +    CXL_68B_FLIT_AND_VH_CAPABLE = 0x01,
>> +    CXL_256B_FLIT_CAPABLE = 0x02,
>> +    CXL_LATENCY_OPTIMIZED_256B_FLIT = 0x03,
>> +    CXL_PBR_CAPABLE = 0x04
>> +} supported_cxl_modes;
>> +
>> +typedef enum {
>> +    LTSSM_DETECT = 0x00,
>> +    LTSSM_POLLING = 0x01,
>> +    LTSSM_CONFIGURATION = 0x02,
>> +    LTSSM_RECOVERY = 0x03,
>> +    LTSSM_L0 = 0x04,
>> +    LTSSM_L0S = 0x05,
>> +    LTSSM_L1 = 0x06,
>> +    LTSSM_L2 = 0x07,
>> +    LTSSM_DISABLED = 0x08,
>> +    LTSSM_LOOPBACK = 0x09,
>> +    LTSSM_HOT_RESET = 0x0A
>> +} LTSSM_State;
>> +
>>  /* CCI Message Format CXL r3.1 Figure 7-19 */
>>  typedef struct CXLCCIMessage {
>>      uint8_t category;
>> @@ -3686,6 +3744,112 @@ void cxl_add_cci_commands(CXLCCI *cci, const struct cxl_cmd (*cxl_cmd_set)[256],
>>      cxl_rebuild_cel(cci);
>>  }
>>
>> +static CXLRetCode cxl_set_port_type(struct phy_port *ports, int pnum,
>> +                                    CXLCCI *cci)
>> +{
>> +    PCIDevice *port_dev;
>> +    uint16_t lnkcap, lnkcap2, lnksta;
>> +    int i = pnum;
>I'd just use pnum for all the indexes.  This local variable doesn't add
>much readability.
>
okay
>> +    if (!cci) {
>
>As below. No need to defend against bugs so only check if there is a
>chance cci might not be set.
>
right, thanks for pointing out.
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>
>Don't interleave declarations and cod.e
>
got it
>> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +    PCIEPort *usp = PCIE_PORT(cci->d);
>
>blank line after declarations - of include this
>in the declarations as
>    PCIEDevice *port_dev = pcie_find_port_by_pn(bus, i);
>
>However that is reasonable expensive, so maybe do the usp case first?
>
got it
>
>> +    port_dev = pcie_find_port_by_pn(bus, i);
>> +
>> +    if (port_dev) { /* DSP */
>> +        PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
>> +            ->devices[0];
>> +        ports->pport_info[i].port_id = i;
>
>This is not affected by USP or DSP, so drop it out of this if / else.
>
okay
>> +        ports->pport_info[i].current_port_config_state = DSP;
>> +        ports->active_port_bitmask[i / 8] |= (1 << i % 8);
>
>As below - this is currently set but not read, so I'd bring it in as part of
>the patch that uses it.
>
>Independent of port type, so doesn't belong in the if/else.
>
okay
>
>> +        if (ds_dev) {
>> +            if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
>> +                ports->pport_info[i].connected_device_type = CXL_TYPE_3_MLD;
>
>Hmm. We should make this controllable (vs SLD).  I made this up in the existing
>code and probably shouldn't have done :(
>
got it, will currently change it to CXL_TYPE_3_SLD in next iteration of
patch series. However, once we add support for MLD, will make this
controllable as suggested.
>> +            } else {
>> +                ports->pport_info[i].connected_device_type = PCIE_DEVICE;
>> +            }
>> +        } else {
>> +            ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
>> +        }
>> +        ports->pport_info[i].supported_ld_count = 3;
>> +    } else if (usp->port == i) { /* USP */
>> +        port_dev = PCI_DEVICE(usp);
>> +        ports->pport_info[i].port_id = i;
>> +        ports->pport_info[i].current_port_config_state = USP;
>> +        ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
>> +        ports->active_port_bitmask[i / 8] |= (1 << i % 8);
>> +    } else {
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +
>> +    if (!port_dev->exp.exp_cap) {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    lnksta = port_dev->config_read(port_dev,
>> +                                   port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
>> +                                   sizeof(lnksta));
>> +    lnkcap = port_dev->config_read(port_dev,
>> +                                   port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
>> +                                   sizeof(lnkcap));
>> +    lnkcap2 = port_dev->config_read(port_dev,
>> +                                    port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
>> +                                    sizeof(lnkcap2));
>> +
>> +    ports->pport_info[i].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
>> +    ports->pport_info[i].negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
>> +    ports->pport_info[i].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
>> +    ports->pport_info[i].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
>> +    ports->pport_info[i].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
>> +
>> +    ports->pport_info[i].ltssm_state = LTSSM_L2;
>> +    ports->pport_info[i].first_negotiated_lane_num = 0;
>> +    ports->pport_info[i].link_state_flags = 0;
>> +    ports->pport_info[i].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
>> +    ports->pport_info[i].connected_device_mode = STANDARD_256B_FLIT_MODE;
>> +
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
>> +{
>> +    uint8_t phy_port_num;
>> +    if (!cci) {
>
>What is this defending against?  At least for now the cci
>is definitely not null where this function is used.
>
got it
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    PCIEPort *usp = PCIE_PORT(cci->d);
>
>Don't mix declarations and code.  Declarations still typically
>go at the top.
>
got it
>> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +    struct phy_port *ports = &cci->pports;
>> +    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
>> +    if (num_phys_ports < 0) {
>
>Given add 1 this is always false and the check serve no
>purpose.
>
right, thanks for pointing this out
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    ports->num_ports = num_phys_ports;
>> +    phy_port_num =  usp->port;
>> +
>> +    cxl_set_port_type(ports, phy_port_num, cci); /* usp */
>
>USP for comment given it's an acronym.
>
okay
>> +
>> +    for (int devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
>
>Maybe use pci_for_each_device_under_bus() ?
>
okay
>> +        PCIDevice *dev = bus->devices[devfn];
>> +
>> +        if (dev) {
>> +            phy_port_num = PCIE_PORT(dev)->port;
>> +            const char *typename = object_get_typename(OBJECT(dev));
>
>Normally in qemu we just use a dynamic cast to identify types rather than
>matching on names.  I'd prefer that approach here.
>
>if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)
>
>
got it
>> +
>> +            if ((strcmp(typename, "cxl-downstream") == 0)) {
>> +                cxl_set_port_type(ports, phy_port_num, cci);
>> +            } else {
>> +                return CXL_MBOX_INTERNAL_ERROR;
>> +            }
>> +        }
>> +    }
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>>  void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>>                                    DeviceState *d, size_t payload_max)
>>  {
>> @@ -3693,6 +3857,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>>      cci->d = d;
>>      cci->intf = intf;
>>      cxl_init_cci(cci, payload_max);
>> +    cxl_set_phy_port_info(cci); /* store port info */
>>  }
>>
>>  void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
>> @@ -3797,4 +3962,5 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>>      cci->d = d;
>>      cci->intf = intf;
>>      cxl_init_cci(cci, payload_max);
>> +    cxl_set_phy_port_info(cci); /* store port info */
>
>I'd not bother with the comment.  The naming of the function is clear enough.
>
okay
>>  }
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index ca515cab13..9eb128a1e8 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -127,6 +127,31 @@
>>                    CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
>>                    (1 << 16))
>>
>> +/* CXL r3.2 Table 7-19: Port Info */
>> +struct cxl_phy_port_info {
>
>There is one of these in cxl-mailbox-utils.c already.  Pull it out
>as part of this patch.  That may mean combining patches 1 and 2.
>
>
>> +    uint8_t port_id;
>> +    uint8_t current_port_config_state;
>
>I'd pull the field defines and masks etc alongside the structure
>definition.
>
okay
>
>> +    uint8_t connected_device_mode;
>> +    uint8_t rsv1;
>> +    uint8_t connected_device_type;
>> +    uint8_t supported_cxl_modes;
>> +    uint8_t max_link_width;
>> +    uint8_t negotiated_link_width;
>> +    uint8_t supported_link_speeds_vector;
>> +    uint8_t max_link_speed;
>> +    uint8_t current_link_speed;
>> +    uint8_t ltssm_state;
>> +    uint8_t first_negotiated_lane_num;
>> +    uint16_t link_state_flags;
>> +    uint8_t supported_ld_count;
>> +} QEMU_PACKED;
>> +
>> +struct phy_port {
>> +    uint8_t num_ports;
>> +    uint8_t active_port_bitmask[0x20];
>
>Why that size?  Also not used yet - so maybe bring this in where
>you need it?
>
>> +    struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
>
>I think there are lower limits than that on how many ports
>we can have on a given bus + does this actually care about
>the limits from one bus?  Request is per switch, not per
>virtual heirarchy.  So we might be limited by number of unique
>port numbers.
>
>Today we only have one VCH so maybe this is fine for now.
>
>
Right, the request is per switch and not per virtual heirarchy.
According to CXL r3.2 Table 7-16, length of number of physical ports
is 1 byte which accounts for 256 physical ports. So, will
#define CXL_MAX_PHY_PORTS 256 and change the length of pport_info[] to
it.

>> +};
>> +
>>  /* CXL r3.1 Table 8-34: Command Return Codes */
>>  typedef enum {
>>      CXL_MBOX_SUCCESS = 0x0,
>> @@ -223,6 +248,9 @@ typedef struct CXLCCI {
>>      /* get log capabilities */
>>      const CXLLogCapabilities *supported_log_cap;
>>
>> +    /*physical ports information */
>
>Space after /*
>
>Run checkpatch.pl over qemu patches. I 'think' it would have
>caught this trivial thing.
>
I did run checkpatch.pl but it didn't catch this. will recheck for such
trivial things moving forward.

>> +    struct phy_port pports;
>Storing this in the CCI is a little odd seeing (as opposed to
>in the USP - maybe DSP) as there is only one answer to this query whichever
>CCI we come in on.
>
>For similar things we do have firmware update in there which
>is a mixture of device side stuff and CCI specific handling.
>That might ideally be split up.  Obviously not something for
>this patch, but maybe we can avoid making CCI more bloated?
>
>To me it seems reasonable to store this in the port and set
>it up whether or not the cci is connected.
>
got it, will try the implementation accordingly. It will be helpful if could share some
reference on handling the same.
>> +
>>      /* background command handling (times in ms) */
>>      struct {
>>          uint16_t opcode;
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] hw/cxl: Storing physical ports info during enumeration

[Jonathan Cameron via]

> >> +#define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
> >> +#define LINK_STATE_FLAG_PRSNT            (1 << 2)
> >> +#define LINK_STATE_FLAG_POWER_OFF        (1 << 3)
> >> +
> >> +/* physical port control info - CXL r3.2 table 7-19 */
> >> +typedef enum {
> >> +    PORT_DISABLED = 0,
> >> +    BIND_IN_PROGRESS = 1,
> >> +    UNBIND_IN_PROGRESS = 2,
> >> +    DSP = 3,
> >> +    USP = 4,
> >> +    FABRIC_PORT = 5,
> >> +    INVALID_PORT_ID = 15
> >> +} current_port_config_state;  
> >
> >These aren't used as types really but more as defines.
> >Hence I'd be tempted to make them defines. Also provide
> >defines for the masks.
> >
> >Namespace the defines so it is obvious which field
> >they are in.
> >  
> Will the below changes be the right way to proceed?
> #define CXL_PORT_CONFIG_STATE_DISABLED           0x0
> #define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1 etc.

Yes


...

> >For similar things we do have firmware update in there which
> >is a mixture of device side stuff and CCI specific handling.
> >That might ideally be split up.  Obviously not something for
> >this patch, but maybe we can avoid making CCI more bloated?
> >
> >To me it seems reasonable to store this in the port and set
> >it up whether or not the cci is connected.
> >  
> got it, will try the implementation accordingly. It will be helpful if could share some
> reference on handling the same.

+    /*physical ports information */
+    struct phy_port pports;
+

Move this stuff to the CXLUpstreamPort.
Query the data to fill it in will need to be late enough that
everything is there to query. cxl_usp_reset() might work similar to how
we fill in link registers etc there. 

Then when you handle any of the CCI calls, use CXL_USP(cci->d) to
get to that USP structure.  Check that you have the right type if necessary
first though. 

Jonathan




> >> +
> >>      /* background command handling (times in ms) */
> >>      struct {
> >>          uint16_t opcode;  
> >  
> 

[PATCH 2/3] hw/cxl: Simplified Identify Switch Device & Get Physical Port State

[Arpit Kumar]

Modified Identify Switch Device (Opcode 5100h)
& Get Physical Port State(Opcode 5101h)
using physical ports info stored during enumeration

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c | 133 +++++++------------------------------
 1 file changed, 24 insertions(+), 109 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 680055c6c0..b2fa79a721 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -558,17 +558,7 @@ static CXLRetCode cmd_set_response_msg_limit(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
-static void cxl_set_dsp_active_bm(PCIBus *b, PCIDevice *d,
-                                  void *private)
-{
-    uint8_t *bm = private;
-    if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
-        uint8_t port = PCIE_PORT(d)->port;
-        bm[port / 8] |= 1 << (port % 8);
-    }
-}
-
-/* CXL r3.1 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
+/* CXL r3.2 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
 static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
                                              uint8_t *payload_in,
                                              size_t len_in,
@@ -576,9 +566,7 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
                                              size_t *len_out,
                                              CXLCCI *cci)
 {
-    PCIEPort *usp = PCIE_PORT(cci->d);
-    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
-    int num_phys_ports = pcie_count_ds_ports(bus);
+    int num_phys_ports = cci->pports.num_ports;
 
     struct cxl_fmapi_ident_switch_dev_resp_pl {
         uint8_t ingress_port_id;
@@ -595,11 +583,11 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
 
     out = (struct cxl_fmapi_ident_switch_dev_resp_pl *)payload_out;
     *out = (struct cxl_fmapi_ident_switch_dev_resp_pl) {
-        .num_physical_ports = num_phys_ports + 1, /* 1 USP */
+        .num_physical_ports = num_phys_ports,
         .num_vcss = 1, /* Not yet support multiple VCS - potentially tricky */
         .active_vcs_bitmask[0] = 0x1,
-        .total_vppbs = num_phys_ports + 1,
-        .bound_vppbs = num_phys_ports + 1,
+        .total_vppbs = num_phys_ports,
+        .bound_vppbs = num_phys_ports,
         .num_hdm_decoders_per_usp = 4,
     };
 
@@ -611,16 +599,14 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
         out->ingress_port_id = 0;
     }
 
-    pci_for_each_device_under_bus(bus, cxl_set_dsp_active_bm,
-                                  out->active_port_bitmask);
-    out->active_port_bitmask[usp->port / 8] |= (1 << usp->port % 8);
-
+    memcpy(out->active_port_bitmask, cci->pports.active_port_bitmask,
+           sizeof(cci->pports.active_port_bitmask));
     *len_out = sizeof(*out);
 
     return CXL_MBOX_SUCCESS;
 }
 
-/* CXL r3.1 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
+/* CXL r3.2 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
 static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
                                               uint8_t *payload_in,
                                               size_t len_in,
@@ -628,44 +614,21 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
                                               size_t *len_out,
                                               CXLCCI *cci)
 {
-    /* CXL r3.1 Table 7-17: Get Physical Port State Request Payload */
+    size_t pl_size;
+    int i;
+
+    /* CXL r3.2 Table 7-17: Get Physical Port State Request Payload */
     struct cxl_fmapi_get_phys_port_state_req_pl {
         uint8_t num_ports;
         uint8_t ports[];
     } QEMU_PACKED *in;
 
-    /*
-     * CXL r3.1 Table 7-19: Get Physical Port State Port Information Block
-     * Format
-     */
-    struct cxl_fmapi_port_state_info_block {
-        uint8_t port_id;
-        uint8_t config_state;
-        uint8_t connected_device_cxl_version;
-        uint8_t rsv1;
-        uint8_t connected_device_type;
-        uint8_t port_cxl_version_bitmask;
-        uint8_t max_link_width;
-        uint8_t negotiated_link_width;
-        uint8_t supported_link_speeds_vector;
-        uint8_t max_link_speed;
-        uint8_t current_link_speed;
-        uint8_t ltssm_state;
-        uint8_t first_lane_num;
-        uint16_t link_state;
-        uint8_t supported_ld_count;
-    } QEMU_PACKED;
-
-    /* CXL r3.1 Table 7-18: Get Physical Port State Response Payload */
+    /* CXL r3.2 Table 7-18: Get Physical Port State Response Payload */
     struct cxl_fmapi_get_phys_port_state_resp_pl {
         uint8_t num_ports;
         uint8_t rsv1[3];
-        struct cxl_fmapi_port_state_info_block ports[];
+        struct cxl_phy_port_info ports[];
     } QEMU_PACKED *out;
-    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
-    PCIEPort *usp = PCIE_PORT(cci->d);
-    size_t pl_size;
-    int i;
 
     in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
     out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
@@ -673,72 +636,24 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
     if (len_in < sizeof(*in)) {
         return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
     }
-    /* Check if what was requested can fit */
+
     if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max) {
         return CXL_MBOX_INVALID_INPUT;
     }
 
-    /* For success there should be a match for each requested */
-    out->num_ports = in->num_ports;
+    if (in->num_ports > cci->pports.num_ports) {
+        return CXL_MBOX_INVALID_INPUT;
+    }
 
+    out->num_ports = in->num_ports;
     for (i = 0; i < in->num_ports; i++) {
-        struct cxl_fmapi_port_state_info_block *port;
-        /* First try to match on downstream port */
-        PCIDevice *port_dev;
-        uint16_t lnkcap, lnkcap2, lnksta;
-
-        port = &out->ports[i];
-
-        port_dev = pcie_find_port_by_pn(bus, in->ports[i]);
-        if (port_dev) { /* DSP */
-            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
-                ->devices[0];
-            port->config_state = 3;
-            if (ds_dev) {
-                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
-                    port->connected_device_type = 5; /* Assume MLD for now */
-                } else {
-                    port->connected_device_type = 1;
-                }
-            } else {
-                port->connected_device_type = 0;
+        int pn = in->ports[i];
+        for (int j = 0; j < PCI_DEVFN_MAX; j++) {
+            if (pn == cci->pports.pport_info[j].port_id) {
+                memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
+                       sizeof(struct cxl_phy_port_info));
             }
-            port->supported_ld_count = 3;
-        } else if (usp->port == in->ports[i]) { /* USP */
-            port_dev = PCI_DEVICE(usp);
-            port->config_state = 4;
-            port->connected_device_type = 0;
-        } else {
-            return CXL_MBOX_INVALID_INPUT;
-        }
-
-        port->port_id = in->ports[i];
-        /* Information on status of this port in lnksta, lnkcap */
-        if (!port_dev->exp.exp_cap) {
-            return CXL_MBOX_INTERNAL_ERROR;
         }
-        lnksta = port_dev->config_read(port_dev,
-                                       port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
-                                       sizeof(lnksta));
-        lnkcap = port_dev->config_read(port_dev,
-                                       port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
-                                       sizeof(lnkcap));
-        lnkcap2 = port_dev->config_read(port_dev,
-                                        port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
-                                        sizeof(lnkcap2));
-
-        port->max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
-        port->negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
-        /* No definition for SLS field in linux/pci_regs.h */
-        port->supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
-        port->max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
-        port->current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
-        /* TODO: Track down if we can get the rest of the info */
-        port->ltssm_state = 0x7;
-        port->first_lane_num = 0;
-        port->link_state = 0;
-        port->port_cxl_version_bitmask = 0x2;
-        port->connected_device_cxl_version = 0x2;
     }
 
     pl_size = sizeof(*out) + sizeof(*out->ports) * in->num_ports;
-- 
2.34.1

Re: [PATCH 2/3] hw/cxl: Simplified Identify Switch Device & Get Physical Port State

[Jonathan Cameron via]

On Mon,  2 Jun 2025 19:29:41 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

> Modified Identify Switch Device (Opcode 5100h)
> & Get Physical Port State(Opcode 5101h)
> using physical ports info stored during enumeration
> 
> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
A few additional comments in here.

J
> ---
>  hw/cxl/cxl-mailbox-utils.c | 133 +++++++------------------------------
>  1 file changed, 24 insertions(+), 109 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 680055c6c0..b2fa79a721 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -558,17 +558,7 @@ static CXLRetCode cmd_set_response_msg_limit(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> -static void cxl_set_dsp_active_bm(PCIBus *b, PCIDevice *d,
> -                                  void *private)
> -{
> -    uint8_t *bm = private;
> -    if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
> -        uint8_t port = PCIE_PORT(d)->port;
> -        bm[port / 8] |= 1 << (port % 8);
> -    }
> -}
> -
> -/* CXL r3.1 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
> +/* CXL r3.2 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */

I'd prefer the spec reference updates in a separate patch. They are noise here
and kind of suggest there are real changes rather than just refactoring.


> @@ -611,16 +599,14 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
>          out->ingress_port_id = 0;
>      }
>  
> -    pci_for_each_device_under_bus(bus, cxl_set_dsp_active_bm,
> -                                  out->active_port_bitmask);
> -    out->active_port_bitmask[usp->port / 8] |= (1 << usp->port % 8);

Ah. With this in front of me the reason for the sizeing is much clearer
than in previous patch on it's own. Combining the two will make it all more obvious.

> -
> +    memcpy(out->active_port_bitmask, cci->pports.active_port_bitmask,
> +           sizeof(cci->pports.active_port_bitmask));
>      *len_out = sizeof(*out);
>  
>      return CXL_MBOX_SUCCESS;
>  }
>  
> -/* CXL r3.1 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
> +/* CXL r3.2 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
>  static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>                                                uint8_t *payload_in,
>                                                size_t len_in,
> @@ -628,44 +614,21 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>                                                size_t *len_out,
>                                                CXLCCI *cci)
>  {

>  
>      in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
>      out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
> @@ -673,72 +636,24 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>      if (len_in < sizeof(*in)) {
>          return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>      }
> -    /* Check if what was requested can fit */
> +

The check is still here... So why remove the comment?

>      if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max) {
>          return CXL_MBOX_INVALID_INPUT;
>      }
>  
> -    /* For success there should be a match for each requested */
> -    out->num_ports = in->num_ports;
> +    if (in->num_ports > cci->pports.num_ports) {
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
>  
> +    out->num_ports = in->num_ports;
>      for (i = 0; i < in->num_ports; i++) {
> -        struct cxl_fmapi_port_state_info_block *port;
> -        /* First try to match on downstream port */
> -        PCIDevice *port_dev;
> -        uint16_t lnkcap, lnkcap2, lnksta;
> -
> -        port = &out->ports[i];
> -
> -        port_dev = pcie_find_port_by_pn(bus, in->ports[i]);
> -        if (port_dev) { /* DSP */
> -            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
> -                ->devices[0];  
> -            port->config_state = 3;
> -            if (ds_dev) {
> -                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
> -                    port->connected_device_type = 5; /* Assume MLD for now */
> -                } else {
> -                    port->connected_device_type = 1;
> -                }
> -            } else {
> -                port->connected_device_type = 0;
> +        int pn = in->ports[i];
> +        for (int j = 0; j < PCI_DEVFN_MAX; j++) {
> +            if (pn == cci->pports.pport_info[j].port_id) {

Given port id is 0-255 and your port_info has 256 elements, why not index
by port_id when storing them in the first place? That should reduce
complexity of this look up.  I don't think we ever actually look up
by devfn?

> +                memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
> +                       sizeof(struct cxl_phy_port_info));

Re: [PATCH 2/3] hw/cxl: Simplified Identify Switch Device & Get Physical Port State

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 5237 bytes --]

On 10/06/25 03:29PM, Jonathan Cameron wrote:
>On Mon,  2 Jun 2025 19:29:41 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> Modified Identify Switch Device (Opcode 5100h)
>> & Get Physical Port State(Opcode 5101h)
>> using physical ports info stored during enumeration
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>A few additional comments in here.
>
>J
>> ---
>>  hw/cxl/cxl-mailbox-utils.c | 133 +++++++------------------------------
>>  1 file changed, 24 insertions(+), 109 deletions(-)
>>
>> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
>> index 680055c6c0..b2fa79a721 100644
>> --- a/hw/cxl/cxl-mailbox-utils.c
>> +++ b/hw/cxl/cxl-mailbox-utils.c
>> @@ -558,17 +558,7 @@ static CXLRetCode cmd_set_response_msg_limit(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> -static void cxl_set_dsp_active_bm(PCIBus *b, PCIDevice *d,
>> -                                  void *private)
>> -{
>> -    uint8_t *bm = private;
>> -    if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
>> -        uint8_t port = PCIE_PORT(d)->port;
>> -        bm[port / 8] |= 1 << (port % 8);
>> -    }
>> -}
>> -
>> -/* CXL r3.1 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
>> +/* CXL r3.2 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
>
>I'd prefer the spec reference updates in a separate patch. They are noise here
>and kind of suggest there are real changes rather than just refactoring.
>
okay
>
>> @@ -611,16 +599,14 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
>>          out->ingress_port_id = 0;
>>      }
>>
>> -    pci_for_each_device_under_bus(bus, cxl_set_dsp_active_bm,
>> -                                  out->active_port_bitmask);
>> -    out->active_port_bitmask[usp->port / 8] |= (1 << usp->port % 8);
>
>Ah. With this in front of me the reason for the sizeing is much clearer
>than in previous patch on it's own. Combining the two will make it all more obvious.
>
right, will do the same in next iteration(V2) of the patch series.
>> -
>> +    memcpy(out->active_port_bitmask, cci->pports.active_port_bitmask,
>> +           sizeof(cci->pports.active_port_bitmask));
>>      *len_out = sizeof(*out);
>>
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> -/* CXL r3.1 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
>> +/* CXL r3.2 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
>>  static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>>                                                uint8_t *payload_in,
>>                                                size_t len_in,
>> @@ -628,44 +614,21 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>>                                                size_t *len_out,
>>                                                CXLCCI *cci)
>>  {
>
>>
>>      in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
>>      out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
>> @@ -673,72 +636,24 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>>      if (len_in < sizeof(*in)) {
>>          return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>>      }
>> -    /* Check if what was requested can fit */
>> +
>
>The check is still here... So why remove the comment?
thanks for pointing this out, will add the comment back.
>
>>      if (sizeof(*out) + sizeof(*out->ports) * in->num_ports > cci->payload_max) {
>>          return CXL_MBOX_INVALID_INPUT;
>>      }
>>
>> -    /* For success there should be a match for each requested */
>> -    out->num_ports = in->num_ports;
>> +    if (in->num_ports > cci->pports.num_ports) {
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>>
>> +    out->num_ports = in->num_ports;
>>      for (i = 0; i < in->num_ports; i++) {
>> -        struct cxl_fmapi_port_state_info_block *port;
>> -        /* First try to match on downstream port */
>> -        PCIDevice *port_dev;
>> -        uint16_t lnkcap, lnkcap2, lnksta;
>> -
>> -        port = &out->ports[i];
>> -
>> -        port_dev = pcie_find_port_by_pn(bus, in->ports[i]);
>> -        if (port_dev) { /* DSP */
>> -            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
>> -                ->devices[0];
>> -            port->config_state = 3;
>> -            if (ds_dev) {
>> -                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
>> -                    port->connected_device_type = 5; /* Assume MLD for now */
>> -                } else {
>> -                    port->connected_device_type = 1;
>> -                }
>> -            } else {
>> -                port->connected_device_type = 0;
>> +        int pn = in->ports[i];
>> +        for (int j = 0; j < PCI_DEVFN_MAX; j++) {
>> +            if (pn == cci->pports.pport_info[j].port_id) {
>
>Given port id is 0-255 and your port_info has 256 elements, why not index
>by port_id when storing them in the first place? That should reduce
>complexity of this look up.  I don't think we ever actually look up
>by devfn?
okay
>
>> +                memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
>> +                       sizeof(struct cxl_phy_port_info));

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 3/3] hw/cxl: Add Physical Port Control (Opcode 5102h)

[Arpit Kumar]

added assert-deassert PERST implementation, reset PPB
for physical port.

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c  | 128 ++++++++++++++++++++++++++++++++++++
 include/hw/cxl/cxl_device.h |   8 +++
 2 files changed, 136 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index b2fa79a721..4f09692713 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -118,12 +118,16 @@ enum {
     PHYSICAL_SWITCH = 0x51,
         #define IDENTIFY_SWITCH_DEVICE      0x0
         #define GET_PHYSICAL_PORT_STATE     0x1
+        #define PHYSICAL_PORT_CONTROL       0X2
     TUNNEL = 0x53,
         #define MANAGEMENT_COMMAND     0x0
     MHD = 0x55,
         #define GET_MHD_INFO 0x0
 };
 
+/* Assert - Deassert PERST */
+#define ASSERT_WAIT_TIME_MS 100
+
 /* link state flags */
 #define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)
 #define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
@@ -662,6 +666,114 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static struct PCIDevice *cxl_find_port_dev(uint8_t ppb_id, PCIBus *bus)
+{
+    PCIDevice *d;
+    int devfn;
+
+    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
+        d = bus->devices[devfn];
+        if (d) {
+            if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
+                uint8_t port = PCIE_PORT(d)->port;
+                if (port == ppb_id) {
+                    return d;
+                }
+            }
+        }
+    }
+    return NULL;
+}
+
+static CXLRetCode deassert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+
+    if (cci->pports.perst[pn].issued_assert_PERST) {
+        if (cci->pports.perst[pn].asrt_time == -1 && !s->hold_phase_pending) {
+            qemu_mutex_lock(&cci->pports.perst[pn].lock);
+            resettable_release_reset(obj, type);
+            cci->pports.perst[pn].issued_assert_PERST = false;
+            cci->pports.pport_info[pn].link_state_flags &=
+                ~LINK_STATE_FLAG_PERST_ASSERTED;
+            cci->pports.perst[pn].asrt_time = ASSERT_WAIT_TIME_MS;
+            qemu_mutex_unlock(&cci->pports.perst[pn].lock);
+        } else {
+            return CXL_MBOX_INTERNAL_ERROR;
+        }
+    } else {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+    return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode assert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
+{
+    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+    ResettableState *s = rc->get_state(obj);
+
+    if (cci->pports.perst[pn].issued_assert_PERST || s->exit_phase_in_progress) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    qemu_mutex_lock(&cci->pports.perst[pn].lock);
+    cci->pports.perst[pn].issued_assert_PERST = true;
+    cci->pports.pport_info[pn].link_state_flags |=
+        LINK_STATE_FLAG_PERST_ASSERTED;
+    resettable_assert_reset(obj, type);
+    qemu_mutex_unlock(&cci->pports.perst[pn].lock);
+
+    /* holding reset phase for 100ms */
+    while (cci->pports.perst[pn].asrt_time--) {
+        usleep(1000);
+    }
+    return CXL_MBOX_SUCCESS;
+}
+
+/*CXL r3.2 Section 7.6.7.1.3: Get Physical Port Control (Opcode 5102h)*/
+static CXLRetCode cmd_physical_port_control(const struct cxl_cmd *cmd,
+                                            uint8_t *payload_in,
+                                            size_t len_in,
+                                            uint8_t *payload_out,
+                                            size_t *len_out,
+                                            CXLCCI *cci)
+{
+    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+    PCIDevice *dev;
+    struct cxl_fmapi_get_physical_port_control_req_pl {
+        uint8_t PPB_ID;
+        uint8_t Ports_Op;
+    } QEMU_PACKED *in;
+
+    in = (struct cxl_fmapi_get_physical_port_control_req_pl *)payload_in;
+
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    uint8_t pn = in->PPB_ID;
+    dev = cxl_find_port_dev(pn, bus);
+    if (!dev) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    switch (in->Ports_Op) {
+    case 0:
+        assert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);
+        break;
+    case 1:
+        deassert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);
+        break;
+    case 2:
+        device_cold_reset(&dev->qdev);
+        break;
+    default:
+        return CXL_MBOX_INVALID_INPUT;
+    }
+    return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 Section 8.2.9.1.2: Background Operation Status (Opcode 0002h) */
 static CXLRetCode cmd_infostat_bg_op_sts(const struct cxl_cmd *cmd,
                                          uint8_t *payload_in,
@@ -3637,6 +3749,9 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
 void cxl_destroy_cci(CXLCCI *cci)
 {
     qemu_mutex_destroy(&cci->bg.lock);
+    for (int i = 0; i < PCI_DEVFN_MAX; i++) {
+        qemu_mutex_destroy(&cci->pports.perst[i].lock);
+    }
     cci->initialized = false;
 }
 
@@ -3866,6 +3981,8 @@ static const struct cxl_cmd cxl_cmd_set_usp_mctp[256][256] = {
         cmd_identify_switch_device, 0, 0 },
     [PHYSICAL_SWITCH][GET_PHYSICAL_PORT_STATE] = { "SWITCH_PHYSICAL_PORT_STATS",
         cmd_get_physical_port_state, ~0, 0 },
+    [PHYSICAL_SWITCH][PHYSICAL_PORT_CONTROL] = { "SWITCH_PHYSICAL_PORT_CONTROL",
+        cmd_physical_port_control, 2, 0 },
     [TUNNEL][MANAGEMENT_COMMAND] = { "TUNNEL_MANAGEMENT_COMMAND",
                                      cmd_tunnel_management_cmd, ~0, 0 },
 };
@@ -3878,4 +3995,15 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
     cci->intf = intf;
     cxl_init_cci(cci, payload_max);
     cxl_set_phy_port_info(cci); /* store port info */
+    /* physical port control */
+    for (int i = 0; i < PCI_DEVFN_MAX; i++) {
+        qemu_mutex_init(&cci->pports.perst[i].lock);
+        cci->pports.perst[i].issued_assert_PERST = false;
+        /* Assert PERST involves physical port to be in
+         * hold reset phase for minimum 100ms. No other calls
+         * are entertained until Deassert PERST command.
+         * https://patchwork.ozlabs.org/project/linux-pci/patch/20190523194409.17718-1-niklas.cassel@linaro.org/#2178369
+         */
+        cci->pports.perst[i].asrt_time = ASSERT_WAIT_TIME_MS;
+    }
 }
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index 9eb128a1e8..f877d60b39 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -146,10 +146,18 @@ struct cxl_phy_port_info {
     uint8_t supported_ld_count;
 } QEMU_PACKED;
 
+/* assert-deassert PERST */
+struct pperst {
+    bool issued_assert_PERST;
+    int asrt_time;
+    QemuMutex lock;
+};
+
 struct phy_port {
     uint8_t num_ports;
     uint8_t active_port_bitmask[0x20];
     struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
+    struct pperst perst[PCI_DEVFN_MAX];
 };
 
 /* CXL r3.1 Table 8-34: Command Return Codes */
-- 
2.34.1

Re: [PATCH 3/3] hw/cxl: Add Physical Port Control (Opcode 5102h)

[Jonathan Cameron via]

On Mon,  2 Jun 2025 19:29:42 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:


Very interesting to see support for this. It will enable a load
of additional test cases.

> added assert-deassert PERST implementation, reset PPB
> for physical port.
Added

Please also include some details of testing done and what happens.
Given I know we have some issues with reset that we haven't resolved
I'm curious if you see them here.

> 
> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>

> +/* Assert - Deassert PERST */
> +#define ASSERT_WAIT_TIME_MS 100
> +
>  /* link state flags */
>  #define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)
>  #define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
> @@ -662,6 +666,114 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static struct PCIDevice *cxl_find_port_dev(uint8_t ppb_id, PCIBus *bus)
> +{
> +    PCIDevice *d;
> +    int devfn;
> +
> +    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {

As in patch one, maybe use the for_each_pci_...  Though with the callback
needed it may end up slightly more complex that this.


> +        d = bus->devices[devfn];
> +        if (d) {
> +            if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
> +                uint8_t port = PCIE_PORT(d)->port;
I'd not bother with the local variable for this one.

                 if (PCIE_PORT(d)->port == ppb_id) {
                     return d;
                 }

> +                if (port == ppb_id) {
> +                    return d;
> +                }
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static CXLRetCode deassert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +
> +    if (cci->pports.perst[pn].issued_assert_PERST) {
> +        if (cci->pports.perst[pn].asrt_time == -1 && !s->hold_phase_pending) {

I'd flip the logic as then can return early in error case and reduce
indent of the rest.


> +            qemu_mutex_lock(&cci->pports.perst[pn].lock);

            QEMU_LOCK_GUARD(&cci->pports.prst[pn].lock);

> +            resettable_release_reset(obj, type);
> +            cci->pports.perst[pn].issued_assert_PERST = false;
> +            cci->pports.pport_info[pn].link_state_flags &=
> +                ~LINK_STATE_FLAG_PERST_ASSERTED;
> +            cci->pports.perst[pn].asrt_time = ASSERT_WAIT_TIME_MS;
> +            qemu_mutex_unlock(&cci->pports.perst[pn].lock);
and drop explicit unlock.
> +        } else {
> +            return CXL_MBOX_INTERNAL_ERROR;
> +        }
> +    } else {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +static CXLRetCode assert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
> +{
> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
> +    ResettableState *s = rc->get_state(obj);
> +
> +    if (cci->pports.perst[pn].issued_assert_PERST || s->exit_phase_in_progress) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +

WITH_QEMU_LOCK_GUARD() perhaps

> +    qemu_mutex_lock(&cci->pports.perst[pn].lock);
> +    cci->pports.perst[pn].issued_assert_PERST = true;
> +    cci->pports.pport_info[pn].link_state_flags |=
> +        LINK_STATE_FLAG_PERST_ASSERTED;
> +    resettable_assert_reset(obj, type);
> +    qemu_mutex_unlock(&cci->pports.perst[pn].lock);
> +
> +    /* holding reset phase for 100ms */
> +    while (cci->pports.perst[pn].asrt_time--) {
> +        usleep(1000);
Is this happening synchronously?  I'd kind of expect it to be a background thing
where we'd just check it had finished.
> +    }
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +/*CXL r3.2 Section 7.6.7.1.3: Get Physical Port Control (Opcode 5102h)*/
> +static CXLRetCode cmd_physical_port_control(const struct cxl_cmd *cmd,
> +                                            uint8_t *payload_in,
> +                                            size_t len_in,
> +                                            uint8_t *payload_out,
> +                                            size_t *len_out,
> +                                            CXLCCI *cci)
> +{
> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +    PCIDevice *dev;
> +    struct cxl_fmapi_get_physical_port_control_req_pl {
> +        uint8_t PPB_ID;
> +        uint8_t Ports_Op;
> +    } QEMU_PACKED *in;
> +
> +    in = (struct cxl_fmapi_get_physical_port_control_req_pl *)payload_in;

Often we cheat on these where the type is locally defined and do

    struct cxl_fmapi_get_physical_port_control_req_pl {
        uint8_t ppb_id;
        uint8_t ports_op;
    } QEMU_PACKED *in = (void *)payload_in;

Given it's all together the type isn't confusing or ambiguous even
though we use a void * instead of the more specific cast.

Note also that it is better to stick to local style and use lower_case
style for structure element naming etc.

> +
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    uint8_t pn = in->PPB_ID;
> +    dev = cxl_find_port_dev(pn, bus);
> +    if (!dev) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    switch (in->Ports_Op) {
> +    case 0:
> +        assert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);

Even for these probably

assert_perst()

> +        break;
return CXL_MBOX_SUCESS;
> +    case 1:
> +        deassert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);
> +        break;
> +    case 2:
> +        device_cold_reset(&dev->qdev);
> +        break;
> +    default:
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +    return CXL_MBOX_SUCCESS;
> +}
> +

> @@ -3878,4 +3995,15 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>      cci->intf = intf;
>      cxl_init_cci(cci, payload_max);
>      cxl_set_phy_port_info(cci); /* store port info */
> +    /* physical port control */
> +    for (int i = 0; i < PCI_DEVFN_MAX; i++) {
> +        qemu_mutex_init(&cci->pports.perst[i].lock);

perst is definitely port wise - not linked to CCI so that stuff should be
in the port structures themselves.

> +        cci->pports.perst[i].issued_assert_PERST = false;
> +        /* Assert PERST involves physical port to be in
wrap at 80 chars.
> +         * hold reset phase for minimum 100ms. No other calls
> +         * are entertained until Deassert PERST command.
> +         * https://patchwork.ozlabs.org/project/linux-pci/patch/20190523194409.17718-1-niklas.cassel@linaro.org/#2178369

Blocking other commands is fine but we should lock up emulation of other stuff
in the system and I think you currently do.

> +         */
> +        cci->pports.perst[i].asrt_time = ASSERT_WAIT_TIME_MS;
> +    }
>  }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 9eb128a1e8..f877d60b39 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -146,10 +146,18 @@ struct cxl_phy_port_info {
>      uint8_t supported_ld_count;
>  } QEMU_PACKED;
>  
> +/* assert-deassert PERST */
> +struct pperst {
> +    bool issued_assert_PERST;
> +    int asrt_time;
> +    QemuMutex lock;
> +};
> +
>  struct phy_port {
>      uint8_t num_ports;
>      uint8_t active_port_bitmask[0x20];
>      struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
> +    struct pperst perst[PCI_DEVFN_MAX];
>  };
>  
>  /* CXL r3.1 Table 8-34: Command Return Codes */

Re: [PATCH 3/3] hw/cxl: Add Physical Port Control (Opcode 5102h)

[Arpit Kumar]

[-- Attachment #1: Type: text/plain, Size: 8087 bytes --]

On 10/06/25 03:45PM, Jonathan Cameron wrote:
>On Mon,  2 Jun 2025 19:29:42 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>
>Very interesting to see support for this. It will enable a load
>of additional test cases.
>
>> added assert-deassert PERST implementation, reset PPB
>> for physical port.
>Added
okay
>
>Please also include some details of testing done and what happens.
>Given I know we have some issues with reset that we haven't resolved
>I'm curious if you see them here.
>
sure, I have tested this command using libcxl-mi. will share the details in the
next iteration (V2) of the patch series.
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>
>> +/* Assert - Deassert PERST */
>> +#define ASSERT_WAIT_TIME_MS 100
>> +
>>  /* link state flags */
>>  #define LINK_STATE_FLAG_LANE_REVERSED    (1 << 0)
>>  #define LINK_STATE_FLAG_PERST_ASSERTED   (1 << 1)
>> @@ -662,6 +666,114 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>>      return CXL_MBOX_SUCCESS;
>>  }
>>
>> +static struct PCIDevice *cxl_find_port_dev(uint8_t ppb_id, PCIBus *bus)
>> +{
>> +    PCIDevice *d;
>> +    int devfn;
>> +
>> +    for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
>
>As in patch one, maybe use the for_each_pci_...  Though with the callback
>needed it may end up slightly more complex that this.
>
right, will use the same.
>
>> +        d = bus->devices[devfn];
>> +        if (d) {
>> +            if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
>> +                uint8_t port = PCIE_PORT(d)->port;
>I'd not bother with the local variable for this one.
>
okay
>                 if (PCIE_PORT(d)->port == ppb_id) {
>                     return d;
>                 }
>
>> +                if (port == ppb_id) {
>> +                    return d;
>> +                }
>> +            }
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static CXLRetCode deassert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
>> +    ResettableState *s = rc->get_state(obj);
>> +
>> +    if (cci->pports.perst[pn].issued_assert_PERST) {
>> +        if (cci->pports.perst[pn].asrt_time == -1 && !s->hold_phase_pending) {
>
>I'd flip the logic as then can return early in error case and reduce
>indent of the rest.
>
okay, will update in V2.
>
>> +            qemu_mutex_lock(&cci->pports.perst[pn].lock);
>
>            QEMU_LOCK_GUARD(&cci->pports.prst[pn].lock);
>
>> +            resettable_release_reset(obj, type);
>> +            cci->pports.perst[pn].issued_assert_PERST = false;
>> +            cci->pports.pport_info[pn].link_state_flags &=
>> +                ~LINK_STATE_FLAG_PERST_ASSERTED;
>> +            cci->pports.perst[pn].asrt_time = ASSERT_WAIT_TIME_MS;
>> +            qemu_mutex_unlock(&cci->pports.perst[pn].lock);
>and drop explicit unlock.
got it
>> +        } else {
>> +            return CXL_MBOX_INTERNAL_ERROR;
>> +        }
>> +    } else {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +static CXLRetCode assert_PERST(Object *obj, ResetType type, uint8_t pn, CXLCCI *cci)
>> +{
>> +    ResettableClass *rc = RESETTABLE_GET_CLASS(ocpgs@samsung.combj);
>> +    ResettableState *s = rc->get_state(obj);
>> +
>> +    if (cci->pports.perst[pn].issued_assert_PERST || s->exit_phase_in_progress) {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>
>WITH_QEMU_LOCK_GUARD() perhaps
okay
>
>> +    qemu_mutex_lock(&cci->pports.perst[pn].lock);
>> +    cci->pports.perst[pn].issued_assert_PERST = true;
>> +    cci->pports.pport_info[pn].link_state_flags |=
>> +        LINK_STATE_FLAG_PERST_ASSERTED;
>> +    resettable_assert_reset(obj, type);
>> +    qemu_mutex_unlock(&cci->pports.perst[pn].lock);
>> +
>> +    /* holding reset phase for 100ms */
>> +    while (cci->pports.perst[pn].asrt_time--) {
>> +        usleep(1000);
>Is this happening synchronously?  I'd kind of expect it to be a background thing
>where we'd just check it had finished.
okay, will update it in V2 of patch series.
>> +    }
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +/*CXL r3.2 Section 7.6.7.1.3: Get Physical Port Control (Opcode 5102h)*/
>> +static CXLRetCode cmd_physical_port_control(const struct cxl_cmd *cmd,
>> +                                            uint8_t *payload_in,
>> +                                            size_t len_in,
>> +                                            uint8_t *payload_out,
>> +                                            size_t *len_out,
>> +                                            CXLCCI *cci)
>> +{
>> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +    PCIDevice *dev;
>> +    struct cxl_fmapi_get_physical_port_control_req_pl {
>> +        uint8_t PPB_ID;
>> +        uint8_t Ports_Op;
>> +    } QEMU_PACKED *in;
>> +
>> +    in = (struct cxl_fmapi_get_physical_port_control_req_pl *)payload_in;
>
>Often we cheat on these where the type is locally defined and do
>
>    struct cxl_fmapi_get_physical_port_control_req_pl {
>        uint8_t ppb_id;
>        uint8_t ports_op;
>    } QEMU_PACKED *in = (void *)payload_in;
>
>Given it's all together the type isn't confusing or ambiguous even
>though we use a void * instead of the more specific cast.
>
>Note also that it is better to stick to local style and use lower_case
>style for structure element naming etc.
>
got it
>> +
>> +    if (len_in < sizeof(*in)) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>> +
>> +    uint8_t pn = in->PPB_ID;
>> +    dev = cxl_find_port_dev(pn, bus);
>> +    if (!dev) {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    switch (in->Ports_Op) {
>> +    case 0:
>> +        assert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);
>
>Even for these probably
>
>assert_perst()
>
okay
>> +        break;
>return CXL_MBOX_SUCESS;
>> +    case 1:
>> +        deassert_PERST(OBJECT(&dev->qdev), RESET_TYPE_COLD, pn, cci);
>> +        break;
>> +    case 2:
>> +        device_cold_reset(&dev->qdev);
>> +        break;
>> +    default:
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>
>> @@ -3878,4 +3995,15 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>>      cci->intf = intf;
>>      cxl_init_cci(cci, payload_max);
>>      cxl_set_phy_port_info(cci); /* store port info */
>> +    /* physical port control */
>> +    for (int i = 0; i < PCI_DEVFN_MAX; i++) {
>> +        qemu_mutex_init(&cci->pports.perst[i].lock);
>
>perst is definitely port wise - not linked to CCI so that stuff should be
>in the port structures themselves.
>
>> +        cci->pports.perst[i].issued_assert_PERST = false;
>> +        /* Assert PERST involves physical port to be in
>wrap at 80 chars.
okay
>> +         * hold reset phase for minimum 100ms. No other calls
>> +         * are entertained until Deassert PERST command.
>> +         * https://patchwork.ozlabs.org/project/linux-pci/patch/20190523194409.17718-1-niklas.cassel@linaro.org/#2178369
>
>Blocking other commands is fine but we should lock up emulation of other stuff
>in the system and I think you currently do.
>
got it. I think you meant 'we should *not lock up..'
>> +         */
>> +        cci->pports.perst[i].asrt_time = ASSERT_WAIT_TIME_MS;
>> +    }
>>  }
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index 9eb128a1e8..f877d60b39 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -146,10 +146,18 @@ struct cxl_phy_port_info {
>>      uint8_t supported_ld_count;
>>  } QEMU_PACKED;
>>
>> +/* assert-deassert PERST */
>> +struct pperst {
>> +    bool issued_assert_PERST;
>> +    int asrt_time;
>> +    QemuMutex lock;
>> +};
>> +
>>  struct phy_port {
>>      uint8_t num_ports;
>>      uint8_t active_port_bitmask[0x20];
>>      struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
>> +    struct pperst perst[PCI_DEVFN_MAX];
>>  };
>>
>>  /* CXL r3.1 Table 8-34: Command Return Codes */
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] hw/cxl: Add Physical Port Control (Opcode 5102h)

[Jonathan Cameron via]

> >> +        /* Assert PERST involves physical port to be in  
> >wrap at 80 chars.  
> okay
> >> +         * hold reset phase for minimum 100ms. No other calls
> >> +         * are entertained until Deassert PERST command.
> >> +         * https://patchwork.ozlabs.org/project/linux-pci/patch/20190523194409.17718-1-niklas.cassel@linaro.org/#2178369  
> >
> >Blocking other commands is fine but we should lock up emulation of other stuff
> >in the system and I think you currently do.
> >  
> got it. I think you meant 'we should *not lock up..'
Absolutely!