On 2016-10-05 14:11, Claudio Scordino wrote:
> This patch contains a few general comments in the source code of the
> driver subsystem.
>
> Signed-off-by: Claudio Scordino <clau...@evidence.eu.com>
> ---
> driver/cell.c | 3 +++
> driver/main.c | 26 +++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/driver/cell.c b/driver/cell.c
> index b8c4403..56280c5 100644
> --- a/driver/cell.c
> +++ b/driver/cell.c
> @@ -212,6 +212,7 @@ int jailhouse_cmd_cell_create(struct
> jailhouse_cell_create __user *arg)
> goto error_cell_delete;
> }
>
> + /* Off-line each CPU assigned to the new cell */
The loop also removes the CPU from the root cell set. So either extend
the comment or move it inside the loop, at the block that does offlining.
> for_each_cpu(cpu, &cell->cpus_assigned) {
> if (cpu_online(cpu)) {
> err = cpu_down(cpu);
> @@ -225,6 +226,8 @@ int jailhouse_cmd_cell_create(struct
> jailhouse_cell_create __user *arg)
> jailhouse_pci_do_all_devices(cell, JAILHOUSE_PCI_TYPE_DEVICE,
> JAILHOUSE_PCI_ACTION_CLAIM);
>
> + /* This hypercall will eventually dispatch the call to the cell_create()
> + * function defined in hypervisor/control.c */
This information does not really help to understand the driver code.
Also, the driver should not care: we have a (hopefully) well-defined
hypercall interface.
> err = jailhouse_call_arg1(JAILHOUSE_HC_CELL_CREATE, __pa(config));
> if (err < 0)
> goto error_cpu_online;
> diff --git a/driver/main.c b/driver/main.c
> index 746de07..63d7464 100644
> --- a/driver/main.c
> +++ b/driver/main.c
> @@ -138,6 +138,12 @@ void *jailhouse_ioremap(phys_addr_t phys, unsigned long
> virt,
> return vma->addr;
> }
>
> +/**
If using special comment marks, the comment should be formatted
accordingly. But this is just an internal function, so the comment can
stay local ("/*").
> + * Called for each cpu by the JAILHOUSE_ENABLE ioctl.
> + * It is a thin wrapper that jumps to the entry point set in the header.
Actually more than a wrapper. It calls the entry point, yes, reports the
result and it signals completion to the main thread that invoked this.
> + * The entry point is defined in hypervisor/setup.c as arch_entry, which is
> + * coded in assembler and resides in entry.S.
Don't dig into the hypervisor here. Things may change on the other side,
and then such comments only cause confusions.
> + */
> static void enter_hypervisor(void *info)
> {
> struct jailhouse_header *header = info;
> @@ -164,6 +170,11 @@ static void enter_hypervisor(void *info)
> atomic_inc(&call_done);
> }
>
> +/**
> + * Function returning the name of the firmware to be loaded.
Would "jailhouse_get_fw_name" make the purpose of this functions clearer
(and obsolet the comment)?
> + *
> + * The firmware name changes based on the arch (e.g. AMD, Intel, ARM).
Well, that's what the code tells us already.
> + */
> static inline const char * jailhouse_fw_name(void)
> {
> #ifdef CONFIG_X86
> @@ -177,6 +188,7 @@ static inline const char * jailhouse_fw_name(void)
> #endif
> }
>
> +/* @See Documentation/bootstrap-interface.txt */
Kernel-doc is used for the driver, and I don't think it can process the
@see tag. Plaintext reference is fine.
> static int jailhouse_cmd_enable(struct jailhouse_system __user *arg)
> {
> const struct firmware *hypervisor;
> @@ -197,7 +209,7 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
> return -ENODEV;
> }
> #endif
> -
Unrelated whitespace removal.
> + /* Get the name of the firmware to be loaded: */
That should be clear(er) from the name of the invoked function.
> fw_name = jailhouse_fw_name();
> if (!fw_name) {
> pr_err("jailhouse: Missing or unsupported HVM technology\n");
> @@ -228,6 +240,7 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
> if (jailhouse_enabled || !try_module_get(THIS_MODULE))
> goto error_unlock;
>
> + /* Load hypervisor image */
> err = request_firmware(&hypervisor, fw_name, jailhouse_dev);
> if (err) {
> pr_err("jailhouse: Missing hypervisor image %s\n", fw_name);
> @@ -252,6 +265,8 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
> #ifdef JAILHOUSE_BORROW_ROOT_PT
> remap_addr = JAILHOUSE_BASE;
> #endif
> + /* Map physical memory region reserved for Jailhouse at kernel's virtual
> + * address JAILHOUSE_BASE. */
Right now, only #ifdef JAILHOUSE_BORROW_ROOT_PT. Otherwise, remap_addr
is 0, and ioremap will chose the virtual address.
That's an ARM64 topic and I'm not yet completely sure we can't do it
differently. But we will for a while for sure because I will merge the
current ARM64 logic as-is.
> hypervisor_mem = jailhouse_ioremap(hv_mem->phys_start, remap_addr,
> hv_mem->size);
> if (!hypervisor_mem) {
> @@ -260,11 +275,14 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
> goto error_release_fw;
> }
>
> + /* Load hypervisor's binary image at beginning of the memory region */
You already "loaded" above. This is more a copying or moving, I would say.
> memcpy(hypervisor_mem, hypervisor->data, hypervisor->size);
> memset(hypervisor_mem + hypervisor->size, 0,
> hv_mem->size - hypervisor->size);
>
> header = (struct jailhouse_header *)hypervisor_mem;
> + /* Set number of max CPUs in hypervisor header according to
> + * system state */
/* Set i to 0 */ ;)
> header->max_cpus = max_cpus;
>
> /*
> @@ -276,6 +294,8 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
> flush_icache_range((unsigned long)hypervisor_mem,
> (unsigned long)(hypervisor_mem + header->core_size));
>
> + /* Copy system configuration to its target address in hypervisor memory
> + * region: */
> config = (struct jailhouse_system *)
> (hypervisor_mem + hv_core_and_percpu_size);
> if (copy_from_user(config, arg, config_size)) {
> @@ -311,8 +331,11 @@ static int jailhouse_cmd_enable(struct jailhouse_system
> __user *arg)
>
> preempt_disable();
>
> + /* Set number of online CPUs in hypervisor header according to
> + * system state */
Another one of "set i to 0".
> header->online_cpus = num_online_cpus();
>
> + /* Call enter_hypervisor() on each CPU. */
If you want to tell something maybe not that obvious (on_each_cpu is
fairly self-describing), rather point out the barrier used here (call_done).
> atomic_set(&call_done, 0);
> on_each_cpu(enter_hypervisor, header, 0);
> while (atomic_read(&call_done) != num_online_cpus())
> @@ -436,6 +459,7 @@ static int jailhouse_cmd_disable(void)
> if (err)
> goto unlock_out;
>
> + /* Unmap jailhouse's reserved memory in kernel space */
It's not the whole reserved memory, just the hypervisor memory. But
rather than describing the what (people can read that), mention the
"why" (no longer required, even no longer usable).
> vunmap(hypervisor_mem);
>
> jailhouse_cell_delete_root();
>
Thanks for the comment. I think they can help improving the
understanding on both sides: What the code is doing or supposed to do as
well as where it would benefit from more worlds of explanation.
Will look at the other patches the other days. This may already help you
understanding what kind of comments I would welcome and which I would
like to avoid. Specifically, I prefer self-explaining code more than
explanation fix-ups in comments. So don't hesitate to suggests better
names as well!
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 jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
