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

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

On Tue, 16 Sep 2025 13:37:36 +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>
See below for why but I've picked this up with this and a define move that
was needed to build it where I've queued it up.

Thanks!

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 6f7291e6ad..1d16d033c7 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -607,7 +607,7 @@ static void *bg_assertcb(void *opaque)
     return NULL;
 }

-static CXLRetCode deassert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
+static CXLRetCode cxl_deassert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
 {
     if (!pp->pports.perst[pn].issued_assert_perst) {
         return CXL_MBOX_INTERNAL_ERROR;
@@ -623,7 +623,7 @@ static CXLRetCode deassert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
     return CXL_MBOX_SUCCESS;
 }

-static CXLRetCode assert_perst(Object *obj, uint8_t pn, CXLUpstreamPort *pp)
+static CXLRetCode cxl_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) {
@@ -668,7 +668,6 @@ static CXLRetCode cmd_physical_port_control(const struct cxl_cmd *cmd,
    CXLUpstreamPort *pp = CXL_USP(cci->d);
    PCIDevice *dev;
    uint8_t pn;
-   uint8_t ret = CXL_MBOX_SUCCESS;

    struct cxl_fmapi_get_physical_port_control_req_pl {
         uint8_t ppb_id;
@@ -691,22 +690,19 @@ static CXLRetCode cmd_physical_port_control(const struct cxl_cmd *cmd,

     switch (in->ports_op) {
     case 0:
-        ret = assert_perst(OBJECT(&dev->qdev), pn, pp);
-        break;
+        return cxl_assert_perst(OBJECT(&dev->qdev), pn, pp);
     case 1:
-        ret = deassert_perst(OBJECT(&dev->qdev), pn, pp);
-        break;
+        return cxl_deassert_perst(OBJECT(&dev->qdev), pn, pp);
     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;
+        return CXL_MBOX_SUCCESS;
     default:
         return CXL_MBOX_INVALID_INPUT;
     }
-    return ret;
 }

> ---
>  hw/cxl/cxl-mailbox-utils.c                | 139 ++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h               |   9 ++
>  include/hw/cxl/cxl_mailbox.h              |   1 +

Had to move the define for now that went in here because I'm carrying this
further up my tree than Anisa's patch that moved this as a precursor
to the MCTP support.


>  include/hw/pci-bridge/cxl_upstream_port.h |   1 +
>  4 files changed, 150 insertions(+)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 2a104dd337..8cccb2c0ed 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -541,6 +541,120 @@ static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static void *bg_assertcb(void *opaque)
> +{
> +    CXLPhyPortPerst *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)

I think we want to namespace these to cxl given these are not
a PCI standard thing.  So when I pick these up I'll prefix with cxl_

> +{
> +    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 &=
> +        ~CXL_PORT_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 |=
> +        CXL_PORT_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) {
> +        return pci_bridge_get_device(bus);
> +    }
> +
> +    if (pp->pports.pport_info[pn].current_port_config_state ==
> +        CXL_PORT_CONFIG_STATE_DSP) {
> +        return pcie_find_port_by_pn(bus, pn);
> +    }
> +    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 pn;
> +   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;
> +    }
> +
> +    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);
To me, direct returns are clearer here.
     return cxl_assert_perst();

I tweaked this and the other case statements in here.  That lets
me drop the local variable ret and the return code hidden up top.

> +        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,
> @@ -4347,6 +4461,8 @@ static const struct cxl_cmd cxl_cmd_set_sw[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 },
>  };
> @@ -4702,11 +4818,34 @@ static CXLRetCode cxl_set_phy_port_info(CXLCCI *cci)
>  void cxl_initialize_mailbox_swcci(CXLCCI *cci, DeviceState *intf,
>                                    DeviceState *d, size_t payload_max)
>  {
> +    CXLUpstreamPort *pp;
> +    uint8_t pn = 0;
> +
>      cxl_copy_cci_commands(cci, cxl_cmd_set_sw);
>      cci->d = d;
>      cci->intf = intf;
>      cxl_init_cci(cci, payload_max);
>      cxl_set_phy_port_info(cci);
> +    /* physical port control */
> +    pp = CXL_USP(cci->d);
> +    for (int byte_index = 0; byte_index < (CXL_MAX_PHY_PORTS / BITS_PER_BYTE);
> +         byte_index++) {
> +        unsigned char byte = pp->pports.active_port_bitmask[byte_index];
> +
> +        for (int bit_index = 0; bit_index < 8; bit_index++, pn++) {
> +            if (((byte) & (1 << bit_index)) != 0) {
> +                qemu_mutex_init(&pp->pports.perst[pn].lock);
> +                pp->pports.perst[pn].issued_assert_perst = false;
> +                /*
> +                 * Assert PERST involves physical port to be in
> +                 * hold reset phase for minimum 100ms. No other
> +                 * physical port control requests are entertained
> +                 * until Deassert PERST command.
> +                 */
> +                pp->pports.perst[pn].asrt_time = ASSERT_WAIT_TIME_MS;
> +            }
> +        }
> +    }
>  }
>  
>  void cxl_initialize_mailbox_t3(CXLCCI *cci, DeviceState *d, size_t payload_max)