Re: [PATCH 2/3] hw/cxl: Simplified Identify Switch Device & Get Physical Port State

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

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

On 10/06/25 03:29PM, Jonathan Cameron wrote:
>On Mon,  2 Jun 2025 19:29:41 +0530
>Arpit Kumar <arpit1.kumar@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.kumar@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.
>
okay
>
>> @@ -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.
>
right, will do the same in next iteration(V2) of the patch series.
>> -
>> +    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?
thanks for pointing this out, will add the comment back.
>
>>      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?
okay
>
>> +                memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
>> +                       sizeof(struct cxl_phy_port_info));

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