On 10/06/25 03:21PM, Jonathan Cameron wrote:
On Mon, 2 Jun 2025 19:29:40 +0530
Arpit Kumar <arpit1.ku...@samsung.com> wrote:
Physical ports info is stored for both mailbox cci &
mctp based cci type as per spec CXL r3.2 Table 8-230: Physical Switch
Signed-off-by: Arpit Kumar <arpit1.ku...@samsung.com>
Hi Arpit,
Sorry for slow response. I got behind on reviews in general and missed
this in the backlog!
Anyhow, main comment is that this is mostly a refactor that only makes
sense in a single patch with what you have in patch 2.
Don't introduce duplicate infrastructure for so few usecases. It means
we can't just look at he diff and see what was added vs what was removed.
Much cleaner to just have a single refactoring patch.
Thanks Jonathan for the review. Will combine the first 2 patches and
apply changes as suggested in the next iteration(V2) of the patch
series.
---
hw/cxl/cxl-mailbox-utils.c | 166 ++++++++++++++++++++++++++++++++++++
include/hw/cxl/cxl_device.h | 28 ++++++
2 files changed, 194 insertions(+)
diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index a02d130926..680055c6c0 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -124,6 +124,64 @@ enum {
#define GET_MHD_INFO 0x0
};
+/* link state flags */
Naming makes that clear (which is great) so I'd drop the comment.
okay
+#define LINK_STATE_FLAG_LANE_REVERSED (1 << 0)
BIT(0) from qemu/bitops.h
got it
+#define LINK_STATE_FLAG_PERST_ASSERTED (1 << 1)
+#define LINK_STATE_FLAG_PRSNT (1 << 2)
+#define LINK_STATE_FLAG_POWER_OFF (1 << 3)
+
+/* physical port control info - CXL r3.2 table 7-19 */
+typedef enum {
+ PORT_DISABLED = 0,
+ BIND_IN_PROGRESS = 1,
+ UNBIND_IN_PROGRESS = 2,
+ DSP = 3,
+ USP = 4,
+ FABRIC_PORT = 5,
+ INVALID_PORT_ID = 15
+} current_port_config_state;
These aren't used as types really but more as defines.
Hence I'd be tempted to make them defines. Also provide
defines for the masks.
Namespace the defines so it is obvious which field
they are in.
Will the below changes be the right way to proceed?
#define CXL_PORT_CONFIG_STATE_DISABLED 0x0
#define CXL_PORT_CONFIG_STATE_BIND_IN_PROGRESS 0x1 etc.
+
+typedef enum {
+ NOT_CXL_OR_DISCONNECTED = 0x00,
+ RCD_MODE = 0x01,
+ CXL_68B_FLIT_AND_VH_MODE = 0x02,
+ STANDARD_256B_FLIT_MODE = 0x03,
+ CXL_LATENCY_OPTIMIZED_256B_FLIT_MODE = 0x04,
+ PBR_MODE = 0x05
+} connected_device_mode;
+
+typedef enum {
+ NO_DEVICE_DETECTED = 0,
+ PCIE_DEVICE = 1,
+ CXL_TYPE_1_DEVICE = 2,
+ CXL_TYPE_2_DEVICE_OR_HBR_SWITCH = 3,
+ CXL_TYPE_3_SLD = 4,
+ CXL_TYPE_3_MLD = 5,
+ PBR_COMPONENT = 6
+} connected_device_type;
+
+typedef enum {
+ CXL_RCD_MODE = 0x00,
+ CXL_68B_FLIT_AND_VH_CAPABLE = 0x01,
+ CXL_256B_FLIT_CAPABLE = 0x02,
+ CXL_LATENCY_OPTIMIZED_256B_FLIT = 0x03,
+ CXL_PBR_CAPABLE = 0x04
+} supported_cxl_modes;
+
+typedef enum {
+ LTSSM_DETECT = 0x00,
+ LTSSM_POLLING = 0x01,
+ LTSSM_CONFIGURATION = 0x02,
+ LTSSM_RECOVERY = 0x03,
+ LTSSM_L0 = 0x04,
+ LTSSM_L0S = 0x05,
+ LTSSM_L1 = 0x06,
+ LTSSM_L2 = 0x07,
+ LTSSM_DISABLED = 0x08,
+ LTSSM_LOOPBACK = 0x09,
+ LTSSM_HOT_RESET = 0x0A
+} LTSSM_State;
+
/* CCI Message Format CXL r3.1 Figure 7-19 */
typedef struct CXLCCIMessage {
uint8_t category;
@@ -3686,6 +3744,112 @@ void cxl_add_cci_commands(CXLCCI *cci, const struct
cxl_cmd (*cxl_cmd_set)[256],
cxl_rebuild_cel(cci);
}
+static CXLRetCode cxl_set_port_type(struct phy_port *ports, int pnum,
+ CXLCCI *cci)
+{
+ PCIDevice *port_dev;
+ uint16_t lnkcap, lnkcap2, lnksta;
+ int i = pnum;
I'd just use pnum for all the indexes. This local variable doesn't add
much readability.
okay
+ if (!cci) {
As below. No need to defend against bugs so only check if there is a
chance cci might not be set.
right, thanks for pointing out.
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+
Don't interleave declarations and cod.e
got it
+ PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+ PCIEPort *usp = PCIE_PORT(cci->d);
blank line after declarations - of include this
in the declarations as
PCIEDevice *port_dev = pcie_find_port_by_pn(bus, i);
However that is reasonable expensive, so maybe do the usp case first?
got it
+ port_dev = pcie_find_port_by_pn(bus, i);
+
+ if (port_dev) { /* DSP */
+ PCIDevice *ds_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(port_dev))
+ ->devices[0];
+ ports->pport_info[i].port_id = i;
This is not affected by USP or DSP, so drop it out of this if / else.
okay
+ ports->pport_info[i].current_port_config_state = DSP;
+ ports->active_port_bitmask[i / 8] |= (1 << i % 8);
As below - this is currently set but not read, so I'd bring it in as part of
the patch that uses it.
Independent of port type, so doesn't belong in the if/else.
okay
+ if (ds_dev) {
+ if (object_dynamic_cast(OBJECT(ds_dev), TYPE_CXL_TYPE3)) {
+ ports->pport_info[i].connected_device_type = CXL_TYPE_3_MLD;
Hmm. We should make this controllable (vs SLD). I made this up in the existing
code and probably shouldn't have done :(
got it, will currently change it to CXL_TYPE_3_SLD in next iteration of
patch series. However, once we add support for MLD, will make this
controllable as suggested.
+ } else {
+ ports->pport_info[i].connected_device_type = PCIE_DEVICE;
+ }
+ } else {
+ ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
+ }
+ ports->pport_info[i].supported_ld_count = 3;
+ } else if (usp->port == i) { /* USP */
+ port_dev = PCI_DEVICE(usp);
+ ports->pport_info[i].port_id = i;
+ ports->pport_info[i].current_port_config_state = USP;
+ ports->pport_info[i].connected_device_type = NO_DEVICE_DETECTED;
+ ports->active_port_bitmask[i / 8] |= (1 << i % 8);
+ } else {
+ return CXL_MBOX_INVALID_INPUT;
+ }
+
+ if (!port_dev->exp.exp_cap) {
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+
+ lnksta = port_dev->config_read(port_dev,
+ port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
+ sizeof(lnksta));
+ lnkcap = port_dev->config_read(port_dev,
+ port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
+ sizeof(lnkcap));
+ lnkcap2 = port_dev->config_read(port_dev,
+ port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
+ sizeof(lnkcap2));
+
+ ports->pport_info[i].max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
+ ports->pport_info[i].negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW)
>> 4;
+ ports->pport_info[i].supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
+ ports->pport_info[i].max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
+ ports->pport_info[i].current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
+
+ ports->pport_info[i].ltssm_state = LTSSM_L2;
+ ports->pport_info[i].first_negotiated_lane_num = 0;
+ ports->pport_info[i].link_state_flags = 0;
+ ports->pport_info[i].supported_cxl_modes = CXL_256B_FLIT_CAPABLE;
+ ports->pport_info[i].connected_device_mode = STANDARD_256B_FLIT_MODE;
+
+ return CXL_MBOX_SUCCESS;
+}
+
+static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
+{
+ uint8_t phy_port_num;
+ if (!cci) {
What is this defending against? At least for now the cci
is definitely not null where this function is used.
got it
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+
+ PCIEPort *usp = PCIE_PORT(cci->d);
Don't mix declarations and code. Declarations still typically
go at the top.
got it
+ PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
+ struct phy_port *ports = &cci->pports;
+ int num_phys_ports = pcie_count_ds_ports(bus) + 1;
+ if (num_phys_ports < 0) {
Given add 1 this is always false and the check serve no
purpose.
right, thanks for pointing this out
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+
+ ports->num_ports = num_phys_ports;
+ phy_port_num = usp->port;
+
+ cxl_set_port_type(ports, phy_port_num, cci); /* usp */
USP for comment given it's an acronym.
okay
+
+ for (int devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
Maybe use pci_for_each_device_under_bus() ?
okay
+ PCIDevice *dev = bus->devices[devfn];
+
+ if (dev) {
+ phy_port_num = PCIE_PORT(dev)->port;
+ const char *typename = object_get_typename(OBJECT(dev));
Normally in qemu we just use a dynamic cast to identify types rather than
matching on names. I'd prefer that approach here.
if (object_dynamic_cast(OBJECT(dev), TYPE_CXL_DSP)
got it
+
+ if ((strcmp(typename, "cxl-downstream") == 0)) {
+ cxl_set_port_type(ports, phy_port_num, cci);
+ } else {
+ return CXL_MBOX_INTERNAL_ERROR;
+ }
+ }
+ }
+ return CXL_MBOX_SUCCESS;
+}
+
void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
DeviceState *d, size_t payload_max)
{
@@ -3693,6 +3857,7 @@ void cxl_initialize_mailbox_swcci(CXLCCI *cci,
DeviceState *intf,
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
+ cxl_set_phy_port_info(cci); /* store port info */
}
void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)
@@ -3797,4 +3962,5 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState
*d, DeviceState *intf,
cci->d = d;
cci->intf = intf;
cxl_init_cci(cci, payload_max);
+ cxl_set_phy_port_info(cci); /* store port info */
I'd not bother with the comment. The naming of the function is clear enough.
okay
}
diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
index ca515cab13..9eb128a1e8 100644
--- a/include/hw/cxl/cxl_device.h
+++ b/include/hw/cxl/cxl_device.h
@@ -127,6 +127,31 @@
CXL_NUM_CHMU_INSTANCES * (1 << 16), \
(1 << 16))
+/* CXL r3.2 Table 7-19: Port Info */
+struct cxl_phy_port_info {
There is one of these in cxl-mailbox-utils.c already. Pull it out
as part of this patch. That may mean combining patches 1 and 2.
+ uint8_t port_id;
+ uint8_t current_port_config_state;
I'd pull the field defines and masks etc alongside the structure
definition.
okay
+ uint8_t connected_device_mode;
+ uint8_t rsv1;
+ uint8_t connected_device_type;
+ uint8_t supported_cxl_modes;
+ uint8_t max_link_width;
+ uint8_t negotiated_link_width;
+ uint8_t supported_link_speeds_vector;
+ uint8_t max_link_speed;
+ uint8_t current_link_speed;
+ uint8_t ltssm_state;
+ uint8_t first_negotiated_lane_num;
+ uint16_t link_state_flags;
+ uint8_t supported_ld_count;
+} QEMU_PACKED;
+
+struct phy_port {
+ uint8_t num_ports;
+ uint8_t active_port_bitmask[0x20];
Why that size? Also not used yet - so maybe bring this in where
you need it?
+ struct cxl_phy_port_info pport_info[PCI_DEVFN_MAX];
I think there are lower limits than that on how many ports
we can have on a given bus + does this actually care about
the limits from one bus? Request is per switch, not per
virtual heirarchy. So we might be limited by number of unique
port numbers.
Today we only have one VCH so maybe this is fine for now.
Right, the request is per switch and not per virtual heirarchy.
According to CXL r3.2 Table 7-16, length of number of physical ports
is 1 byte which accounts for 256 physical ports. So, will
#define CXL_MAX_PHY_PORTS 256 and change the length of pport_info[] to
it.
+};
+
/* CXL r3.1 Table 8-34: Command Return Codes */
typedef enum {
CXL_MBOX_SUCCESS = 0x0,
@@ -223,6 +248,9 @@ typedef struct CXLCCI {
/* get log capabilities */
const CXLLogCapabilities *supported_log_cap;
+ /*physical ports information */
Space after /*
Run checkpatch.pl over qemu patches. I 'think' it would have
caught this trivial thing.
I did run checkpatch.pl but it didn't catch this. will recheck for such
trivial things moving forward.
+ struct phy_port pports;
Storing this in the CCI is a little odd seeing (as opposed to
in the USP - maybe DSP) as there is only one answer to this query whichever
CCI we come in on.
For similar things we do have firmware update in there which
is a mixture of device side stuff and CCI specific handling.
That might ideally be split up. Obviously not something for
this patch, but maybe we can avoid making CCI more bloated?
To me it seems reasonable to store this in the port and set
it up whether or not the cci is connected.
got it, will try the implementation accordingly. It will be helpful if could
share some
reference on handling the same.
+
/* background command handling (times in ms) */
struct {
uint16_t opcode;
