rust-vmm / mshv

Crates for Microsoft Hypervisor ioctls and bindings
Apache License 2.0
29 stars 12 forks source link

Rework several IOCTLs to not depend on hv_ definitions #149

Closed NunoDasNeves closed 1 month ago

NunoDasNeves commented 4 months ago

Summary of the PR

The kernel interfaces are being update to not depend on hv_ definitions directly. Update the relevant IOCTLs to work with the new interfaces.

Requirements

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

russell-islam commented 4 months ago

I can see you remove all the Synic Features and related codes, Where are those now?

NunoDasNeves commented 4 months ago

I can see you remove all the Synic Features and related codes, Where are those now?

In the kernel

russell-islam commented 4 months ago

LGTM

liuw commented 3 months ago
error[E0425]: cannot find value `HV_PAGE_SIZE` in this scope
   --> mshv-ioctls/src/ioctls/vm.rs:683:49
    |
683 |         let bitmap_size = div_ceil(memory_size, HV_PAGE_SIZE * 64);
    |                                                 ^^^^^^^^^^^^ help: a constant with a similar name exists: `HV_HYP_PAGE_SIZE`
    |
   ::: /project/mshv-bindings/src/arm64/bindings.rs:194:1

This is a legit error -- missing changes for arm64.

NunoDasNeves commented 3 months ago
error[E0425]: cannot find value `HV_PAGE_SIZE` in this scope
   --> mshv-ioctls/src/ioctls/vm.rs:683:49
    |
683 |         let bitmap_size = div_ceil(memory_size, HV_PAGE_SIZE * 64);
    |                                                 ^^^^^^^^^^^^ help: a constant with a similar name exists: `HV_HYP_PAGE_SIZE`
    |
   ::: /project/mshv-bindings/src/arm64/bindings.rs:194:1

This is a legit error -- missing changes for arm64.

Looks like this was already broken in vcpu.rs too... Fixed!

NunoDasNeves commented 3 months ago

@liuw @russell-islam The cloud hypervisor build/clippy should still be failing, but it's now passing because it's ignoring the [patch] section and continuing to use v0.2.0! It shows this message:

warning: Patch `mshv-bindings v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-bindings)` was not used in the crate graph.
Patch `mshv-ioctls v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-ioctls)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

Also, I can't repro this locally. It just fails to build (as it should) when I patch the mshv crates.

russell-islam commented 3 months ago

@liuw @russell-islam The cloud hypervisor build/clippy should still be failing, but it's now passing because it's ignoring the [patch] section and continuing to use v0.2.0! It shows this message:

warning: Patch `mshv-bindings v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-bindings)` was not used in the crate graph.
Patch `mshv-ioctls v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-ioctls)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

Also, I can't repro this locally. It just fails to build (as it should) when patch the mshv crates.

We can merge this PR and use it in CLH. Will that fix the issue?

NunoDasNeves commented 3 months ago

We can merge this PR and use it in CLH. Will that fix the issue?

No. That would just hide the issue.

The issue is: the Cloud Hypervisor Build workflow is passing. But, that's wrong, it should be failing. It should fail because upstream CLH is not up to date with this PR (that will be fixed in https://github.com/cloud-hypervisor/cloud-hypervisor/pull/6428). So, something is wrong with the workflow.

russell-islam commented 3 months ago

Patching in CLH is not working then.

I am not sure about this warning.

warning: Patch mshv-bindings v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-bindings) was not used in the crate graph. Patch mshv-ioctls v0.3.0 (/home/runner/work/mshv/mshv/mshv/mshv-ioctls) was not used in the crate graph. Check that the patched package version and available features are compatible with the dependency requirements. If the patch has a different version from what is locked in the Cargo.lock file, run cargo update to use the new version. This may also occur with an optional dependency that is not enabled.

liuw commented 3 months ago

We can merge this PR and use it in CLH. Will that fix the issue?

No. That would just hide the issue.

The issue is: the Cloud Hypervisor Build workflow is passing. But, that's wrong, it should be failing. It should fail because upstream CLH is not up to date with this PR (that will be fixed in cloud-hypervisor/cloud-hypervisor#6428). So, something is wrong with the workflow.

Can you compare the command you use for local builds and the command in the workflow.

I see that workflow builds don't use --locked.

jinankjain commented 1 month ago

@NunoDasNeves is it ready to merge now?

liuw commented 1 month ago

@NunoDasNeves is it ready to merge now?

Nuno told me it is.