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 support for arm cca #211

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MatiasVara
Copy link
Collaborator

@MatiasVara MatiasVara commented Jul 29, 2024

This PR aims at adding support to build realm guests. First commit adds support for create_guest_memfd() and set_user_memory_region2(). To do this, the memory_init() is modified by adding a boolean parameter. This is required when building a confidential guest for arm cca and probably also required by other cases.
The second commit imports the virtee/cca crate and adds the steps to build a cca guest. The following items should be completed before merge the PR:

  • use populate() only for kernel and initialize() for non-kernel area
  • handle the case in which the guest switch from shared -> private
  • correctly calculation of max_ipa

This has been testing using the v7 series for Linux as a guest and v6 series for KVM on FVP model.

Feedback is welcome.

@MatiasVara MatiasVara changed the title Enable creating guest memory regions with create_guest_memfd() and set_user_memory_region2() Add support for arm cca Aug 26, 2024
@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 2 times, most recently from d5952be to d4362d6 Compare September 4, 2024 12:10
@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 3 times, most recently from cae99a3 to 4a0e61b Compare September 10, 2024 11:40
@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 3 times, most recently from 5089b4f to e7e2a90 Compare September 13, 2024 17:30
@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 3 times, most recently from dae13da to 6cebf95 Compare October 1, 2024 10:44
@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 2 times, most recently from 18e9a7d to edb2f4a Compare December 19, 2024 13:58
@MatiasVara MatiasVara marked this pull request as ready for review December 19, 2024 15:05
@jakecorrenti
Copy link
Member

@MatiasVara is this ready for review, or should I wait?

@tylerfanelli
Copy link
Member

tylerfanelli commented Jan 9, 2025

@jakecorrenti I'm addressing the KVM guest_memfd changes (which @MatiasVara also added in this PR) to my latest SEV-SNP patches. This will probably require a rebase after that. Once that happens, it will likely be ready for a review.

@MatiasVara
Copy link
Collaborator Author

@jakecorrenti I'm addressing the KVM guest_memfd changes (which @MatiasVara also added in this PR) to my latest SEV-SNP patches. This will probably require a rebase after that. Once that happens, it will likely be ready for a review.

I think the PR is ready to review. The current issue is that it does not work for the latest series for KVM (v6). I am investigating the issue.

@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 2 times, most recently from ba8a4b1 to 0cf214d Compare February 12, 2025 10:53
@MatiasVara
Copy link
Collaborator Author

I just updated the PR with the fix for v6 series. The fix is only to set the KVM_ARM_VCPU_REC feature during the initialization of VCPUs otherwise vcpu_finalize would fail.

@MatiasVara
Copy link
Collaborator Author

@jakecorrenti @tylerfanelli @slp Feel free to give a quick look. I am pretty sure there many things to fix.

@MatiasVara MatiasVara force-pushed the use-guest-memfd branch 2 times, most recently from 7b1f763 to 8b97dc4 Compare February 20, 2025 12:30
Copy link
Member

@jakecorrenti jakecorrenti left a comment

Choose a reason for hiding this comment

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

I didn't actually run it, just looked at the code. Overall looks good. Few comments, though.

Thanks for the work!

kvm-bindings = { version = ">=0.8", features = ["fam-wrappers"] , git = "https://github.com/virtee/kvm-bindings", branch = "add_bindings_for_realms" }
kvm-ioctls = { version = ">=0.17", git = "https://github.com/virtee/kvm-ioctls", branch = "cca" }
Copy link
Member

Choose a reason for hiding this comment

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

Can you make a PR once these branches are ready? I'd like to have the deps be on the main branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean to use the main branch rather than the cca?

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. I'm going to have to make some changes for my TDX work, so I think we'll need to merge your CCA work and my TDX work into the main branch and then use that here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tylerfanelli Do you think we can merge the cca branches into main for kvm-ioctl/bindings repos? Or, shall we have one branch per flavor? Bear in mind that those repo are temporal until changes are upstreamed.

Comment on lines +11 to +12
kvm-bindings = { version = ">=0.8", features = ["fam-wrappers"] , git = "https://github.com/virtee/kvm-bindings", branch = "add_bindings_for_realms" }
kvm-ioctls = { version = ">=0.17", git = "https://github.com/virtee/kvm-ioctls", branch = "cca" }
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here

