[PATCH v2 0/2] FM-API Physical switch command set support

[PATCH v2 0/2] FM-API Physical switch command set support

[Arpit Kumar]

This patch series refactor existing support for Identify Switch Device
and Get Physical Port State by utilizing physical ports (USP & DSP)
information stored during enumeration. Additionally, it introduces new
support for Physical Port Control of FM-API based physical switch
command set as per CXL spec r3.2 Table 8-230:Physical Switch.

This v2 patch series addresses feedback from v1 and incorporates some 
new changes. It merges [PATCH 1/3] and [PATCH 2/3] from the v1 series,
resulting in total of two patches in this patch series.

Change log [PATCH v2 1/2]:
-namespace defines intead of enum for current_port_config_state.
-Relocates all defines & enums to include/hw/cxl/cxl_device.h for
 improved readibility.
-Total number of unique physical ports defined as 256 since the length
 of number of physical ports given in CXL r3.2 Table 7-16 is 1 byte. Thus,
 dropping any dependencies on virtual heirarchy throughout the implementation
 as the request is per switch.
-Moves struct phy_port to CXLUpstreamPort avoiding cci specific handling of
 physical ports hence making it more reasonable.
-Utilizes pci_for_each_device_under_bus() to store downstream ports info.
-Declarations at the beginning of the function.

Change log [PATCH v2 2/2]:
-cxl_find_port_dev() now locates device objects for both USP and DSP as physical
 port control request includes all physical ports.
-Replaces qemu_mutex_lock with QEMU_LOCK_GUARD.
-Holding reset phase for 100ms added as backgroung operation using qemu_thread_create().
-Updates naming function/variables names as suggested by using lower_case style
 wherever apt.
-Removes CCI sepcific implementation for Assert-Deassert PERST & Reset as it is port
 wise and utilize struct phy_port stored in CXLUpstreamPort as per [PATCH 1/2].
-Changes made does not lock up other emulations.

Logic behind physical port control request:
-Assert-Deassert PERST: Assert PERST involves physical port to be in hold reset phase
 for minimum 100ms. No other physical port control request are entertained until Deassert
 PERST command for the given port is issued.
-Reset PPB: cold reset of physical port (completing enter->hold->exit phases).

The Physcial port control request is tested through libcxl-mi interface. It tests 
all active ports and all opcodes per active port. Since it does not support run time input,
all other possible edge cases were tested manually:
https://github.com/computexpresslink/libcxlmi/pull/31

Qemu Topology used and it's results:
-without any devices connected to downstream ports - success
-with virtio-rng-pci devices connected to downstream ports - success
-with CXLType3 devices connected to downstream ports - failure
-with different unique values of ports (both upstream and downstream) - success

Below given topology is an example topology with 3 downstream ports and 1 upstream port.
It has one virtio-rng-pci device connected to downstream port 4.
FM="-object memory-backend-file,id=cxl-mem1,mem-path=$TMP_DIR/t3_cxl1.raw,size=256M \
    -object memory-backend-file,id=cxl-lsa1,mem-path=$TMP_DIR/t3_lsa1.raw,size=1M \
    -object memory-backend-file,id=cxl-mem2,mem-path=$TMP_DIR/t3_cxl2.raw,size=16M \
    -object memory-backend-file,id=cxl-lsa2,mem-path=$TMP_DIR/t3_lsa2.raw,size=1M \
    -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1,hdm_for_passthrough=true \
    -device cxl-rp,port=0,bus=cxl.1,id=cxl_rp_port0,chassis=0,slot=2 \
    -device cxl-upstream,port=2,sn=1234,bus=cxl_rp_port0,id=us0,addr=0.0,multifunction=on, \
    -device cxl-switch-mailbox-cci,bus=cxl_rp_port0,addr=0.1,target=us0 \
    -device cxl-downstream,port=0,bus=us0,id=swport0,chassis=0,slot=4 \
    -device cxl-downstream,port=4,bus=us0,id=swport1,chassis=0,slot=5 \
    -device cxl-downstream,port=8,bus=us0,id=swport2,chassis=0,slot=6 \
    -machine cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=4G,cxl-fmw.0.interleave-granularity=1k \
    -device i2c_mctp_cxl,bus=aspeed.i2c.bus.0,address=4,target=us0 \
    -device i2c_mctp_cxl,bus=aspeed.i2c.bus.0,address=5,target=us0 \
    -device i2c_mctp_cxl,bus=aspeed.i2c.bus.0,address=6,target=us0 \
    -device virtio-rng-pci,bus=swport1"
   
Observation/Finding:
The assert-deassert and reset PPB operation fails when downstream ports are connected
to a CXLType3 device as it would mean device reset instead of switch port reset.

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 (2):
  hw/cxl: Refactored Identify Switch Device & Get Physical Port State
  hw/cxl: Add Physical Port Control (Opcode 5102h)

 hw/cxl/cxl-mailbox-utils.c                | 364 +++++++++++++++-------
 include/hw/cxl/cxl_device.h               |  92 ++++++
 include/hw/pci-bridge/cxl_upstream_port.h |   4 +
 3 files changed, 352 insertions(+), 108 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] hw/cxl: Refactored Identify Switch Device & Get Physical Port State

[Arpit Kumar]

-Storing physical ports info during enumeration.
-Refactored changes using physical ports info for
 Identify Switch Device (Opcode 5100h) & Get Physical Port State
 (Opcode 5101h) physical switch FM-API command set.

Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
---
 hw/cxl/cxl-mailbox-utils.c                | 229 ++++++++++++----------
 include/hw/cxl/cxl_device.h               |  82 ++++++++
 include/hw/pci-bridge/cxl_upstream_port.h |   4 +
 3 files changed, 207 insertions(+), 108 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index a02d130926..c4e83fb2aa 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -500,16 +500,6 @@ 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) */
 static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
                                              uint8_t *payload_in,
