On 2016-10-05 14:11, Claudio Scordino wrote:
> This patch contains a few general comments in the source code of the
> core subsystem.
>
> Signed-off-by: Claudio Scordino <[email protected]>
> ---
> hypervisor/control.c | 8 ++++++++
> hypervisor/include/jailhouse/cell-config.h | 7 +++++++
> hypervisor/include/jailhouse/header.h | 17 +++++++++++++----
> hypervisor/paging.c | 8 ++++++++
> hypervisor/setup.c | 13 +++++++++++++
> 5 files changed, 49 insertions(+), 4 deletions(-)
>
> diff --git a/hypervisor/control.c b/hypervisor/control.c
> index bdb8eae..a953891 100644
> --- a/hypervisor/control.c
> +++ b/hypervisor/control.c
> @@ -69,6 +69,10 @@ bool cpu_id_valid(unsigned long cpu_id)
> test_bit(cpu_id, system_cpu_set));
> }
>
> +/**
Same remark as on patch: "/**" requires conforming comments or just "/*"
for an informal description.
> + * Suspend all CPUs assigned to the cell except the one executing
> + * the function (if it is in the cell's CPU set) to prevent races.
If you want to make it round, link to cell_resume as well.
> + */
> static void cell_suspend(struct cell *cell, struct per_cpu *cpu_data)
> {
> unsigned int cpu;
> @@ -401,6 +405,7 @@ static int cell_create(struct per_cpu *cpu_data, unsigned
> long config_address)
> if (err)
> goto err_cell_exit;
>
> + /* Shrinking: the new cell's CPUs are removed from root CPU set */
...are parked and removed and assigned to the new cell and get their
stats cleared. Misleading.
> for_each_cpu(cpu, cell->cpu_set) {
> arch_park_cpu(cpu);
>
> @@ -438,6 +443,8 @@ static int cell_create(struct per_cpu *cpu_data, unsigned
> long config_address)
>
> cell->comm_page.comm_region.cell_state = JAILHOUSE_CELL_SHUT_DOWN;
>
> + /* The new cell is added to the cells list (which is singly linked list
> + * having linux_cell as its head) */
More precisely, it's added to the end of the cell list. The structure of
the list likely better described where it is defined.
> last = &root_cell;
> while (last->next)
> last = last->next;
> @@ -692,6 +699,7 @@ static int shutdown(struct per_cpu *cpu_data)
> if (cpu_data->shutdown_state == SHUTDOWN_NONE) {
> if (num_cells == 1) {
> printk("Shutting down hypervisor\n");
> + /* Shutdown iommu, pci, ioapic */
That is arch business. The generic code doesn't know what's happening there.
> arch_shutdown();
> state = SHUTDOWN_STARTED;
> } else {
> diff --git a/hypervisor/include/jailhouse/cell-config.h
> b/hypervisor/include/jailhouse/cell-config.h
> index a2baaea..9a9e041 100644
> --- a/hypervisor/include/jailhouse/cell-config.h
> +++ b/hypervisor/include/jailhouse/cell-config.h
> @@ -158,8 +158,15 @@ struct jailhouse_iommu {
>
> #define JAILHOUSE_SYSTEM_SIGNATURE "JAILSYST"
>
> +/**
> + * General descriptor of the system.
> + * Passed as parameter from user-level during the JAILHOUSE_ENABLE ioctl.
Not only. It's also used to compile the configs, and more. Leave out the
second line.
> + */
> struct jailhouse_system {
> + /* Signature to check validity of data structure passed by user-level */
Rather refer to the possible signatures. And please use doxygen format.
In that case, all fields require comments.
> char signature[8];
> +
> + /* Jailhouse's location in memory */
> struct jailhouse_memory hypervisor_memory;
> struct jailhouse_memory debug_console;
> union {
> diff --git a/hypervisor/include/jailhouse/header.h
> b/hypervisor/include/jailhouse/header.h
> index 4a4d1cb..0173b63 100644
> --- a/hypervisor/include/jailhouse/header.h
> +++ b/hypervisor/include/jailhouse/header.h
> @@ -24,15 +24,22 @@
> */
> typedef int (*jailhouse_entry)(unsigned int);
>
> -/** Hypervisor description. */
> +/**
> + * Hypervisor description.
> + * Located at the beginning of the hypervisor binary image which is loaded as
> + * firmware during the JAILHOUSE_ENABLE ioctl.
> + */
> struct jailhouse_header {
> - /** Signature "JAILHOUS".
> + /** Signature "JAILHOUS" used to check the validity of the firmware
> + * containing the hypervisor image.
. Used ...
Maybe rather "Used for basic validity check of the hypervisor image".
It's neither a checksum nor something even stronger.
> * @note Filled at build time. */
> char signature[8];
> - /** Size of hypervisor core, rounded up to page boundary.
> + /** Size of hypervisor core, starting with its
Maybe end the sentence after "core" - that will give the short
description in doxygen.
> + * header, ending after its bss section.
> + * Rounded up to page boundary.
> * @note Filled at build time. */
> unsigned long core_size;
> - /** Size of per-CPU data structure.
> + /** Size of the per-CPU data structure.
Being a non-native speaker, I cannot tell for sure if the "the" is
needed here.
> * @note Filled at build time. */
> unsigned long percpu_size;
> /** Entry point (arch_entry()).
> @@ -40,6 +47,8 @@ struct jailhouse_header {
> int (*entry)(unsigned int);
>
> /** Configured maximum logical CPU ID + 1.
> + * It defines the range of CPU ID that can be passed as argument
> + * to the initialization function.
"IDs" - but that those are passed to some functions is not the purpose
of this value. It is not related to those functions. In general, try to
describe the meanings without too many relations to code usage as that
can change, leaving those comments inconsistent behind (no one will scan
for them on updates).
> * @note Filled by Linux loader driver before entry. */
> unsigned int max_cpus;
> /** Number of online CPUs that will call the entry function.
> diff --git a/hypervisor/paging.c b/hypervisor/paging.c
> index 1f22887..c4563e2 100644
> --- a/hypervisor/paging.c
> +++ b/hypervisor/paging.c
> @@ -27,6 +27,14 @@ extern u8 __page_pool[];
> /**
> * Offset between virtual and physical hypervisor addresses.
> *
> + *
One blank line too much.
> + * Jailhouse operates in a physically continuous memory region reserved at
> boot
> + * time using the "memmap=" kernel parameter.
Not only memmap... Leave out the detail how it is reserved.
> + * When Jailhouse is enabled, the loader linearly maps this memory into the
> + * kernel's virtual address space. This variable keeps the offset from the
> + * memory region's base address to make converting from host virtual to
> physical
> + * address (and the reverse) trivial.
Not accurate anymore, see JAILHOUSE_BORROW_ROOT_PT: All archs have a
fixed virt-phys offset (and that is the key information here), but some
(currently ARM64) only establish that themselves. May also change again
in the future as I'm very unhappy with the current approach.
> + *
> * @note Private, use page_map_hvirt2phys() or page_map_phys2hvirt() instead.
> */
> unsigned long page_offset;
> diff --git a/hypervisor/setup.c b/hypervisor/setup.c
> index b76f845..5e2e46f 100644
> --- a/hypervisor/setup.c
> +++ b/hypervisor/setup.c
> @@ -47,6 +47,7 @@ static void init_early(unsigned int cpu_id)
> JAILHOUSE_VERSION, cpu_id);
> printk("Code location: %p\n", __text_start);
>
> + /* Set-up paging: */
Obviously.
> error = paging_init();
> if (error)
> return;
> @@ -57,6 +58,7 @@ static void init_early(unsigned int cpu_id)
> if (error)
> return;
>
> + /* Initialize APIC and create Jailhouse's Interrupt Descriptor Table
> (IDT)*/
Same as arch_shutdown.
> error = arch_init_early();
> if (error)
> return;
> @@ -84,6 +86,9 @@ static void init_early(unsigned int cpu_id)
> printk("Initializing processors:\n");
> }
>
> +/**
> + * CPU initialization.
Obvious.
> + */
> static void cpu_init(struct per_cpu *cpu_data)
> {
> int err = -EINVAL;
> @@ -152,6 +157,9 @@ static void init_late(void)
> paging_dump_stats("after late setup");
> }
>
> +/**
> + * This is the function that actually enables Jailhouse.
That is not really a helpful comment. Rather, this is the entry point,
called by the Linux driver on each CPU when initializing Jailhouse.
> + */
> int entry(unsigned int cpu_id, struct per_cpu *cpu_data)
> {
> static volatile bool activate;
> @@ -162,6 +170,11 @@ int entry(unsigned int cpu_id, struct per_cpu *cpu_data)
> spin_lock(&init_lock);
>
> if (master_cpu_id == -1) {
> + /*
> + * It behaves slightly differently for the first CPU ("master")
> + * which is responsible for system-wide initialization
> + * (e.g. paging, apic, etc).
What about this instead: "Only the master CPU, the first to enter this
function, performs system-wide initializations."
> + */
> master = true;
> init_early(cpu_id);
> }
>
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
