On 25/07/25 03:54PM, Jonathan Cameron wrote:
On Thu, 10 Jul 2025 20:13:37 +0530
Arpit Kumar <arpit1.ku...@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.ku...@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 */
