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

wip: draft porting guide #81

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

wip: draft porting guide #81

wants to merge 1 commit into from

Conversation

josecm
Copy link
Member

@josecm josecm commented Jan 19, 2024

This PR details the steps for porting the Bao Hypervisor to a new platform. Still WIP, but any feedback is welcome.

@Diogo21Costa Diogo21Costa self-requested a review January 29, 2024 16:32
Comment on lines +4 to +8
The Bao Hypervisor only contains architecture-specific drivers (e.g., interrupt controllers or
IOMMUS). Therefore porting the hypervisor essentially resumes to two simple operation: creating a
platform description folder containing a platform definition source and header files which
indicates, among others, the number of CPUs or the memory available. The exception is an UART driver
used fore logging, so if a driver is not available for a UART on the target platform this might
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, the first sentence doesn't contribute significantly. It looks like a bit disconnected from the remaining text.

Suggested change
The Bao Hypervisor only contains architecture-specific drivers (e.g., interrupt controllers or
IOMMUS). Therefore porting the hypervisor essentially resumes to two simple operation: creating a
platform description folder containing a platform definition source and header files which
indicates, among others, the number of CPUs or the memory available. The exception is an UART driver
used fore logging, so if a driver is not available for a UART on the target platform this might
Porting the hypervisor involves two basic steps: (i) creating a platform description folder with
source and header files that specify platform details, such as the number of CPUs or available
memory, and (ii) integrating a UART driver used for logging, so if a driver is not available for a
UART on the target platform this might require writing a custom driver.

Comment on lines +9 to +10
require writing such a driver. Finally, given for some platforms provide a custom mechanism to
define DMA device Master IDs required for configuring IOMMUs, one might require to configure
Copy link
Member

Choose a reason for hiding this comment

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

At this point, it would be interesting to refer to some examples to contextualize the firmware changes

Comment on lines +20 to +21
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
*src/platform*. For example, for port to a *foo* platform, create the directory *src/platform/foo*.
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
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
*src/platform*. For example, for port to a *foo* platform, create the directory *src/platform/foo*.
Creating a new platform description for the Bao Hypervisor begins by establishing a new directory
under src/platform. For instance, when porting to a foo platform, generate the directory
src/platform/foo.

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
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
*src/platform*. For example, for port to a *foo* platform, create the directory *src/platform/foo*.
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
``src/platform``. For example, for port to a *foo* platform, create the directory ``src/platform/foo``.

Comment on lines +97 to +98
- **base** [mandatory] - Is the base physical address of that memory region. Must be aligned to the minimum architecture's page size;
- **size** [mandatory] - The size of the memory region. Must be aligned to the minimum architecture's page size;
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
- **base** [mandatory] - Is the base physical address of that memory region. Must be aligned to the minimum architecture's page size;
- **size** [mandatory] - The size of the memory region. Must be aligned to the minimum architecture's page size;
- **base** [mandatory] - Is the base physical address of that memory region.
- **size** [mandatory] - The size of the memory region.
.. warning::
Ensure that the memory ``base`` and ``size`` are aligned to the minimum architecture's page size.

Copy link
Member

Choose a reason for hiding this comment

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

Change the format to inline code.

Comment on lines +100 to +101
Note that, by convention, we do not add on-chip/scratcpad SRAMs as a platform memory region as the hypervisor as an uniform view of memory.
This regions should be assigned to guests in the form of devices.
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
Note that, by convention, we do not add on-chip/scratcpad SRAMs as a platform memory region as the hypervisor as an uniform view of memory.
This regions should be assigned to guests in the form of devices.
.. note::
By convention, we do not add on-chip/scratcpad SRAMs as a platform memory region as the hypervisor as an uniform view of memory. This regions should be assigned to guests in the form of devices.

@danielRep danielRep self-assigned this Feb 14, 2024
@bao-project bao-project deleted a comment from Diogo21Costa Feb 14, 2024
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.

