Skip to content

Conversation

@Kostr
Copy link

@Kostr Kostr commented Jul 23, 2025

Currently MCTP Control message handler assumes that 'Instance ID' field in the message header is equal zero which is not always the case. Update code to support MCTP Control messages with non-zero Instance ID.

Currently MCTP Control message handler assumes that 'Instance ID' field
in the message header is equal zero which is not always the case.
Update code to support MCTP Control messages with non-zero Instance ID.

Signed-off-by: Konstantin Aladyshev <[email protected]>
@aspeedyh
Copy link

Hi @Kostr,

Thanks for the PR!

Just a quick note—this function is planned to be deprecated in the next version. We originally implemented it for two main reasons:

  • To maintain and update a route table within the driver.
  • To update the route type based on the control message opcode, since we didn’t have visibility into the payload contents when the MCTP core called the create_hdr() operation.

However, after further discussion with the MCTP core maintainer, we reached the following conclusions:

  • The route table should be maintained by the core.
  • The core will update the address to the format [route_type, addr], allowing drivers to populate the correct values during the create_hdr() phase.

You can find the full discussion in the email thread here:
https://lists.openwall.net/netdev/2025/07/14/78

As for this PR—since the driver no longer needs to update the route type or maintain the route table, we can simplify things by removing this function and sending the packet directly.

@Kostr
Copy link
Author

Kostr commented Jul 24, 2025

I thought that the need for the table was the fact that aspeed MCTP hardware doesn't receive full PCIe headers, but only some parts of it.
Because of that we can't get the actual PCIe BDF of the sender from incoming packet:
(https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/soc/aspeed/aspeed-mctp.c#L843)
Right now aspeed-mctp always sets it to 05:00.0 BDF

	/* TODO: PCI Requester ID: HW didn't get this information */
	hdr[6] = 0;
	hdr[7] = 5;

And if we put it as-is in the VDM driver (https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/drivers/net/mctp/mctp-pcie-vdm.c#L427)

		cb = __mctp_cb(skb);
		cb->halen = 2; // BDF size is 2 bytes
		memcpy(cb->haddr, &vdm_hdr->pci_req_id, cb->halen);

the packet would be routed incorrectly.

What would change regarding this case? Will aspeed-mctp still set BDF to 05:00.0 BDF? And if so, how the core would know how to route?

@aspeedyh
Copy link

Hi @Kostr,

Currently, the aspeed-mctp driver supports AST2500, AST2600, and AST2700.
The code snippet you mentioned is only required for the AST2500 series SoCs. When running on AST2600 or AST2700, cb->haddr will carry the correct physical address to the MCTP core.

May I ask if you're using an AST2500 platform for development?

@Kostr
Copy link
Author

Kostr commented Jul 24, 2025

@aspeedyh you are correct. In my case I'm using a custom server board with the Intel CPU and AST2500.
I obtain BDFs of the devices that support MCTP-over-PCIe via Intel BOProxy protocol. And then use standard mctpd to setup MCTP endpoints. After that devices are correctly picked up by the platform-mc pldm module (https://github.com/openbmc/pldm/tree/master/platform-mc) and devices sensors automatically appear in Redfish.

I had to make several tweaks to the aspeed-mctp and mctp-pcie-vdm drivers to make everything work correctly, and now I'm sending them to Aspeed team for the review.

@Kostr
Copy link
Author

Kostr commented Jul 25, 2025

@aspeedyh So what is the plan regarding AST2500? Would it still support messaging via mctp-pcie-vdm driver after your upcoming updates?

I have some other changes for AST2500, but with the information above, I don't really understand if you still want to support this chip.

@aspeedyh
Copy link

Hi @Kostr
We need some discussion internally regarding the support plan.
Could you help to send an email to [email protected] with your company and project information to request support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants