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

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

On Thu, 10 Jul 2025 20:13:38 +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>
Hi Arpit,

Minor comments inline.

We have plenty of time given where the qemu cycle is, so no huge rush
for a v3.

Jonathan

> +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) {
> +        PCIDevice *usp_dev = pci_bridge_get_device(bus);
> +        return usp_dev;

As below.

> +    }
> +
> +    if (pp->pports.pport_info[pn].current_port_config_state ==
> +        CXL_PORT_CONFIG_STATE_DSP) {
> +        PCIDevice *dsp_dev = pcie_find_port_by_pn(bus, pn);
> +        return dsp_dev;

What benefit do we get from a local variable?

> +    }
> +    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 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;
> +    }
> +
> +    uint8_t pn = in->ppb_id;

Avoid inline declarations of local variables. 

> +    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);
> +        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;
> +}

> @@ -3810,4 +3932,17 @@ void cxl_initialize_usp_mctpcci(CXLCCI *cci, DeviceState *d, DeviceState *intf,
>      cci->intf = intf;
>      cxl_init_cci(cci, payload_max);
>      cxl_set_phy_port_info(cci);
> +    /* physical port control */
> +    CXLUpstreamPort *pp = CXL_USP(cci->d);

Still sticking to the old style c option of declarations at the top of the scope.

> +    for (int i = 0; i < CXL_MAX_PHY_PORTS; i++) {
> +        qemu_mutex_init(&pp->pports.perst[i].lock);
> +        pp->pports.perst[i].issued_assert_perst = false;
> +        /*
> +         * Assert PERST involves physical port to be in
> +         * hold reset phase for minimum 100ms. No other
> +         * physcial port control requests are entertained
> +         * until Deassert PERST command.
> +         */
> +         pp->pports.perst[i].asrt_time = ASSERT_WAIT_TIME_MS;
> +    }
>  }
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 1fa6cf7536..bb47e671eb 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -128,6 +128,7 @@
>                    (1 << 16))
>  
>  #define CXL_MAX_PHY_PORTS 256
> +#define ASSERT_WAIT_TIME_MS 100 /* Assert - Deassert PERST */
>  
>  #define LINK_STATE_FLAG_LANE_REVERSED    BIT(0)
>  #define LINK_STATE_FLAG_PERST_ASSERTED   BIT(1)
> @@ -203,10 +204,19 @@ struct cxl_phy_port_info {
>      uint8_t supported_ld_count;
>  } QEMU_PACKED;
>  
> +/* Assert - Deassert PERST */
> +struct pperst {

Prefix this name. pperst is a PCIE thing so it may otherwise end up confusing.

> +    bool issued_assert_perst;
> +    QemuMutex lock;

Good practice to add a comment to say exactly what data this lock is protecting.

> +    uint64_t asrt_time;
> +    QemuThread asrt_thread; /* thread for 100ms delay */
> +};
> +
>  struct phy_port {
>      uint8_t num_ports;
>      uint8_t active_port_bitmask[0x20];
>      struct cxl_phy_port_info pport_info[CXL_MAX_PHY_PORTS];
> +    struct pperst perst[CXL_MAX_PHY_PORTS];
>  };
>  
>  /* CXL r3.1 Table 8-34: Command Return Codes */