bao-project / bao-hypervisor

Bao, a Lightweight Static Partitioning Hypervisor
Apache License 2.0
379 stars 128 forks source link

feat(core/remio): add Remote I/O infrastructure #176

Closed joaopeixoto13 closed 2 weeks ago

joaopeixoto13 commented 2 months ago

This PR introduces the foundational support for VirtIO by implementing a mechanism to forward VirtIO requests bidirectionally between the guest VM (or frontend VM) and its associated backend VM.

Validation

joaopeixoto13 commented 2 months ago

This new version includes a refactor of the code following @DavidMCerdeira's suggestions. The key changes are as follows:

This refactoring improves the structure and maintainability of the code.

joaopeixoto13 commented 2 months ago

Generic comment: the namespace for this module can be shortened. I suggest rmio, remio, rm_io, rem_io

some of the comments may be a bit subjective. @josecm feel free to wage in

@josecm What are your thoughts on this? From @DavidMCerdeira's list of suggestions, remio seems like the most appropriate name. However, I'm unsure if remote_io is still considered too lengthy. For this reason, I've retained the current name in the latest version.

josecm commented 2 months ago

Generic comment: the namespace for this module can be shortened. I suggest rmio, remio, rm_io, rem_io some of the comments may be a bit subjective. @josecm feel free to wage in

@josecm What are your thoughts on this? From @DavidMCerdeira's list of suggestions, remio seems like the most appropriate name. However, I'm unsure if remote_io is still considered too lengthy. For this reason, I've retained the current name in the latest version.

My suggestion would be either remio or simply rio.

joaopeixoto13 commented 2 months ago

Generic comment: the namespace for this module can be shortened. I suggest rmio, remio, rm_io, rem_io some of the comments may be a bit subjective. @josecm feel free to wage in

@josecm What are your thoughts on this? From @DavidMCerdeira's list of suggestions, remio seems like the most appropriate name. However, I'm unsure if remote_io is still considered too lengthy. For this reason, I've retained the current name in the latest version.

My suggestion would be either remio or simply rio.

Update: https://github.com/bao-project/bao-hypervisor/pull/176/commits/01c2cbed6e573c28af1b82229e798d1298da16a6

DavidMCerdeira commented 2 months ago

Generic comment: the namespace for this module can be shortened. I suggest rmio, remio, rm_io, rem_io some of the comments may be a bit subjective. @josecm feel free to wage in

@josecm What are your thoughts on this? From @DavidMCerdeira's list of suggestions, remio seems like the most appropriate name. However, I'm unsure if remote_io is still considered too lengthy. For this reason, I've retained the current name in the latest version.

My suggestion would be either remio or simply rio.

Update: 01c2cbe

The namespace change should be reflected in the function, variable names, enums, etc

joaopeixoto13 commented 2 months ago

Generic comment: the namespace for this module can be shortened. I suggest rmio, remio, rm_io, rem_io some of the comments may be a bit subjective. @josecm feel free to wage in

@josecm What are your thoughts on this? From @DavidMCerdeira's list of suggestions, remio seems like the most appropriate name. However, I'm unsure if remote_io is still considered too lengthy. For this reason, I've retained the current name in the latest version.

My suggestion would be either remio or simply rio.

Update: 01c2cbe

The namespace change should be reflected in the function, variable names, enums, etc

Done https://github.com/bao-project/bao-hypervisor/pull/176/commits/00b5acdefe83a3f03f83816ffe7b51133b892cc6

joaopeixoto13 commented 2 months ago

I'd still like to test this before approving, but for now it looks good! 👍 You can squash the commits into a single one now

Thanks, @DavidMCerdeira ! Also, please take a look at this commit, where I temporarily resolved the issue by replacing the psci_power_down call with a WFI instruction. However, I believe we need to find a more appropriate long-term solution. I haven't reviewed the current guest reboot PR yet, but it may offer some insight or help in addressing this issue.

DavidMCerdeira commented 2 months ago

I'd still like to test this before approving, but for now it looks good! 👍 You can squash the commits into a single one now

Thanks, @DavidMCerdeira ! Also, please take a look at this commit, where I temporarily resolved the issue by replacing the psci_power_down call with a WFI instruction. However, I believe we need to find a more appropriate long-term solution. I haven't reviewed the current guest reboot PR yet, but it may offer some insight or help in addressing this issue.

I think for now it is fine. Actually we are aware of at least two platforms that require this behavior to work with bao currently. We should eventually allow platforms to have specific code to handle this sort of platform specific quirks

josecm commented 3 weeks ago

@joaopeixoto13, to keep a clean history, try as much as possible remove the "fix" commits and introduce the change correctly in the first commit that code is introduced.

joaopeixoto13 commented 3 weeks ago

@joaopeixoto13, to keep a clean history, try as much as possible remove the "fix" commits and introduce the change correctly in the first commit that code is introduced.

Yes, I understand. I split these smaller fixes to make it easier for you to review the changes. At the end of the PR, I’ll rebase all the fix commits into the main "parent" commit that introduced the feature.

josecm commented 3 weeks ago

@joaopeixoto13 rebase on main and resolve conflicts please

joaopeixoto13 commented 3 weeks ago

@joaopeixoto13 rebase on main and resolve conflicts please

Done. If everything looks good, I can proceed to merge all the fix commits into the main parent commit that introduced the feature.

josecm commented 3 weeks ago

Done. If everything looks good, I can proceed to merge all the fix commits into the main parent commit that introduced the feature.

Please go ahead with that.

joaopeixoto13 commented 3 weeks ago

Done. If everything looks good, I can proceed to merge all the fix commits into the main parent commit that introduced the feature.

Please go ahead with that.

Done ✅