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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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 */