crossbeam-channel = "0.5"
env_logger = "0.9.0"
libc = ">=0.2.39"
log = "0.4.0"
once_cell = "1.4.1"

kvm-bindings = { version = ">=0.8", features = ["fam-wrappers"] , git = "https://github.com/virtee/kvm-bindings", branch = "add_bindings_for_realms" }
Copy link
Member

Choose a reason for hiding this comment

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

same comment here

Comment on lines +734 to 749
//vmm.kernel_cmdline.insert_str("tsi_hijack")?;
#[cfg(feature = "net")]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be CCA only? There's another one a few lines below as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't remember why I commented this line.

id: u8,
vm_fd: &VmFd,
exit_evt: EventFd,
sender_io: Sender<MemProperties>,
Copy link
Member

Choose a reason for hiding this comment

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

should be CCA only

Copy link
Member

Choose a reason for hiding this comment

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

The memory fault exception isn't TEE-specific (although it could be used that way), so I don't think I agree that this should be CCA only.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. I did this part of the review before you and I had talked about the order of patches.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can enable the extra this extra thread only when the tee feature is enabled.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I made the original comment before it became clear that all of the TEE functionality would end up adopting it

@@ -1103,17 +1231,19 @@ fn create_vcpus_aarch64(
guest_mem: &GuestMemoryMmap,
entry_addr: GuestAddress,
exit_evt: &EventFd,
sender_io: Sender<MemProperties>,
Copy link
Member

Choose a reason for hiding this comment

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

should be CCA only

@MatiasVara
Copy link
Collaborator Author

MatiasVara commented Mar 17, 2025

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

  1. Add Use create_guest_memfd() and set_user_memory_region2() only when tee is enabled.
  2. Add the handling of KVM_MEMORY_FAULT only for tee, which includes the extra thread for handling shared to private and viceverse.
  3. Tee should be replaced with tee and x86-64 since all tee is for x86-64.
  4. Add CCA support. This shall be added with cca and tee enabled.
  5. Eventually some code will be shared for tee in x86-64 and aarch64 but I can't see it yet.

@jakecorrenti @tylerfanelli @slp WDYT?

@jakecorrenti
Copy link
Member

jakecorrenti commented Mar 17, 2025

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

0. Add `Use create_guest_memfd() and set_user_memory_region2()` only when `tee` is enabled.

1. Add the handling of `KVM_MEMORY_FAULT` only for `tee`, which includes the extra thread for handling shared to private and viceverse.

2. `Tee` should be replaced with `tee and x86-64` since all `tee` is for `x86-64`.

I don't know if this is idiomatic by any means, and it depends on whether you can have target arch-specific features, but could we have something like tee-x86 and tee? tee-x86 for the x86 specific functionality, and then tee for the shared bits? Would that be cleaner than just writing #[cfg(all(feature = "tee", target_arch = "x86_64"))]? Again, I don't know if that's "proper" Rust or even possible.

3. Add CCA support. This shall be added with `cca and tee` enabled.

4. Eventually some code will be shared for `tee` in `x86-64` and `aarch64` but I can't see it yet.

@jakecorrenti @tylerfanelli @slp WDYT?

Other than my question above, that all sounds good to me.

@tylerfanelli
Copy link
Member

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

  1. Add Use create_guest_memfd() and set_user_memory_region2() only when tee is enabled.

I don't think set_user_memory_region2 should be tee only, as I believe that on newer kernels, set_user_memory_region is deprecated.

  1. Add the handling of KVM_MEMORY_FAULT only for tee, which includes the extra thread for handling shared to private and viceverse.

I'm addressing that in my SEV-SNP PR currently.

  1. Tee should be replaced with tee and x86-64 since all tee is for x86-64.

Makes sense to me.

  1. Add CCA support. This shall be added with cca and tee enabled.

Do you mean aarch64 and tee?

@MatiasVara
Copy link
Collaborator Author

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

0. Add `Use create_guest_memfd() and set_user_memory_region2()` only when `tee` is enabled.

1. Add the handling of `KVM_MEMORY_FAULT` only for `tee`, which includes the extra thread for handling shared to private and viceverse.

2. `Tee` should be replaced with `tee and x86-64` since all `tee` is for `x86-64`.

I don't know if this is idiomatic by any means, and it depends on whether you can have target arch-specific features, but could we have something like tee-x86 and tee? tee-x86 for the x86 specific functionality, and then tee for the shared bits? Would that be cleaner than just writing #[cfg(all(feature = "tee", target_arch = "x86_64"))]? Again, I don't know if that's "proper" Rust or even possible.

I do not know either. I though that tee and x86 would have code that tdx and snv share, if that exists.

3. Add CCA support. This shall be added with `cca and tee` enabled.

4. Eventually some code will be shared for `tee` in `x86-64` and `aarch64` but I can't see it yet.

@jakecorrenti @tylerfanelli @slp WDYT?

Other than my question above, that all sounds good to me.

@MatiasVara
Copy link
Collaborator Author

MatiasVara commented Mar 18, 2025

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

  1. Add Use create_guest_memfd() and set_user_memory_region2() only when tee is enabled.

I don't think set_user_memory_region2 should be tee only, as I believe that on newer kernels, set_user_memory_region is deprecated.

That's OK. I could just leave set_user_memory_region2 for the future.

  1. Add the handling of KVM_MEMORY_FAULT only for tee, which includes the extra thread for handling shared to private and viceverse.

I'm addressing that in my SEV-SNP PR currently.

The first two items in my list are for all tee but they were in my PR for almost a year already. In any case, I need to split those chunks from my PR.

  1. Tee should be replaced with tee and x86-64 since all tee is for x86-64.

Makes sense to me.

  1. Add CCA support. This shall be added with cca and tee enabled.

Do you mean aarch64 and tee?

Yes for the moment.

When tee feature is enabled, use create_guest_memfd() and
set_user_memory_region2(). The created regions are marked as private by
using set_memory_attributes() thus imitating QEMU behavior.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
These descriptors are required for handling the KVM_FAULT_MEMORY vmexit.
Although this vmexit is only present when the tee feature is enabled,
changing for a reference does not seem the right thing to do by using
feratures. Use reference and protect it to be able to share these
descriptors. These changes prepare the code for handling
KVM_EXIT_FAULT_MEMORY

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
The KVM_EXIT_MEMORY_FAULT vmexit is triggered when guest wants to switch
a region of memory from private to shared and viceversa. To support this
when tee is enabled, add an extra thread named sender_io that gets the
parameters from the vcpu thread and triggers the
set_memory_properties(). The vcpu fd is owned only by this thread.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
This commit will be removed when changes are in upstream.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
The `tee` feature has been used by having in mind only the x86-64 case.
Currently, `tee` means sense only if `x86-64` is also enabled. In the
future, the `tee` will enable code that is shared by arm and x86-64.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
Enable to build confidential guests using ARM CCA (Confidential
Computing Architecture). This work relies on v7 series for Linux and v5
series for KVM. This has been tested only on the corresponding FVP model
simulator. For testing, you require specific kvm-ioctls and kvm-bindings
crates.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
This commit will be remove when cca use virtio-blk. For the moment,
enable virtio-fs in cca.

Signed-off-by: Matias Ezequiel Vara Larsen <[email protected]>
@MatiasVara
Copy link
Collaborator Author

MatiasVara commented Mar 19, 2025

After reviewing the Jake comments I came to the conclusion that this PR should be split otherwise is hard to review:

  1. Add Use create_guest_memfd() and set_user_memory_region2() only when tee is enabled.
  2. Add the handling of KVM_MEMORY_FAULT only for tee, which includes the extra thread for handling shared to private and viceverse.
  3. Tee should be replaced with tee and x86-64 since all tee is for x86-64.
  4. Add CCA support. This shall be added with cca and tee enabled.
  5. Eventually some code will be shared for tee in x86-64 and aarch64 but I can't see it yet.

@jakecorrenti @tylerfanelli @slp WDYT?

I just split the PR into a few commits. It is the same code though. The first three commits are independent of cca and should be shared by all tees. Also [Enable tee only in x86-64 shall for all tees since it aims at separating the code for tee and x86 and tee and aach64.

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.

3 participants