-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.1On 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 */
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 */
>
© 2016 - 2025 Red Hat, Inc.