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.

Sign up for GitHub

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

Jump to bottom

Update get/set Standard Registers to use VP register page #183

Merged
merged 5 commits into from
Feb 3, 2025
Merged

Update get/set Standard Registers to use VP register page #183

merged 5 commits into from
Feb 3, 2025

Conversation

russell-islam
Copy link
Collaborator

Summary of the PR

Please summarize here why the changes in this PR are needed.

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.

@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch from 53c3cea to 349a0b5 Compare January 9, 2025 07:55
@russell-islam russell-islam changed the title Update get/set Standard register to use VP register page Update get/set Standard Registers to use VP register page Jan 9, 2025
@russell-islam
Copy link
Collaborator Author

russell-islam commented Jan 9, 2025 •
edited
Loading

@jinankjain What do you think about arm64? I added the wrapper struct to lib.rs, I am open to add a similar regs.rs to arm64 and add the RegisterPage wrapper struct there.

@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch from 349a0b5 to 05d26e5 Compare January 9, 2025 20:31
@jinankjain
Copy link
Collaborator

Yes there would be similar regs for ARM64, but I haven't finalized the set of registers we would need.

NunoDasNeves
NunoDasNeves requested changes Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@NunoDasNeves NunoDasNeves left a comment

Choose a reason for hiding this comment

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

It looks like we need to handle the case where the register page is unavailable, e.g. in encrypted partitions.

mshv-bindings/src/lib.rs Outdated Show resolved Hide resolved
mshv-ioctls/src/ioctls/vcpu.rs Outdated Show resolved Hide resolved
mshv-ioctls/src/ioctls/vcpu.rs Outdated Show resolved Hide resolved
mshv-ioctls/src/ioctls/vcpu.rs Show resolved Hide resolved
@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch 3 times, most recently from 5d883df to 5a74c89 Compare January 12, 2025 22:54
@russell-islam
Copy link
Collaborator Author

@NunoDasNeves Please take another look.

@liuw
Copy link
Member

liuw commented Jan 13, 2025

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?

We want to be careful here. The kernel may decide some day that it also wants to touch that page.

@russell-islam
Copy link
Collaborator Author

russell-islam commented Jan 13, 2025 •
edited
Loading

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?

We want to be careful here. The kernel may decide some day that it also wants to touch that page.

I do not see any such feature in KVM. @NunoDasNeves What is the kernel scenario in our driver code. Can the driver change the page contents?

@liuw
Copy link
Member

liuw commented Jan 14, 2025

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?
We want to be careful here. The kernel may decide some day that it also wants to touch that page.

I do not see any such feature in KVM. @NunoDasNeves What is the kernel scenario in our driver code. Can the driver change the page contents?

It is all very nebulous in my head, but supposedly one day we may decide we want to do code emulation in Linux kernel, or make the kernel to directly inspect / modify guest register states.

We need to be thorough here. If such use cases arise, we need to make sure correctness is maintained. With the current code, the VMM blindly maps the page. It may or may not be okay. I'm looking for the reasoning why the current scheme is okay; if not, what needs to be done.

@russell-islam
Copy link
Collaborator Author

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?
We want to be careful here. The kernel may decide some day that it also wants to touch that page.

I do not see any such feature in KVM. @NunoDasNeves What is the kernel scenario in our driver code. Can the driver change the page contents?

It is all very nebulous in my head, but supposedly one day we may decide we want to do code emulation in Linux kernel, or make the kernel to directly inspect / modify guest register states.

We need to be thorough here. If such use cases arise, we need to make sure correctness is maintained. With the current code, the VMM blindly maps the page. It may or may not be okay. I'm looking for the reasoning why the current scheme is okay; if not, what needs to be done.

The whole purpose of this PR is to speed up the boot process. Mainly the emulation code is accessing GP and Control registers. If you move the emulation in the Kernel later we might remove this mapping from the VMM.

@liuw
Copy link
Member

liuw commented Jan 14, 2025

Actually the code as-is already makes a case for gating that feature with a flag. Mapping the register page can be separated from the guest type.

  1. You don't need to carry the knowledge of the guest type in that particular code path.
  2. You don't prevent a possible improvement in the future if hardware allows you to access a snapshot of the register page for an encrypted guest.

In general, you should always provide a mechanism to discover features across component boundary.

@russell-islam
Copy link
Collaborator Author

russell-islam commented Jan 14, 2025 •
edited
Loading

Actually the code as-is already makes a case for gating that feature with a flag. Mapping the register page can be separated from the guest type.

1. You don't need to carry the knowledge of the guest type in that particular code path.

2. You don't prevent a possible improvement in the future if hardware allows you to access a snapshot of the register page for an encrypted guest.

In general, you should always provide a mechanism to discover features across component boundary.