I don't have any major suggestions to add on the structure of the document. I considerer it already touches all the necessary topics.
Few points:

  • inline code format must be used be used for file paths, file names, shell commands, programming language directives/keywords/identifiers, syntax characters as required in the Documentation Guidelines;
  • when using tabs, the contents of each tab must one step indented from the ..tab: directive;
  • an image from the folder structure of a platform definition would be helpful

Comment on lines +13 to +15
1) Create a platform directory containing a platform description [mandatory];
2) Writing an UART driver [if the driver for the target UART is not available];
3) Setting up firmware to define DMA device Master IDs used by the platform IOMMU [platform dependent];
Copy link
Member

Choose a reason for hiding this comment

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

@Diogo21Costa suggested to add here reference links to each section on the document detailing each process. I deleted the comment by mistake. I would also change the [] for ()

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

Comment on lines +24 to +29
1) A source file (e.g., *src/platform/foo/foo_desc.c*) containing the platform description structure, `struct platform platform`
defininition;
2) A *plat.h* header file under *src/platform/foo/inc/plat/* with platform-specific includes and definitions;
3) A *platform.mk* makefile containing definitions required by the build system (e.g., the target architecture);
4) A *objects.mk* containing the platform's target object files. In principle this should only contain the
object for the source description file, but you may add others if you have any other platform specific sources.
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
1) A source file (e.g., *src/platform/foo/foo_desc.c*) containing the platform description structure, `struct platform platform`
defininition;
2) A *plat.h* header file under *src/platform/foo/inc/plat/* with platform-specific includes and definitions;
3) A *platform.mk* makefile containing definitions required by the build system (e.g., the target architecture);
4) A *objects.mk* containing the platform's target object files. In principle this should only contain the
object for the source description file, but you may add others if you have any other platform specific sources.
1) A source file (e.g., ``src/platform/foo/foo_desc.c``) containing the platform description structure, `struct platform platform`
definition;
2) A ``plat.h`` header file under ``src/platform/foo/inc/plat/`` with platform-specific includes and definitions;
3) A ``platform.mk`` makefile containing definitions required by the build system (e.g., the target architecture);
4) A ``objects.mk`` containing the platform's target object files. In principle this should only contain the
object for the source description file, but you may add others if you have any other platform specific sources.

Comment on lines +20 to +21
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
*src/platform*. For example, for port to a *foo* platform, create the directory *src/platform/foo*.
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
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
*src/platform*. For example, for port to a *foo* platform, create the directory *src/platform/foo*.
Creating a new platform description for the Bao Hypervisor starts by creating a new directory under
``src/platform``. For example, for port to a *foo* platform, create the directory ``src/platform/foo``.

Platform Description Structure
******************************

A global structure of type `struct platform` named `platform` must be defined when porting Bao to a new platform.
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
A global structure of type `struct platform` named `platform` must be defined when porting Bao to a new platform.
A global structure of type ``struct platform`` named ``platform`` must be defined when porting Bao to a new platform.

Comment on lines +1 to +2
Porting
=======
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
Porting
=======
Porting Guide
=============

Comment on lines +110 to +111
Architcture-Specific Platform Description
#########################################
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
Architcture-Specific Platform Description
#########################################
Architecture-Specific Platform Description
##########################################

Comment on lines +119 to +162

.. code-block:: c

struct arch_platform {
struct gic_dscrp {
paddr_t gicc_addr;
paddr_t gich_addr;
paddr_t gicv_addr;
paddr_t gicd_addr;
paddr_t gicr_addr;

irqid_t maintenance_id;
} gic;

struct smmu_dscrp {
paddr_t base;
streamid_t global_mask;
} smmu;

struct clusters {
size_t num;
size_t* core_num;
} clusters;
};

Where, for the GIC interrupt controller `struct gic_dscrp` description:

- **gic.gicc_addr** [mandatory for GICv2 platforms] - base address for the GIC's CPU Interface;
- **gic.gich_addr** [mandatory for GICv2 platforms] - base address for the GIC's Virtual Interface Control Registers;
- **gic.gicv_addr** [mandatory for GICv2 platforms] - base address for the GIC's Virtual CPU Interface;
- **gic.gicd_addr** [mandatory] - base address for the GIC's Distributor;
- **gic.gicr_addr** [mandatory for GICv3/4 platforms] - base address for the GIC's Redistributor;
- **gic.maintenance_id** [mandatory] - The interrupt ID for the GIC's maintenance interrupt;

For the SMMU `struct smmu_dscrp`:

- **smmu.base** [mandatory] - is the base address for the SMMU;
- **smmu.global_mask** [optional; only valid for SMMUv2] - a mask to be applied to all SMMUv2's Stream Match Registers;


Finally, when CPUs are organized in clusters, in the Arm architecture their IDs are assigned using an hierarchical schema.
To be able to calculate the linearized ID for each core, we require the port to provide the number of CPUs of cluster in
ascending order of AFF1.

Copy link
Member

Choose a reason for hiding this comment

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

This needs an extra indentation to be grouped on each tab. Same for the RISC-V information.

Comment on lines +152 to +157

For the SMMU `struct smmu_dscrp`:

- **smmu.base** [mandatory] - is the base address for the SMMU;
- **smmu.global_mask** [optional; only valid for SMMUv2] - a mask to be applied to all SMMUv2's Stream Match Registers;

Copy link
Member

Choose a reason for hiding this comment

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

Use inline code format for all code related text.

- **gic.gicd_addr** [mandatory] - base address for the GIC's Distributor;
- **gic.gicr_addr** [mandatory for GICv3/4 platforms] - base address for the GIC's Redistributor;
- **gic.maintenance_id** [mandatory] - The interrupt ID for the GIC's maintenance interrupt;

Copy link
Member

Choose a reason for hiding this comment

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

I would split this for each GIC version to be more explicit.

For all platforms:
- **gic.maintenance_id**
- **gic.gicd_addr**

For GICv2 platforms:
- **gic.gicc_addr**
- **gic.gich_addr**
- **gic.gicv_addr**

For GICv3/4 platforms:
- **gic.gicr_addr**

Comment on lines +221 to +222
Platform Make Defintions
************************
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
Platform Make Defintions
************************
Platform Make Definitions
*************************

Signed-off-by: Jose Martins <josemartins90@gmail.com>
Comment on lines +236 to +244
.. tabs::
.. tab:: Arm

- `GIC_VERSION` [mandatory] - indicates the GIC's version present on the platform. Option: `GICV2`, or `GICV3`.

.. tab:: RISC-V

- `IRQC` [mandatory] - indicates the interrupt controller available on the platform. Options: `PLIC`, `APLIC`, or `AIA`.

Copy link
Member

Choose a reason for hiding this comment

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

Currently not working

Comment on lines +13 to +15
1) Create a platform directory containing a platform description [mandatory];
2) Writing an UART driver [if the driver for the target UART is not available];
3) Setting up firmware to define DMA device Master IDs used by the platform IOMMU [platform dependent];
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

Comment on lines +24 to +29
1) A source file (e.g., *src/platform/foo/foo_desc.c*) containing the platform description structure, `struct platform platform`
defininition;
2) A *plat.h* header file under *src/platform/foo/inc/plat/* with platform-specific includes and definitions;
3) A *platform.mk* makefile containing definitions required by the build system (e.g., the target architecture);
4) A *objects.mk* containing the platform's target object files. In principle this should only contain the
object for the source description file, but you may add others if you have any other platform specific sources.
Copy link
Member

Choose a reason for hiding this comment

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

List is not working


The architecture-specific field of the platform description includes fields that describe interrupt controllers or others.

.. tabs::
Copy link
Member

Choose a reason for hiding this comment

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

Tabs not working

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