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

[Arpit Kumar <arpit1.kumar@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 9954 bytes --]

On 17/09/25 05:05PM, Jonathan Cameron wrote:
>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!
>
Understood, the changes look good to me.
Thanks for handling it.
>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.
>
got it
>
>>  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_
>
got it
>> +{
>> +    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.
>
makes sense, thanks!
>> +        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)

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]