For encrypted guest, there is no register page in the driver. that means mmap will fail and we have to store None.
In your comment I am not sure about the flag you are referring.

@skinsbursky
Copy link
Collaborator

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?

We want to be careful here. The kernel may decide some day that it also wants to touch that page.

Why this might be a problem from your POV?
Kernel does touch the page to inject the interrupt vectors upon VP dispatch and it's not mutually exclusive with other potential setters.
At the end it's just a memory page with some numbers until the VP is dispatched, isn't it?

@russell-islam
Copy link
Collaborator Author

After a discussion with Wei, I think we need to have some feature sets returned from the Kernel driver and make appropriate features available in the VMM. As a result, I am making this PR draft and continue to work on the Kernel and resume here once Kernel implementation is done.

@russell-islam russell-islam changed the title Update get/set Standard Registers to use VP register page [WIP] Update get/set Standard Registers to use VP register page Jan 14, 2025
@russell-islam russell-islam marked this pull request as draft January 14, 2025 23:47
@liuw
Copy link
Member

liuw commented Jan 15, 2025

If KVM supports a similar interface, can you check if they require the VMM to explicitly opt-in / enable to use this feature?
We want to be careful here. The kernel may decide some day that it also wants to touch that page.

Why this might be a problem from your POV? Kernel does touch the page to inject the interrupt vectors upon VP dispatch and it's not mutually exclusive with other potential setters. At the end it's just a memory page with some numbers until the VP is dispatched, isn't it?

When two entities modify the same page in an overlapping manner, there needs to be a way to synchronize them. We don't have a clear model yet.

Having a mechanism to clearly denote what feature is available gives us some leeway if something happens in the future.

@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch 5 times, most recently from ddd96b1 to 9176b81 Compare January 28, 2025 20:21
@russell-islam russell-islam marked this pull request as ready for review January 28, 2025 20:24
@russell-islam russell-islam changed the title [WIP] Update get/set Standard Registers to use VP register page Update get/set Standard Registers to use VP register page Jan 28, 2025
@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch from 9176b81 to dd9c281 Compare January 28, 2025 22:11
@russell-islam
Copy link
Collaborator Author

russell-islam commented Jan 28, 2025 •
edited
Loading

Generic comment:
We don't plan to implement any API for getting or setting registers in the RegisterPage struct, as we can't verify whether the address inside the struct is a valid MMAP. Instead, we aim to handle this within the VCPU, assuming that the VCPU is created and that the MMAP is either valid or None.

NunoDasNeves
NunoDasNeves approved these changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@NunoDasNeves NunoDasNeves left a comment

Choose a reason for hiding this comment

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

LGTM

@russell-islam
Copy link
Collaborator Author

@liuw PTAL

jinankjain
jinankjain reviewed Jan 29, 2025
View reviewed changes
mshv-ioctls/Cargo.toml Outdated Show resolved Hide resolved
@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch from dd9c281 to 7a44f34 Compare January 29, 2025 20:53
@russell-islam russell-islam enabled auto-merge (rebase) January 29, 2025 20:57
@russell-islam
Copy link
Collaborator Author

@jinankjain Could you please take a look and approve if there is no issue?

@russell-islam
mshv-bindings: add wrapper struct around hv_vp_register_page
4be3265 
Define a new struct RegisterPage with mutable pointer
to hv_vp_register_page.

Signed-off-by: Muminul Islam <[email protected]>
@russell-islam
mshv-{bindings-ioctls}: redefine vcpu struct and new_vcpu function
0f01530 
Include the register page in vcpu object for later use
to read and write the VP registers.

Signed-off-by: Muminul Islam <[email protected]>
@russell-islam
mshv-ioctls: add an API to return register page
c324dc0 
One more API to VCPU implementation sin signature
get_vp_reg_page, that return mutable reference to
VP register page. This API could be used by VMM or other
VCPU implementations.

Signed-off-by: Muminul Islam <[email protected]>
@russell-islam
mshv-ioctls: add new API to set standard registers
8519bb6 
This API use mapped VP register page to set the registers.

Signed-off-by: Muminul Islam <[email protected]>
@russell-islam
mshv-ioctls: add a new API to get standard registers
f98af94 
This API uses VP register page to populate the registers.

Signed-off-by: Muminul Islam <[email protected]>
@russell-islam russell-islam force-pushed the muislam/reg-page-initial branch from 7a44f34 to f98af94 Compare January 30, 2025 21:36
@russell-islam russell-islam merged commit 535a84d into main Feb 3, 2025
6 checks passed
@russell-islam russell-islam deleted the muislam/reg-page-initial branch February 3, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuw liuw liuw left review comments

@jinankjain jinankjain jinankjain approved these changes

@NunoDasNeves NunoDasNeves NunoDasNeves approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@russell-islam @jinankjain @liuw @skinsbursky @NunoDasNeves