@@ -518,9 +508,8 @@ 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);
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+    uint8_t num_phys_ports = pp->pports.num_ports;
 
     struct cxl_fmapi_ident_switch_dev_resp_pl {
         uint8_t ingress_port_id;
@@ -537,11 +526,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,
     };
 
@@ -553,16 +542,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, pp->pports.active_port_bitmask,
+           sizeof(pp->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,
@@ -570,44 +557,22 @@ 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 */
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+    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;
@@ -620,69 +585,20 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
         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 > pp->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;
-            }
-            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 {
+        int pn = in->ports[i];
+        if (pp->pports.pport_info[pn].port_id != pn) {
             return CXL_MBOX_INVALID_INPUT;
+        } else {
+            memcpy(&out->ports[i], &(pp->pports.pport_info[pn]),
+                   sizeof(struct cxl_phy_port_info));
         }
-
-        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;
     *len_out = pl_size;
 
@@ -3686,6 +3602,101 @@ 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(CXLUpstreamPort *ports, int pnum,
+                                    CXLCCI *cci)
+{
+    uint16_t lnkcap, lnkcap2, lnksta;
+    PCIBus *bus;
+    PCIDevice *port_dev;
+    PCIEPort *usp = PCIE_PORT(cci->d);
+
+    if (usp->port == pnum) {
+        port_dev = PCI_DEVICE(usp);
+        ports->pports.pport_info[pnum].current_port_config_state =
+            CXL_PORT_CONFIG_STATE_USP;
+        ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;
+    } else {
+        bus = &PCI_BRIDGE(cci->d)->sec_bus;
+        port_dev = pcie_find_port_by_pn(bus, pnum);
+        if (port_dev) { /* DSP */
+            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
+                ->devices[0];
+            ports->pports.pport_info[pnum].current_port_config_state =
+               CXL_PORT_CONFIG_STATE_DSP;
+            if (ds_dev) {
+                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
+                    /* To-do: controllable */
+                    ports->pports.pport_info[pnum].connected_device_type =
+                        CXL_TYPE_3_SLD;
+                } else {
+                    ports->pports.pport_info[pnum].connected_device_type = PCIE_DEVICE;
+                }
+            } else {
+                ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;
+            }
+            ports->pports.pport_info[pnum].supported_ld_count = 3;
+        } 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->pports.pport_info[pnum].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
+    ports->pports.pport_info[pnum].negotiated_link_width =
+        (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
+    ports->pports.pport_info[pnum].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
+    ports->pports.pport_info[pnum].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
+    ports->pports.pport_info[pnum].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
+
+    ports->pports.pport_info[pnum].port_id = pnum;
+    ports->pports.active_port_bitmask[pnum / 8] |= (1 << pnum % 8);
+    ports->pports.pport_info[pnum].ltssm_state = LTSSM_L2;
+    ports->pports.pport_info[pnum].first_negotiated_lane_num = 0;
+    ports->pports.pport_info[pnum].link_state_flags = 0;
+    ports->pports.pport_info[pnum].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
+    ports->pports.pport_info[pnum].connected_device_mode = STANDARD_256B_FLIT_MODE;
+
+    return CXL_MBOX_SUCCESS;
+}
+
+static void cxl_set_dsp_port(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    CXLCCI *cci = (CXLCCI *)opaque;
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+
+    if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)) {
+        uint8_t phy_port_num = PCIE_PORT(dev)->port;
+        cxl_set_port_type(pp, phy_port_num, cci);
+    }
+}
+
+static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
+{
+    PCIEPort *usp = PCIE_PORT(cci->d);
+    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
+    pp->pports.num_ports = num_phys_ports;
+    uint8_t phy_port_num =  usp->port;
+
+    cxl_set_port_type(pp, phy_port_num, cci); /* USP */
+    pci_for_each_device_under_bus(bus, cxl_set_dsp_port, cci); /* DSP */
+
+    return CXL_MBOX_SUCCESS;
+}
+
 void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
                                   DeviceState *d, size_t payload_max)
 {
@@ -3693,6 +3704,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);
 }
 
 void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
@@ -3797,4 +3809,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);
 }
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index ca515cab13..1fa6cf7536 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -127,6 +127,88 @@
                   CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
                   (1 << 16))
 
