On Mon, 2 Jun 2025 19:29:41 +0530
Arpit Kumar <arpit1.ku...@samsung.com> wrote:
> Modified Identify Switch Device (Opcode 5100h)
> & Get Physical Port State(Opcode 5101h)
> using physical ports info stored during enumeration
>
> Signed-off-by: Arpit Kumar <arpit1.ku...@samsung.com>
A few additional comments in here.
J
> ---
> hw/cxl/cxl-mailbox-utils.c | 133 +++++++------------------------------
> 1 file changed, 24 insertions(+), 109 deletions(-)
>
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index 680055c6c0..b2fa79a721 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -558,17 +558,7 @@ static CXLRetCode cmd_set_response_msg_limit(const
> struct cxl_cmd *cmd,
> return CXL_MBOX_SUCCESS;
> }
>
> -static void cxl_set_dsp_active_bm(PCIBus *b, PCIDevice *d,
> - void *private)
> -{
> - uint8_t *bm = private;
> - if (object_dynamic_cast(OBJECT(d), TYPE_CXL_DSP)) {
> - uint8_t port = PCIE_PORT(d)->port;
> - bm[port / 8] |= 1 << (port % 8);
> - }
> -}
> -
> -/* CXL r3.1 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
> +/* CXL r3.2 Section 7.6.7.1.1: Identify Switch Device (Opcode 5100h) */
I'd prefer the spec reference updates in a separate patch. They are noise here
and kind of suggest there are real changes rather than just refactoring.
> @@ -611,16 +599,14 @@ static CXLRetCode cmd_identify_switch_device(const
> struct cxl_cmd *cmd,
> out->ingress_port_id = 0;
> }
>
> - pci_for_each_device_under_bus(bus, cxl_set_dsp_active_bm,
> - out->active_port_bitmask);
> - out->active_port_bitmask[usp->port / 8] |= (1 << usp->port % 8);
Ah. With this in front of me the reason for the sizeing is much clearer
than in previous patch on it's own. Combining the two will make it all more
obvious.
> -
> + memcpy(out->active_port_bitmask, cci->pports.active_port_bitmask,
> + sizeof(cci->pports.active_port_bitmask));
> *len_out = sizeof(*out);
>
> return CXL_MBOX_SUCCESS;
> }
>
> -/* CXL r3.1 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
> +/* CXL r3.2 Section 7.6.7.1.2: Get Physical Port State (Opcode 5101h) */
> static CXLRetCode cmd_get_physical_port_state(const struct cxl_cmd *cmd,
> uint8_t *payload_in,
> size_t len_in,
> @@ -628,44 +614,21 @@ static CXLRetCode cmd_get_physical_port_state(const
> struct cxl_cmd *cmd,
> size_t *len_out,
> CXLCCI *cci)
> {
>
> in = (struct cxl_fmapi_get_phys_port_state_req_pl *)payload_in;
> out = (struct cxl_fmapi_get_phys_port_state_resp_pl *)payload_out;
> @@ -673,72 +636,24 @@ static CXLRetCode cmd_get_physical_port_state(const
> struct cxl_cmd *cmd,
> if (len_in < sizeof(*in)) {
> return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> }
> - /* Check if what was requested can fit */
> +
The check is still here... So why remove the comment?
> if (sizeof(*out) + sizeof(*out->ports) * in->num_ports >
> cci->payload_max) {
> 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 > cci->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;
> + int pn = in->ports[i];
> + for (int j = 0; j < PCI_DEVFN_MAX; j++) {
> + if (pn == cci->pports.pport_info[j].port_id) {
Given port id is 0-255 and your port_info has 256 elements, why not index
by port_id when storing them in the first place? That should reduce
complexity of this look up. I don't think we ever actually look up
by devfn?
> + memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
> + sizeof(struct cxl_phy_port_info));
