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

feat(doc): add guest configuration to bao-classic config documentation #78

Open
wants to merge 70 commits into
base: wip/bao-classic_config
Choose a base branch
from

Conversation

Diogo21Costa
Copy link
Member

PR Description

In this pull request, I'm adding content to the "Guest Configuration" section of the config documentation for the Bao hypervisor.

  1. Guest Image Configuration
  2. VM Configuration (Virtual Platform)
  3. CPU Affinity (Mapping of Virtual to Physical CPUs)
  4. Cache Coloring Setup

The focus here is to provide a comprehensive description for each field, offering detailed insights into their functionalities.

Do you have any suggestions or feedback concerning the content provided in the "Guest Configuration" section?

@Diogo21Costa Diogo21Costa changed the title Feat/bao classic config guests feat(doc): add guest configuration to bao-classic config documentation Dec 7, 2023
@danielRep danielRep self-assigned this Dec 7, 2023
@Diogo21Costa Diogo21Costa force-pushed the feat/bao-classic_config_guests branch from f80f24e to 77c4429 Compare December 7, 2023 18:29
@josecm
Copy link
Member

josecm commented Dec 7, 2023

I think @DavidMCerdeira should also participate in these reviews...

@josecm josecm requested a review from DavidMCerdeira December 7, 2023 18:38
Copy link
Member

@josecm josecm left a comment

Choose a reason for hiding this comment

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

I think we need a few iterations on this one. And possibly discuss an alternative structure where we go into details and have text explaining the features and functionalities, while here we only go field by field as you did.

Copy link
Member

Choose a reason for hiding this comment

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

The cpu affinities in the image are wrong. For example, it should be 0x1 or 0b0001 for VM 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in bae0e3311ec85b598de114a3e05260ba4d109151.

:name: guest-config-fig


The configuration of different guests is done by populating a struct called *vmconfig*, as follows:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here we could add a reference the vmlist referenced in the first part of the document, saying that each entry in that list is a vm_config struct...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in 923bbca5719fda58025bfc192b7461efa1d1a1c4.

.. code-block:: c

struct vm_config {
struct image;
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. I know that the structure used for the image is an anonymous structure, but maybe we can change this is to:

Suggested change
struct image;
struct vm_image image;

(both here and in the code)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in fadc7a2bb90adc5f6d6fd4eebad36337f2748c39.

Comment on lines 127 to 128
- **load_addr** [mandatory] - corresponds to the ``image`` load address in the hypervisor address
space. This value can be defined using the macro VM_IMAGE_OFFSET(img_name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **load_addr** [mandatory] - corresponds to the ``image`` load address in the hypervisor address
space. This value can be defined using the macro VM_IMAGE_OFFSET(img_name);
- **load_addr** [mandatory] - corresponds to the ``image`` load address in physical memory/physical address space. This value can be defined using the macro VM_IMAGE_OFFSET(img_name);

Copy link
Member

Choose a reason for hiding this comment

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

Also, we need to explain that, if separately loaded is set to false, the hypervisor initially interprets this as the offset of the builtin image guest in its own image (hence, VM_IMAGE_OFFSET). At runtime, it fixes the value to be later interpreted as a physical address, by adding to it the address the hypervisor was loaded.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in 4b057cd.

Comment on lines 129 to 130
- **size** [mandatory] - corresponds to the image size. This value can be defined using the macro
VM_IMAGE_SIZE(img_name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **size** [mandatory] - corresponds to the image size. This value can be defined using the macro
VM_IMAGE_SIZE(img_name);
- **size** [mandatory] - corresponds to the image size. For builtin images declared using `VM_IMAGE`, this value can be defined using the macro VM_IMAGE_SIZE(img_name);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed in dde0f11fddf139eee0aa33223d587b912c60aecd.

Comment on lines 224 to 225
- **base** [mandatory] - corresponds to the virtual base address of the IPC memory region;
- **size** [mandatory] - corresponds to the size of the IPC memory region;
Copy link
Member

Choose a reason for hiding this comment

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

Again, it is mandatory for base and size to be aligned to the architecture's smallest page size: in all architectures 4K for MMU systems and 64 bytes for MPU.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanls, fixed in 404de5a.

Comment on lines 202 to 204
3. Inter-Process Communication (IPC)
####################################
.. _Inter-Process Communication (IPC):
Copy link
Member

Choose a reason for hiding this comment

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

We need to explain somewhere how the IPC works 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, included new content in 854fe14.

Comment on lines 257 to 258
- **va** [mandatory] - corresponds to the base virtual address of the device;
- **size** [mandatory] - corresponds to the size of the device memory region;
Copy link
Member

Choose a reason for hiding this comment

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

Again, explain that it is mandatory for base and size to be aligned to the architecture's smallest page size: in all architectures 4K for MMU systems and 64 bytes for MPU.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, addressed in ae5a5bd.

the VM. By default, ``interrupt_num`` equals to 0;
- **interrupts** [mandatory if *interrupt_num*>0] - defines a list of interrupt IDs generated by
the device - ``(irqid_t[]) {irq_1, ..., irq_n};``
- **id** [optional] - corresponds to the bus master id for iommu effects:
Copy link
Member

Choose a reason for hiding this comment

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

Which should also add more explanation to this, possibly on a per-platform basis.

- **cpu_affinity** [optional] - corresponds to a bitmap signaling the preferred physical CPUs
assigned to the VM. If this value is mutually exclusive for all the VMs, the physical CPUs
assigned to each VM follow the bitmap. Otherwise (in case of bit overlap or lack of affinity
definition), the CPU assignment is defined by the hypervisor;
Copy link
Member

Choose a reason for hiding this comment

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

the cpu affinity will be as best as possible be followed, but it is not guaranteed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, added this information!

@DavidMCerdeira DavidMCerdeira self-requested a review September 4, 2024 15:41
DavidMCerdeira
DavidMCerdeira previously approved these changes Sep 4, 2024
danielRep
danielRep previously approved these changes Sep 4, 2024
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
@DavidMCerdeira DavidMCerdeira self-requested a review September 17, 2024 14:01
Copy link
Member

@DavidMCerdeira DavidMCerdeira left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Only thing missing is re-writing the history which should be done after addressing all pending conversations

Copy link
Member

@danielRep danielRep left a comment

Choose a reason for hiding this comment

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

Apart from minor issues easily solved, it looks very good to me! Great work Diogo.

source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
source/bao_hyp/config.rst Outdated Show resolved Hide resolved
danielRep
danielRep previously approved these changes Sep 19, 2024

6. Architectural-Specific Configurations
.. note::
Copy link
Member

Choose a reason for hiding this comment

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

Add a second note that we shouldn't add the gic to these devices.
A scenario would be where we want to give a large portion of the MMIO, and that MMIO range contains the GIC. In this case we need to split the MMIO range in two creating a hole for the gic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the review @DavidMCerdeira! Addressed this point in this commit

Comment on lines +488 to +490
When mapping MMIO regions for guests, the memory regions associated with the GIC (Generic
Interrupt Controller) must be excluded. Mapping these regions can lead to conflicts or incorrect
behavior, as they are typically managed by Bao through trap-and-emulate mechanisms.
Copy link
Member

Choose a reason for hiding this comment

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

When mapping MMIO regions for guests, the memory regions associated with the interrupt controller (only interrupt controllers are target of MMIO trap and emulate in Bao) must be excluded. Mapping these regions can lead to conflicts or incorrect
behavior, as they are typically managed by Bao through trap-and-emulate mechanisms.

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.

4 participants