Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MCTP control command tests #46

Merged
merged 24 commits into from
Dec 11, 2024
Merged

Add MCTP control command tests #46

merged 24 commits into from
Dec 11, 2024

Conversation

parvathib
Copy link
Collaborator

This PR adds MCTP mux component and instantiate it in the board configuration

Allocate resources just enough for instantiating the MCTP MUX driver. There'll be new additions in the future.
The static reference in the platform structure is temporarily added until the driver layer is implemented.
This should set up the stack for testing the MCTP control messages with integration test framework.

Parvathi Bhogaraju and others added 12 commits December 5, 2024 10:36
This maps the internal queues in the I3C emulator peripheral to the
interrupt controller, and modifies the tests so that they no longer
manually poll.

I also added a new package, `romtime`, for drivers and utilities that
are useful for both ROM and Runtime. For now, this contains an
implementation of `print!` and `println!` that output to the emulator
UART immediately, which is useful for both ROM and when running in Tock
kernel when `debug!` may not be available (such as during tests or
interrupts).
@parvathib parvathib changed the base branch from main to i3c-irq December 7, 2024 20:07
@parvathib parvathib changed the base branch from i3c-irq to main December 7, 2024 20:08
@parvathib parvathib marked this pull request as draft December 9, 2024 17:36
@parvathib parvathib changed the title Instantiate MCTP Mux component in board configuration Add MCTP control command tests Dec 10, 2024
@parvathib parvathib marked this pull request as ready for review December 10, 2024 20:32
@@ -25,8 +25,13 @@ gdbstub.workspace = true
hex.workspace = true
tock-registers.workspace = true
zerocopy.workspace = true
bitfield.workspace = true
crc = "3.2.1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: this is the same package that is used in emulator/bmc/pldm-fw-pkg/Cargo.toml, so we might as well consolidate them into the main Cargo.toml so they don't accidentally get out of sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

tests: Vec<Test>,
) {
let running_clone = running.clone();
let addr = SocketAddr::from(([127, 0, 0, 1], port));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be addressed in another PR, but since we are only communicating with the socket locally and not from another program, it might be easier to use the mpsc channels directly so that we don't have to worry about the ports. (Otherwise, we're going to run into issues where there is a port conflict since we hard code the port and don't check if it's already in use.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! I did face a port conflict once while testing. I'll change to mpsc in the next PR when I add more tests.

@@ -0,0 +1,212 @@
// Licensed under the Apache-2.0 license

// use crc;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore?

Suggested change
// use crc;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

pub const MCTP_CTRL_MSG_HDR_SIZE: usize = 2;

#[macro_export]
macro_rules! set_eid_req {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These macros feel like they could just be simple functions?

Like fn set_eid_req(op: u8, eid: u8) -> Vec<u8>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to functions

@@ -21,6 +21,9 @@ use registers_generated::i3c::I3C_CSR_ADDR;
use tock_registers::register_bitfields;
use tock_registers::LocalRegisterCopy;

use core::fmt::Write;
use romtime::println;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these imports are used anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -72,6 +72,7 @@ impl I3cController {
let running = self.running.clone();
let targets = self.targets.clone();
let counter = self.incoming_counter.clone();
println!("Starting I3C controller thread");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't need this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

self.i3c_target.send_ibi(self.tti_ibi_buffer[4]);
self.tti_ibi_buffer.drain(0..len + 4);
self.tti_ibi_buffer.drain(0..8);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically this length could be more, so maybe (len + 4).next_multiple_of(4)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds correct! Changed!

let mut read_cmd = ReguDataTransferCommand::read_from_bytes(&[0; 8]).unwrap();
read_cmd.set_rnw(1);
read_cmd.set_data_length(0);
let cmd_words: [u32; 2] = transmute!(read_cmd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried about endianness biting us, especially in the (unlikely) event we ever run this code with a big endian host.

Could we add a test at least that checks that the packets generated have the bytes that we expect? (That way if something weird happens with endianness in the future, we'll at least have a test to let us know.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Added tests for prepare_private_read and prepare_private_write functions.

@parvathib parvathib requested a review from swenson December 11, 2024 00:37
Copy link
Collaborator

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@parvathib parvathib merged commit 1660f1b into main Dec 11, 2024
1 check passed
@parvathib parvathib deleted the pbhogaraju/mctp_capsule branch December 11, 2024 17:31
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