+#define CXL_MAX_PHY_PORTS 256
+
+#define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
+#define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
+#define LINK_STATE_FLAG_PRSNT            BIT(2)
+#define LINK_STATE_FLAG_POWER_OFF        BIT(3)
+
+/* physical port control info - CXL r3.2 table 7-19 */
+#define CXL_PORT_CONFIG_STATE_DISABLED           0x0
+#define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1
+#define CXL_PORT_CONFIG_STATE_UNBIND_IN_PROGRESS 0x2
+#define CXL_PORT_CONFIG_STATE_DSP                0x3
+#define CXL_PORT_CONFIG_STATE_USP                0x4
+#define CXL_PORT_CONFIG_STATE_FABRIC_PORT        0x5
+#define CXL_PORT_CONFIG_STATE_INVALID_PORT_ID    0xF
+
+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;
+
+/* 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[CXL_MAX_PHY_PORTS];
+};
+
 /* CXL r3.1 Table 8-34: Command Return Codes */
 typedef enum {
     CXL_MBOX_SUCCESS = 0x0,
diff --git a/include/hw/pci-bridge/cxl_upstream_port.h b/include/hw/pci-bridge/cxl_upstream_port.h
index db1dfb6afd..bcd3002cf8 100644
--- a/include/hw/pci-bridge/cxl_upstream_port.h
+++ b/include/hw/pci-bridge/cxl_upstream_port.h
@@ -4,6 +4,7 @@
 #include "hw/pci/pcie.h"
 #include "hw/pci/pcie_port.h"
 #include "hw/cxl/cxl.h"
+#include "include/hw/cxl/cxl_device.h"
 
 typedef struct CXLUpstreamPort {
     /*< private >*/
@@ -23,6 +24,9 @@ typedef struct CXLUpstreamPort {
 
     DOECap doe_cdat;
     uint64_t sn;
+
+    /*< physical ports information >*/
+    struct phy_port pports;
 } CXLUpstreamPort;
 
 #endif /* CXL_SUP_H */
-- 
2.34.1

Re: [PATCH v2 1/2] hw/cxl: Refactored Identify Switch Device & Get Physical Port State

[Jonathan Cameron]

On Thu, 10 Jul 2025 20:13:37 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

> -Storing physical ports info during enumeration.
> -Refactored changes using physical ports info for
>  Identify Switch Device (Opcode 5100h) & Get Physical Port State
>  (Opcode 5101h) physical switch FM-API command set.
> 
> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Hi Arpit,

Sorry it took me a while to get to this. I've pretty much forgotten
the v1, so taking a full fresh look.


Various minor comments inline.

Jonathan

> ---
>  hw/cxl/cxl-mailbox-utils.c                | 229 ++++++++++++----------
>  include/hw/cxl/cxl_device.h               |  82 ++++++++
>  include/hw/pci-bridge/cxl_upstream_port.h |   4 +
>  3 files changed, 207 insertions(+), 108 deletions(-)
> 


> @@ -620,69 +585,20 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>          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 > pp->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;
> -            }
> -            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 {
> +        int pn = in->ports[i];

Blank line after declarations.

> +        if (pp->pports.pport_info[pn].port_id != pn) {
>              return CXL_MBOX_INVALID_INPUT;
> +        } else {

Returned in the if, so no need for an else here.

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

>  
> +static CXLRetCode cxl_set_port_type(CXLUpstreamPort *ports, int pnum,
> +                                    CXLCCI *cci)
> +{
> +    uint16_t lnkcap, lnkcap2, lnksta;
> +    PCIBus *bus;
> +    PCIDevice *port_dev;
> +    PCIEPort *usp = PCIE_PORT(cci->d);
> +
> +    if (usp->port == pnum) {
> +        port_dev = PCI_DEVICE(usp);
> +        ports->pports.pport_info[pnum].current_port_config_state =
> +            CXL_PORT_CONFIG_STATE_USP;
> +        ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;

Use local variables for connected_device_type and supported_ld_count;
Then...

> +    } else {
> +        bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +        port_dev = pcie_find_port_by_pn(bus, pnum);
> +        if (port_dev) { /* DSP */
> +            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
> +                ->devices[0];
> +            ports->pports.pport_info[pnum].current_port_config_state =
> +               CXL_PORT_CONFIG_STATE_DSP;
> +            if (ds_dev) {
> +                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
> +                    /* To-do: controllable */
> +                    ports->pports.pport_info[pnum].connected_device_type =
> +                        CXL_TYPE_3_SLD;
> +                } else {
> +                    ports->pports.pport_info[pnum].connected_device_type = PCIE_DEVICE;
> +                }
> +            } else {
> +                ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;
> +            }
> +            ports->pports.pport_info[pnum].supported_ld_count = 3;
> +        } 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));
> +
Can fill this whole thing with
    ports->pports.pport_info[pnum] = (CXLPhysicalPortInfo) {
       .connected_device_type = connected_device_type,
       .supported_ld_count = supported_ld_count,
       .max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4,
etc.
   };
> +    ports->pports.pport_info[pnum].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
> +    ports->pports.pport_info[pnum].negotiated_link_width =
> +        (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
> +    ports->pports.pport_info[pnum].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
> +    ports->pports.pport_info[pnum].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
> +    ports->pports.pport_info[pnum].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
> +
> +    ports->pports.pport_info[pnum].port_id = pnum;
> +    ports->pports.active_port_bitmask[pnum / 8] |= (1 << pnum % 8);
> +    ports->pports.pport_info[pnum].ltssm_state = LTSSM_L2;
> +    ports->pports.pport_info[pnum].first_negotiated_lane_num = 0;
> +    ports->pports.pport_info[pnum].link_state_flags = 0;
> +    ports->pports.pport_info[pnum].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
> +    ports->pports.pport_info[pnum].connected_device_mode = STANDARD_256B_FLIT_MODE;
> +
> +    return CXL_MBOX_SUCCESS;
> +}
> +
> +static void cxl_set_dsp_port(PCIBus *bus, PCIDevice *dev, void *opaque)
> +{
> +    CXLCCI *cci = (CXLCCI *)opaque;
> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
> +
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)) {
> +        uint8_t phy_port_num = PCIE_PORT(dev)->port;

Does the local variable add anything over

        cxl_set_port_type(pp, PCIE_PORT(dev)->port, cci);

> +        cxl_set_port_type(pp, phy_port_num, cci);
> +    }
> +}
> +
> +static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
> +{
> +    PCIEPort *usp = PCIE_PORT(cci->d);
> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
> +    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
> +    pp->pports.num_ports = num_phys_ports;
> +    uint8_t phy_port_num =  usp->port;

Seems to be an extra space after =

> +
> +    cxl_set_port_type(pp, phy_port_num, cci); /* USP */
> +    pci_for_each_device_under_bus(bus, cxl_set_dsp_port, cci); /* DSP */
> +
> +    return CXL_MBOX_SUCCESS;
> +}

> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index ca515cab13..1fa6cf7536 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -127,6 +127,88 @@
>                    CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
>                    (1 << 16))
>  
> +#define CXL_MAX_PHY_PORTS 256
> +
> +#define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
> +#define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
> +#define LINK_STATE_FLAG_PRSNT            BIT(2)
> +#define LINK_STATE_FLAG_POWER_OFF        BIT(3)
> +
> +/* physical port control info - CXL r3.2 table 7-19 */
> +#define CXL_PORT_CONFIG_STATE_DISABLED           0x0
> +#define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1
> +#define CXL_PORT_CONFIG_STATE_UNBIND_IN_PROGRESS 0x2
> +#define CXL_PORT_CONFIG_STATE_DSP                0x3
> +#define CXL_PORT_CONFIG_STATE_USP                0x4
> +#define CXL_PORT_CONFIG_STATE_FABRIC_PORT        0x5
> +#define CXL_PORT_CONFIG_STATE_INVALID_PORT_ID    0xF
> +
> +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;

