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>
---
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) */
static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
uint8_t *payload_in,
size_t len_in,
@@ -576,9 +566,7 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
size_t *len_out,
CXLCCI *cci)
{
- PCIEPort *usp = PCIE_PORT(cci->d);
- PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
- int num_phys_ports = pcie_count_ds_ports(bus);
+ int num_phys_ports = cci->pports.num_ports;
struct cxl_fmapi_ident_switch_dev_resp_pl {
uint8_t ingress_port_id;
@@ -595,11 +583,11 @@ static CXLRetCode cmd_identify_switch_device(const struct cxl_cmd *cmd,
out = (struct cxl_fmapi_ident_switch_dev_resp_pl *)payload_out;
*out = (struct cxl_fmapi_ident_switch_dev_resp_pl) {
- .num_physical_ports = num_phys_ports + 1, /* 1 USP */
+ .num_physical_ports = num_phys_ports,
.num_vcss = 1, /* Not yet support multiple VCS - potentially tricky */
.active_vcs_bitmask[0] = 0x1,
- .total_vppbs = num_phys_ports + 1,
- .bound_vppbs = num_phys_ports + 1,
+ .total_vppbs = num_phys_ports,
+ .bound_vppbs = num_phys_ports,
.num_hdm_decoders_per_usp = 4,
};
@@ -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);
-
+ 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)
{
- /* CXL r3.1 Table 7-17: Get Physical Port State Request Payload */
+ size_t pl_size;
+ int i;
+
+ /* CXL r3.2 Table 7-17: Get Physical Port State Request Payload */
struct cxl_fmapi_get_phys_port_state_req_pl {
uint8_t num_ports;
uint8_t ports[];
} QEMU_PACKED *in;
- /*
- * CXL r3.1 Table 7-19: Get Physical Port State Port Information Block
- * Format
- */
- struct cxl_fmapi_port_state_info_block {
- uint8_t port_id;
- uint8_t config_state;
- uint8_t connected_device_cxl_version;
- uint8_t rsv1;
- uint8_t connected_device_type;
- uint8_t port_cxl_version_bitmask;
- uint8_t max_link_width;
- uint8_t negotiated_link_width;
- uint8_t supported_link_speeds_vector;
- uint8_t max_link_speed;
- uint8_t current_link_speed;
- uint8_t ltssm_state;
- uint8_t first_lane_num;
- uint16_t link_state;
- uint8_t supported_ld_count;
- } QEMU_PACKED;
-
- /* CXL r3.1 Table 7-18: Get Physical Port State Response Payload */
+ /* CXL r3.2 Table 7-18: Get Physical Port State Response Payload */
struct cxl_fmapi_get_phys_port_state_resp_pl {
uint8_t num_ports;
uint8_t rsv1[3];
- struct cxl_fmapi_port_state_info_block ports[];
+ struct cxl_phy_port_info ports[];
} QEMU_PACKED *out;
- PCIBus *bus = &PCI_BRIDGE(cci->d)->sec_bus;
- PCIEPort *usp = PCIE_PORT(cci->d);
- size_t pl_size;
- int i;
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 */
+
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) {
+ memcpy(&out->ports[i], &(cci->pports.pport_info[pn]),
+ sizeof(struct cxl_phy_port_info));
}
- port->supported_ld_count = 3;
- } else if (usp->port == in->ports[i]) { /* USP */
- port_dev = PCI_DEVICE(usp);
- port->config_state = 4;
- port->connected_device_type = 0;
- } else {
- return CXL_MBOX_INVALID_INPUT;
- }
-
- port->port_id = in->ports[i];
- /* Information on status of this port in lnksta, lnkcap */
- if (!port_dev->exp.exp_cap) {
- return CXL_MBOX_INTERNAL_ERROR;
}
- lnksta = port_dev->config_read(port_dev,
- port_dev->exp.exp_cap + PCI_EXP_LNKSTA,
- sizeof(lnksta));
- lnkcap = port_dev->config_read(port_dev,
- port_dev->exp.exp_cap + PCI_EXP_LNKCAP,
- sizeof(lnkcap));
- lnkcap2 = port_dev->config_read(port_dev,
- port_dev->exp.exp_cap + PCI_EXP_LNKCAP2,
- sizeof(lnkcap2));
-
- port->max_link_width = (lnkcap & PCI_EXP_LNKCAP_MLW) >> 4;
- port->negotiated_link_width = (lnksta & PCI_EXP_LNKSTA_NLW) >> 4;
- /* No definition for SLS field in linux/pci_regs.h */
- port->supported_link_speeds_vector = (lnkcap2 & 0xFE) >> 1;
- port->max_link_speed = lnkcap & PCI_EXP_LNKCAP_SLS;
- port->current_link_speed = lnksta & PCI_EXP_LNKSTA_CLS;
- /* TODO: Track down if we can get the rest of the info */
- port->ltssm_state = 0x7;
- port->first_lane_num = 0;
- port->link_state = 0;
- port->port_cxl_version_bitmask = 0x2;
- port->connected_device_cxl_version = 0x2;
}
pl_size = sizeof(*out) + sizeof(*out->ports) * in->num_ports;
--
2.34.1On 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.
> @@ -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));
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));
© 2016 - 2025 Red Hat, Inc.