vhost-device-spi: Add initial implementation

[HaixuCui]

Summary of the PR

This program is a vhost-user backend that emulates a VirtIO SPI bus. This program takes the layout of the spi bus and its devices on the host OS and then talks to them via the /dev/spidevX.Y interface when a request comes from the guest OS for a SPI device.

The implementation corresponds with the specification: https://github.com/oasis-tcs/virtio-spec/tree/virtio-1.4/device-types/spi

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

• All commits in this PR have Signed-Off-By trailers (with
  git commit -s), and the commit message has max 60 characters for the
  summary and max 75 characters for each description line.
• All added/changed functionality has a corresponding unit/integration
  test.
• All added/changed public-facing functionality has entries in the "Upcoming
  Release" section of CHANGELOG.md (if no such section exists, please create one).
• Any newly added unsafe code is properly documented.

[stefano-garzarella]

@HaixuCui sorry, I'm a bit confused, this PR is named "vhost-device-console: Add initial implementation" and we already have another PR #601, but looking in the code and in the PR description it looks like this PR is not covering the console device.

I suppose the PR title is wrong. Please, can you clarify?

Can you also check the CI failures? (note: cargo-audit failure is expected, see #639 )

[HaixuCui]

@stefano-garzarella Yes the title was wrong, I have changed, it's vhost-user-spi, thanks

[HaixuCui]

Hi all, my CI only has two jobs and both failed with the error log:

git clone -v -- https://github.com/rust-vmm/vhost-device.git .
fatal: destination path '.' already exists and is not an empty directory.
⚠️ Warning: Checkout failed! cloning git repository: exit status 128 (Attempt 1/3 Retrying in 2s)
Removing /var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci
🚨 Error: Failed to remove "/var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci" (unlinkat /var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci/staging/target/debug/incremental: permission denied)

Could you please give me some clue, what should I do to resolve this? Thank you very much.

[stefano-garzarella]

Hi all, my CI only has two jobs and both failed with the error log:

git clone -v -- https://github.com/rust-vmm/vhost-device.git . fatal: destination path '.' already exists and is not an empty directory. ⚠️ Warning: Checkout failed! cloning git repository: exit status 128 (Attempt 1/3 Retrying in 2s) Removing /var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci 🚨 Error: Failed to remove "/var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci" (unlinkat /var/lib/buildkite-agent/builds/ip-172-31-70-93-5/rust-vmm/vhost-device-ci/staging/target/debug/incremental: permission denied)

Could you please give me some clue, what should I do to resolve this? Thank you very much.

It looks like this is unrelated. I'll check what is going on with our CI.

[stefano-garzarella]

@HaixuCui please, can you commit the changes to Cargo.lock?

[HaixuCui]

Hello @stefano-garzarella , I add vhost-user-spi package in Cargo.lock. Thanks!

[HaixuCui]

Hi @stefano-garzarella, I found the CI was workable before my last commit, and after I commit the Cargo.lock, it failed again with the same error I mentioned before.
Could you please tell me what you do to make CI works? Really appreciate.

[stefano-garzarella]

Hi @stefano-garzarella, I found the CI was workable before my last commit, and after I commit the Cargo.lock, it failed again with the same error I mentioned before. Could you please tell me what you do to make CI works? Really appreciate.

@HaixuCui sorry about that, but we have problems with CI that we are investigating. For a moment it ran again because we manually removed the problematic file. We're figuring out why, I'll keep you updated.

[HaixuCui]

Hi @stefano-garzarella,

use libc::{c_ulong, ioctl};

#[cfg(target_env = "musl")]
type IoctlRequest = libc::c_int;
#[cfg(not(target_env = "musl"))]
type IoctlRequest = c_ulong;

const SPI_IOC_RD_MODE32: IoctlRequest = 0x80046b05;

The SPI_IOC_RD_MODE32 definition leads to such CI error: literal out of range for i32, with the note "the literal 0x80046b05 (decimal 2147773189) does not fit into the type i32 and will become -2147194107i32".

But now the type of SPI_IOC_RD_MODE32 is libc::c_int, it's signed and defined explicitly, why the compiler treat its value 0x80046b05 as unsigned, and try to fit 2147773189 into i32 type? Just treat as negative value -2147194107 and it can fit into the i32 range.

Solution:

1. As the CI hint, to use as a negative number (decimal -2147194107), consider using the type u32 for the literal and cast it to i32,
   const SPI_IOC_RD_MODE32: IoctlRequest = 0x80046b05u32 as i32;
   but maybe cause other errors when target is not musl
2. add #[deny(overflowing_literals to prevent checking?

Do you have any suggestions? Thank you very much.

[stefano-garzarella]

Do you have any suggestions? Thank you very much.

What about using the ioctl macros from vmm-sys-util?
https://github.com/rust-vmm/vmm-sys-util/blob/main/src/linux/ioctl.rs

For example we used in https://github.com/rust-vmm/vhost/blob/2a89b283fad40047adb991ae3f5b958e5b4270d7/vhost/src/vhost_kern/vhost_binding.rs#L46

We build vhost for musl and glibc without issues, so I guess it already handles it internally.

[HaixuCui]

Hello @stefano-garzarella, many thanks for your support, the CI errors are resolved.

Now there are two errors, cargo-audit-x86_64 and coverage-x86_64. Are they expected? As you mentioned before cargo-audit is an known issue.

Can the review process begin now, or need to wait for the cargo audit fixed?

Thank you so much, again

[stefano-garzarella]

Hello @stefano-garzarella, many thanks for your support, the CI errors are resolved.

Great!

Now there are two errors, cargo-audit-x86_64 and coverage-x86_64. Are they expected? As you mentioned before cargo-audit is an known issue.

I just opened #641 for the cargo-audit issue.

About coverage, it should be green, so you have 2 ways, add more tests or adapt the value in coverage_config_x86_64.json. The former would be preferred, but the deviation seems small, so if you don't have any tests in mind to add, it's fine to adjust the value as well.

Can the review process begin now, or need to wait for the cargo audit fixed?

It can start, but I'm not sure if I have time this week, sorry :-(

[HaixuCui]

Hi @stefano-garzarella , @epilys

I add some test cases, but the coverage only improved slightly.

I cannot add testcase to cover detect_supported_features function in spi.rs, because it interacts with the real hardware, while it is quite a bit function.

Another point is that, after using reader/writer mode of descriptor_util, some conditions also cannot be covered, because some invalid descriptors will be filtered by the following code at the very early stage.

let mut reader = desc_chain
             .clone()
             .reader(&mem)
             .map_err(|_| Error::DescriptorReadFailed)?;

  let writter = desc_chain
             .clone()
             .writer(&mem)
             .map_err(|_| Error::DescriptorReadFailed)?;

Now I have no testcase in mind, and there is still 0.55% deviation. Do you have any idea about improving the coverage? Thank you very much.

[stefano-garzarella]

Now I have no testcase in mind, and there is still 0.55% deviation. Do you have any idea about improving the coverage? Thank you very much.

I haven't had time to check, but don't worry, 0.55% deviation is acceptable, go ahead and edit coverage_config_x86_64.json in this PR to update coverage_score with the new value.

[epilys]

I cannot add testcase to cover detect_supported_features function in spi.rs, because it interacts with the real hardware, while it is quite a bit function.

It could be mocked but for such a low percentile point we could ignore it for now.

Another point is that, after using reader/writer mode of descriptor_util, some conditions also cannot be covered, because some invalid descriptors will be filtered by the following code at the very early stage.

We have that issue in this repository generally, it'd be nice to have mocking with error condition checks but it's a project on its own, out of the scope of this PR! So let's ignore that for now too.

[epilys]

@HaixuCui could you rebase on top of main? I don't have access to the CodeLinaro organisation otherwise I'd do it myself.