Similar to below. Enums make sense when we actually
use the type.  Apply this comment to all the others above.


> +
> +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;

No Underscore.  Qemu enum type naming is CammelCase
However I'm not a fan of an enum for this stuff unless we
actually use that type to enforce something.

I'd just use a set of defines.  However they'll want
some suitable prefix.  They are PCI terms but I can't
find a similar assignment of values in the PCI spec, so
CXL_LTSMM_x I think.



> +
> +/* CXL r3.2 Table 7-19: Port Info */
> +struct cxl_phy_port_info {
typedef CXLPhyPortInfo {
} CXLPhyPortInfo;
See comment on QEMU use of typedefs below. 

We aren't always doing this right in the CXL code, but lets not make it worse!
> +    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 {

Should prefix this I think and it looks to be plural so
struct cxl_phy_ports maybe?

Actually given it's exposed outside one file we should follow
QEMU naming style and a typedef.
https://qemu-project.gitlab.io/qemu/devel/style.html#typedefs

CXLPhysicalPorts perhaps? 

Or thinking more, do we need this definition at all as it
only gets instantiated in CXLUpstreamPort
See below.

> +    uint8_t num_ports;
> +    uint8_t active_port_bitmask[0x20];

Can we derive that 0x20 ?  I'm guessing it's CXL_MAX_PHY_PORTS / BITS_PER_BYTE

> +    struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
> +};
> +
>  /* CXL r3.1 Table 8-34: Command Return Codes */
>  typedef enum {
>      CXL_MBOX_SUCCESS = 0x0,
> diff --git a/include/hw/pci-bridge/cxl_upstream_port.h b/include/hw/pci-bridge/cxl_upstream_port.h
> index db1dfb6afd..bcd3002cf8 100644
> --- a/include/hw/pci-bridge/cxl_upstream_port.h
> +++ b/include/hw/pci-bridge/cxl_upstream_port.h
> @@ -4,6 +4,7 @@
>  #include "hw/pci/pcie.h"
>  #include "hw/pci/pcie_port.h"
>  #include "hw/cxl/cxl.h"
> +#include "include/hw/cxl/cxl_device.h"
>  
>  typedef struct CXLUpstreamPort {
>      /*< private >*/
> @@ -23,6 +24,9 @@ typedef struct CXLUpstreamPort {
>  
>      DOECap doe_cdat;
>      uint64_t sn;
> +
> +    /*< physical ports information >*/
> +    struct phy_port pports;

No need for type I think.
    struct {
        uint8_t num;
        uint8_t active_bitmask[CXL_MAX_PHY_PORTS / BITS_PER_BYTE];
        CXLPhyPortInto info;
    } pports;

As they all only exists as part of pports, no need say they are phy
ports related in the parameter names.

>  } CXLUpstreamPort;
>  
>  #endif /* CXL_SUP_H */

Re: [PATCH v2 1/2] hw/cxl: Refactored Identify Switch Device & Get Physical Port State

[Arpit Kumar]

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

On 25/07/25 03:54PM, Jonathan Cameron wrote:
>On Thu, 10 Jul 2025 20:13:37 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> -Storing physical ports info during enumeration.
>> -Refactored changes using physical ports info for
>>  Identify Switch Device (Opcode 5100h) & Get Physical Port State
>>  (Opcode 5101h) physical switch FM-API command set.
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>Hi Arpit,
>
>Sorry it took me a while to get to this. I've pretty much forgotten
>the v1, so taking a full fresh look.
>
>
>Various minor comments inline.
>
>Jonathan
>
Hi Jonathan,

Thanks for the review. Will append the changes as suggested in the next
iteration (V3) of the patch series.

Thanks,
Arpit
>> ---
>>  hw/cxl/cxl-mailbox-utils.c                | 229 ++++++++++++----------
>>  include/hw/cxl/cxl_device.h               |  82 ++++++++
>>  include/hw/pci-bridge/cxl_upstream_port.h |   4 +
>>  3 files changed, 207 insertions(+), 108 deletions(-)
>>
>
>
>> @@ -620,69 +585,20 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>>          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 > pp->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;
>> -            }
>> -            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 {
>> +        int pn = in->ports[i];
>
>Blank line after declarations.
>
>> +        if (pp->pports.pport_info[pn].port_id != pn) {
>>              return CXL_MBOX_INVALID_INPUT;
>> +        } else {
>
>Returned in the if, so no need for an else here.
>
Thanks for pointing out.
>> +            memcpy(&out->ports[i], &(pp->pports.pport_info[pn]),
>> +                   sizeof(struct cxl_phy_port_info));
>>          }
>
>>
>> +static CXLRetCode cxl_set_port_type(CXLUpstreamPort *ports, int pnum,
>> +                                    CXLCCI *cci)
>> +{
>> +    uint16_t lnkcap, lnkcap2, lnksta;
>> +    PCIBus *bus;
>> +    PCIDevice *port_dev;
>> +    PCIEPort *usp = PCIE_PORT(cci->d);
>> +
>> +    if (usp->port == pnum) {
>> +        port_dev = PCI_DEVICE(usp);
>> +        ports->pports.pport_info[pnum].current_port_config_state =
>> +            CXL_PORT_CONFIG_STATE_USP;
>> +        ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;
>
>Use local variables for connected_device_type and supported_ld_count;
>Then...
>

>> +    } else {
>> +        bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +        port_dev = pcie_find_port_by_pn(bus, pnum);
>> +        if (port_dev) { /* DSP */
>> +            PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
>> +                ->devices[0];
>> +            ports->pports.pport_info[pnum].current_port_config_state =
>> +               CXL_PORT_CONFIG_STATE_DSP;
>> +            if (ds_dev) {
>> +                if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
>> +                    /* To-do: controllable */
>> +                    ports->pports.pport_info[pnum].connected_device_type =
>> +                        CXL_TYPE_3_SLD;
>> +                } else {
>> +                    ports->pports.pport_info[pnum].connected_device_type = PCIE_DEVICE;
>> +                }
>> +            } else {
>> +                ports->pports.pport_info[pnum].connected_device_type = NO_DEVICE_DETECTED;
>> +            }
>> +            ports->pports.pport_info[pnum].supported_ld_count = 3;
>> +        } 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));
>> +
>Can fill this whole thing with
>    ports->pports.pport_info[pnum] = (CXLPhysicalPortInfo) {
>       .connected_device_type = connected_device_type,
>       .supported_ld_count = supported_ld_count,
>       .max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4,
>etc.
>   };
got it.
>> +    ports->pports.pport_info[pnum].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
>> +    ports->pports.pport_info[pnum].negotiated_link_width =
>> +        (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
>> +    ports->pports.pport_info[pnum].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
>> +    ports->pports.pport_info[pnum].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
>> +    ports->pports.pport_info[pnum].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
>> +
>> +    ports->pports.pport_info[pnum].port_id = pnum;
>> +    ports->pports.active_port_bitmask[pnum / 8] |= (1 << pnum % 8);
>> +    ports->pports.pport_info[pnum].ltssm_state = LTSSM_L2;
>> +    ports->pports.pport_info[pnum].first_negotiated_lane_num = 0;
>> +    ports->pports.pport_info[pnum].link_state_flags = 0;
>> +    ports->pports.pport_info[pnum].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
>> +    ports->pports.pport_info[pnum].connected_device_mode = STANDARD_256B_FLIT_MODE;
>> +
>> +    return CXL_MBOX_SUCCESS;
>> +}
>> +
>> +static void cxl_set_dsp_port(PCIBus *bus, PCIDevice *dev, void *opaque)
>> +{
>> +    CXLCCI *cci = (CXLCCI *)opaque;
>> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
>> +
>> +    if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)) {
>> +        uint8_t phy_port_num = PCIE_PORT(dev)->port;
>
>Does the local variable add anything over
>
>        cxl_set_port_type(pp, PCIE_PORT(dev)->port, cci);
>
got it.
>> +        cxl_set_port_type(pp, phy_port_num, cci);
>> +    }
>> +}
>> +
>> +static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
>> +{
>> +    PCIEPort *usp = PCIE_PORT(cci->d);
>> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
>> +    int num_phys_ports = pcie_count_ds_ports(bus) + 1;
>> +    pp->pports.num_ports = num_phys_ports;
>> +    uint8_t phy_port_num =  usp->port;
>
>Seems to be an extra space after =
>
thanks for pointing out.
>> +
>> +    cxl_set_port_type(pp, phy_port_num, cci); /* USP */
>> +    pci_for_each_device_under_bus(bus, cxl_set_dsp_port, cci); /* DSP */
>> +
>> +    return CXL_MBOX_SUCCESS;
>> +}
>
>> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
>> index ca515cab13..1fa6cf7536 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -127,6 +127,88 @@
>>                    CXL_NUM_CHMU_INSTANCES * (1 << 16),                   \
>>                    (1 << 16))
>>
>> +#define CXL_MAX_PHY_PORTS 256
>> +
>> +#define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
>> +#define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
>> +#define LINK_STATE_FLAG_PRSNT            BIT(2)
>> +#define LINK_STATE_FLAG_POWER_OFF        BIT(3)
>> +
>> +/* physical port control info - CXL r3.2 table 7-19 */
>> +#define CXL_PORT_CONFIG_STATE_DISABLED           0x0
>> +#define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS   0x1
>> +#define CXL_PORT_CONFIG_STATE_UNBIND_IN_PROGRESS 0x2
>> +#define CXL_PORT_CONFIG_STATE_DSP                0x3
>> +#define CXL_PORT_CONFIG_STATE_USP                0x4
>> +#define CXL_PORT_CONFIG_STATE_FABRIC_PORT        0x5
>> +#define CXL_PORT_CONFIG_STATE_INVALID_PORT_ID    0xF
>> +
>> +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;
>
>Similar to below. Enums make sense when we actually
>use the type.  Apply this comment to all the others above.
>
>
okay, got it.
>> +
>> +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;
>
>No Underscore.  Qemu enum type naming is CammelCase
>However I'm not a fan of an enum for this stuff unless we
>actually use that type to enforce something.
>
>I'd just use a set of defines.  However they'll want
>some suitable prefix.  They are PCI terms but I can't
>find a similar assignment of values in the PCI spec, so
>CXL_LTSMM_x I think.
>
>
>
got it, thanks for the info.
>> +
>> +/* CXL r3.2 Table 7-19: Port Info */
>> +struct cxl_phy_port_info {
>typedef CXLPhyPortInfo {
>} CXLPhyPortInfo;
>See comment on QEMU use of typedefs below.
>
>We aren't always doing this right in the CXL code, but lets not make it worse!
>> +    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 {
>
>Should prefix this I think and it looks to be plural so
>struct cxl_phy_ports maybe?
>
okay.
>Actually given it's exposed outside one file we should follow
>QEMU naming style and a typedef.
>https://qemu-project.gitlab.io/qemu/devel/style.html#typedefs
>
>CXLPhysicalPorts perhaps?
>
>Or thinking more, do we need this definition at all as it
>only gets instantiated in CXLUpstreamPort
>See below.
>
Thanks for the reference.
>> +    uint8_t num_ports;
>> +    uint8_t active_port_bitmask[0x20];
>
>Can we derive that 0x20 ?  I'm guessing it's CXL_MAX_PHY_PORTS / BITS_PER_BYTE
>
got it.
>> +    struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
>> +};
>> +
>>  /* CXL r3.1 Table 8-34: Command Return Codes */
>>  typedef enum {
>>      CXL_MBOX_SUCCESS = 0x0,
>> diff --git a/include/hw/pci-bridge/cxl_upstream_port.h b/include/hw/pci-bridge/cxl_upstream_port.h
>> index db1dfb6afd..bcd3002cf8 100644
>> --- a/include/hw/pci-bridge/cxl_upstream_port.h
>> +++ b/include/hw/pci-bridge/cxl_upstream_port.h
>> @@ -4,6 +4,7 @@
>>  #include "hw/pci/pcie.h"
>>  #include "hw/pci/pcie_port.h"
>>  #include "hw/cxl/cxl.h"
>> +#include "include/hw/cxl/cxl_device.h"
>>
>>  typedef struct CXLUpstreamPort {
>>      /*< private >*/
>> @@ -23,6 +24,9 @@ typedef struct CXLUpstreamPort {
>>
>>      DOECap doe_cdat;
>>      uint64_t sn;
>> +
>> +    /*< physical ports information >*/
>> +    struct phy_port pports;
>
>No need for type I think.
>    struct {
>        uint8_t num;
>        uint8_t active_bitmask[CXL_MAX_PHY_PORTS / BITS_PER_BYTE];
>        CXLPhyPortInto info;
>    } pports;
>
>As they all only exists as part of pports, no need say they are phy
>ports related in the parameter names.
>
got it.
>>  } CXLUpstreamPort;
>>
>>  #endif /* CXL_SUP_H */
>

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

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

[Arpit Kumar]

-added assert-deassert PERST implementation
 for physical ports (both USP and DSP's).
-assert PERST involves bg operation for holding 100ms.
-reset PPB implementation for physical ports.

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

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index c4e83fb2aa..3aa8bd14b9 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -118,6 +118,7 @@ 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,
@@ -605,6 +606,121 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static void *bg_assertcb(void *opaque)
+{
+    struct pperst *perst = opaque;
+
+    /* holding reset phase for 100ms */
+    while (perst->asrt_time--) {
+        usleep(1000);
+    }
+    perst->issued_assert_perst = true;
+    return NULL;
+}
+
+static CXLRetCode deassert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
+{
+    if (!pp->pports.perst[pn].issued_assert_perst) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    QEMU_LOCK_GUARD(&pp->pports.perst[pn].lock);
+    resettable_release_reset(obj, RESET_TYPE_COLD);
+    pp->pports.perst[pn].issued_assert_perst = false;
+    pp->pports.pport_info[pn].link_state_flags &=
+        ~LINK_STATE_FLAG_PERST_ASSERTED;
+    pp->pports.perst[pn].asrt_time = ASSERT_WAIT_TIME_MS;
+
+    return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode assert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
+{
+    if (pp->pports.perst[pn].issued_assert_perst ||
+        pp->pports.perst[pn].asrt_time < ASSERT_WAIT_TIME_MS) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    QEMU_LOCK_GUARD(&pp->pports.perst[pn].lock);
+    pp->pports.pport_info[pn].link_state_flags |=
+        LINK_STATE_FLAG_PERST_ASSERTED;
+    resettable_assert_reset(obj, RESET_TYPE_COLD);
+    qemu_thread_create(&pp->pports.perst[pn].asrt_thread, "assert_thread",
+        bg_assertcb, &pp->pports.perst[pn], QEMU_THREAD_DETACHED);
+
+    return CXL_MBOX_SUCCESS;
+}
+
+static struct PCIDevice *cxl_find_port_dev(uint8_t pn, CXLCCI *cci)
+{
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+
+    if (pp->pports.pport_info[pn].current_port_config_state ==
+        CXL_PORT_CONFIG_STATE_USP) {
+        PCIDevice *usp_dev = pci_bridge_get_device(bus);
+        return usp_dev;
+    }
+
+    if (pp->pports.pport_info[pn].current_port_config_state ==
+        CXL_PORT_CONFIG_STATE_DSP) {
+        PCIDevice *dsp_dev = pcie_find_port_by_pn(bus, pn);
+        return dsp_dev;
+    }
+    return NULL;
+}
+
+/* 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)
+{
+   CXLUpstreamPort *pp = CXL_USP(cci->d);
+   PCIDevice *dev;
+   uint8_t ret = CXL_MBOX_SUCCESS;
+
+   struct cxl_fmapi_get_physical_port_control_req_pl {
+        uint8_t ppb_id;
+        uint8_t ports_op;
+    } QEMU_PACKED *in = (void *)payload_in;
+
+    if (len_in < sizeof(*in)) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    uint8_t pn = in->ppb_id;
+    if (pp->pports.pport_info[pn].port_id != pn) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    dev = cxl_find_port_dev(pn, cci);
+    if (!dev) {
+        return CXL_MBOX_INTERNAL_ERROR;
+    }
+
+    switch (in->ports_op) {
+    case 0:
+        ret = assert_perst(OBJECT(&dev->qdev), pn, pp);
+        break;
+    case 1:
+        ret = deassert_perst(OBJECT(&dev->qdev), pn, pp);
+        break;
+    case 2:
+        if (pp->pports.perst[pn].issued_assert_perst ||
+            pp->pports.perst[pn].asrt_time < ASSERT_WAIT_TIME_MS) {
+            return CXL_MBOX_INTERNAL_ERROR;
+        }
+        device_cold_reset(&dev->qdev);
+        break;
+    default:
+        return CXL_MBOX_INVALID_INPUT;
+    }
+    return ret;
+}
+
 /* 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,
@@ -3579,7 +3695,11 @@ void cxl_init_cci(CXLCCI *cci, size_t payload_max)
 
 void cxl_destroy_cci(CXLCCI *cci)
 {
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
     qemu_mutex_destroy(&cci->bg.lock);
+    for (int i = 0; i < CXL_MAX_PHY_PORTS; i++) {
+        qemu_mutex_destroy(&pp->pports.perst[i].lock);
+    }
     cci->initialized = false;
 }
 
@@ -3798,6 +3918,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 },
 };
@@ -3810,4 +3932,17 @@ 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);
+    /* physical port control */
+    CXLUpstreamPort *pp = CXL_USP(cci->d);
+    for (int i = 0; i < CXL_MAX_PHY_PORTS; i++) {
+        qemu_mutex_init(&pp->pports.perst[i].lock);
+        pp->pports.perst[i].issued_assert_perst = false;
+        /*
+         * Assert PERST involves physical port to be in
+         * hold reset phase for minimum 100ms. No other
+         * physcial port control requests are entertained
+         * until Deassert PERST command.
+         */
+         pp->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 1fa6cf7536..bb47e671eb 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -128,6 +128,7 @@
                   (1 << 16))
 
 #define CXL_MAX_PHY_PORTS 256
+#define ASSERT_WAIT_TIME_MS 100 /* Assert - Deassert PERST */
 
 #define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
 #define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
@@ -203,10 +204,19 @@ struct cxl_phy_port_info {
     uint8_t supported_ld_count;
 } QEMU_PACKED;
 
+/* Assert - Deassert PERST */
+struct pperst {
+    bool issued_assert_perst;
+    QemuMutex lock;
+    uint64_t asrt_time;
+    QemuThread asrt_thread; /* thread for 100ms delay */
+};
+
 struct phy_port {
     uint8_t num_ports;
     uint8_t active_port_bitmask[0x20];
     struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
+    struct pperst perst[CXL_MAX_PHY_PORTS];
 };
 
 /* CXL r3.1 Table 8-34: Command Return Codes */
-- 
2.34.1

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

[Jonathan Cameron]

On Thu, 10 Jul 2025 20:13:38 +0530
Arpit Kumar <arpit1.kumar@samsung.com> wrote:

> -added assert-deassert PERST implementation
>  for physical ports (both USP and DSP's).
> -assert PERST involves bg operation for holding 100ms.
> -reset PPB implementation for physical ports.
> 
> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
Hi Arpit,

Minor comments inline.

We have plenty of time given where the qemu cycle is, so no huge rush
for a v3.

Jonathan

> +static struct PCIDevice *cxl_find_port_dev(uint8_t pn, CXLCCI *cci)
> +{
> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
> +
> +    if (pp->pports.pport_info[pn].current_port_config_state ==
> +        CXL_PORT_CONFIG_STATE_USP) {
> +        PCIDevice *usp_dev = pci_bridge_get_device(bus);
> +        return usp_dev;

As below.

> +    }
> +
> +    if (pp->pports.pport_info[pn].current_port_config_state ==
> +        CXL_PORT_CONFIG_STATE_DSP) {
> +        PCIDevice *dsp_dev = pcie_find_port_by_pn(bus, pn);
> +        return dsp_dev;

What benefit do we get from a local variable?

> +    }
> +    return NULL;
> +}
> +
> +/* 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)
> +{
> +   CXLUpstreamPort *pp = CXL_USP(cci->d);
> +   PCIDevice *dev;
> +   uint8_t ret = CXL_MBOX_SUCCESS;
> +
> +   struct cxl_fmapi_get_physical_port_control_req_pl {
> +        uint8_t ppb_id;
> +        uint8_t ports_op;
> +    } QEMU_PACKED *in = (void *)payload_in;
> +
> +    if (len_in < sizeof(*in)) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    uint8_t pn = in->ppb_id;

Avoid inline declarations of local variables. 

> +    if (pp->pports.pport_info[pn].port_id != pn) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    dev = cxl_find_port_dev(pn, cci);
> +    if (!dev) {
> +        return CXL_MBOX_INTERNAL_ERROR;
> +    }
> +
> +    switch (in->ports_op) {
> +    case 0:
> +        ret = assert_perst(OBJECT(&dev->qdev), pn, pp);
> +        break;
> +    case 1:
> +        ret = deassert_perst(OBJECT(&dev->qdev), pn, pp);
> +        break;
> +    case 2:
> +        if (pp->pports.perst[pn].issued_assert_perst ||
> +            pp->pports.perst[pn].asrt_time < ASSERT_WAIT_TIME_MS) {
> +            return CXL_MBOX_INTERNAL_ERROR;
> +        }
> +        device_cold_reset(&dev->qdev);
> +        break;
> +    default:
> +        return CXL_MBOX_INVALID_INPUT;
> +    }
> +    return ret;
> +}

> @@ -3810,4 +3932,17 @@ 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);
> +    /* physical port control */
> +    CXLUpstreamPort *pp = CXL_USP(cci->d);

Still sticking to the old style c option of declarations at the top of the scope.

> +    for (int i = 0; i < CXL_MAX_PHY_PORTS; i++) {
> +        qemu_mutex_init(&pp->pports.perst[i].lock);
> +        pp->pports.perst[i].issued_assert_perst = false;
> +        /*
> +         * Assert PERST involves physical port to be in
> +         * hold reset phase for minimum 100ms. No other
> +         * physcial port control requests are entertained
> +         * until Deassert PERST command.
> +         */
> +         pp->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 1fa6cf7536..bb47e671eb 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -128,6 +128,7 @@
>                    (1 << 16))
>  
>  #define CXL_MAX_PHY_PORTS 256
> +#define ASSERT_WAIT_TIME_MS 100 /* Assert - Deassert PERST */
>  
>  #define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
>  #define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
> @@ -203,10 +204,19 @@ struct cxl_phy_port_info {
>      uint8_t supported_ld_count;
>  } QEMU_PACKED;
>  
> +/* Assert - Deassert PERST */
> +struct pperst {

Prefix this name. pperst is a PCIE thing so it may otherwise end up confusing.

> +    bool issued_assert_perst;
> +    QemuMutex lock;

Good practice to add a comment to say exactly what data this lock is protecting.

> +    uint64_t asrt_time;
> +    QemuThread asrt_thread; /* thread for 100ms delay */
> +};
> +
>  struct phy_port {
>      uint8_t num_ports;
>      uint8_t active_port_bitmask[0x20];
>      struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
> +    struct pperst perst[CXL_MAX_PHY_PORTS];
>  };
>  
>  /* CXL r3.1 Table 8-34: Command Return Codes */

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

[Arpit Kumar]

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

On 25/07/25 03:59PM, Jonathan Cameron wrote:
>On Thu, 10 Jul 2025 20:13:38 +0530
>Arpit Kumar <arpit1.kumar@samsung.com> wrote:
>
>> -added assert-deassert PERST implementation
>>  for physical ports (both USP and DSP's).
>> -assert PERST involves bg operation for holding 100ms.
>> -reset PPB implementation for physical ports.
>>
>> Signed-off-by: Arpit Kumar <arpit1.kumar@samsung.com>
>Hi Arpit,
>
>Minor comments inline.
>
>We have plenty of time given where the qemu cycle is, so no huge rush
>for a v3.
>
>Jonathan
>
>> +static struct PCIDevice *cxl_find_port_dev(uint8_t pn, CXLCCI *cci)
>> +{
>> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
>> +    PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
>> +
>> +    if (pp->pports.pport_info[pn].current_port_config_state ==
>> +        CXL_PORT_CONFIG_STATE_USP) {
>> +        PCIDevice *usp_dev = pci_bridge_get_device(bus);
>> +        return usp_dev;
>
>As below.
>
>> +    }
>> +
>> +    if (pp->pports.pport_info[pn].current_port_config_state ==
>> +        CXL_PORT_CONFIG_STATE_DSP) {
>> +        PCIDevice *dsp_dev = pcie_find_port_by_pn(bus, pn);
>> +        return dsp_dev;
>
>What benefit do we get from a local variable?
>
thanks for pointing out.
>> +    }
>> +    return NULL;
>> +}
>> +
>> +/* 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)
>> +{
>> +   CXLUpstreamPort *pp = CXL_USP(cci->d);
>> +   PCIDevice *dev;
>> +   uint8_t ret = CXL_MBOX_SUCCESS;
>> +
>> +   struct cxl_fmapi_get_physical_port_control_req_pl {
>> +        uint8_t ppb_id;
>> +        uint8_t ports_op;
>> +    } QEMU_PACKED *in = (void *)payload_in;
>> +
>> +    if (len_in < sizeof(*in)) {
>> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
>> +    }
>> +
>> +    uint8_t pn = in->ppb_id;
>
>Avoid inline declarations of local variables.
>
got it.
>> +    if (pp->pports.pport_info[pn].port_id != pn) {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    dev = cxl_find_port_dev(pn, cci);
>> +    if (!dev) {
>> +        return CXL_MBOX_INTERNAL_ERROR;
>> +    }
>> +
>> +    switch (in->ports_op) {
>> +    case 0:
>> +        ret = assert_perst(OBJECT(&dev->qdev), pn, pp);
>> +        break;
>> +    case 1:
>> +        ret = deassert_perst(OBJECT(&dev->qdev), pn, pp);
>> +        break;
>> +    case 2:
>> +        if (pp->pports.perst[pn].issued_assert_perst ||
>> +            pp->pports.perst[pn].asrt_time < ASSERT_WAIT_TIME_MS) {
>> +            return CXL_MBOX_INTERNAL_ERROR;
>> +        }
>> +        device_cold_reset(&dev->qdev);
>> +        break;
>> +    default:
>> +        return CXL_MBOX_INVALID_INPUT;
>> +    }
>> +    return ret;
>> +}
>
>> @@ -3810,4 +3932,17 @@ 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);
>> +    /* physical port control */
>> +    CXLUpstreamPort *pp = CXL_USP(cci->d);
>
>Still sticking to the old style c option of declarations at the top of the scope.
>
okay
>> +    for (int i = 0; i < CXL_MAX_PHY_PORTS; i++) {
>> +        qemu_mutex_init(&pp->pports.perst[i].lock);
>> +        pp->pports.perst[i].issued_assert_perst = false;
>> +        /*
>> +         * Assert PERST involves physical port to be in
>> +         * hold reset phase for minimum 100ms. No other
>> +         * physcial port control requests are entertained
>> +         * until Deassert PERST command.
>> +         */
>> +         pp->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 1fa6cf7536..bb47e671eb 100644
>> --- a/include/hw/cxl/cxl_device.h
>> +++ b/include/hw/cxl/cxl_device.h
>> @@ -128,6 +128,7 @@
>>                    (1 << 16))
>>
>>  #define CXL_MAX_PHY_PORTS 256
>> +#define ASSERT_WAIT_TIME_MS 100 /* Assert - Deassert PERST */
>>
>>  #define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
>>  #define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
>> @@ -203,10 +204,19 @@ struct cxl_phy_port_info {
>>      uint8_t supported_ld_count;
>>  } QEMU_PACKED;
>>
>> +/* Assert - Deassert PERST */
>> +struct pperst {
>
>Prefix this name. pperst is a PCIE thing so it may otherwise end up confusing.
>
okay
>> +    bool issued_assert_perst;
>> +    QemuMutex lock;
>
>Good practice to add a comment to say exactly what data this lock is protecting.
>
got it.
>> +    uint64_t asrt_time;
>> +    QemuThread asrt_thread; /* thread for 100ms delay */
>> +};
>> +
>>  struct phy_port {
>>      uint8_t num_ports;
>>      uint8_t active_port_bitmask[0x20];
>>      struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
>> +    struct pperst perst[CXL_MAX_PHY_PORTS];
>>  };
>>
>>  /* CXL r3.1 Table 8-34: Command Return Codes */
>

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