Re: [PATCH 1/3] Driver: general comments in the source code
On 2016-10-12 14:29, Claudio Scordino wrote: >>> @@ -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)? >> >> > Yes, I think it would be clearer. Can you please change the name ? > You can as well, in a separate patch as part of your next round :) 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.
Re: [PATCH 1/3] Driver: general comments in the source code
Hi Jan,
2016-10-05 17:28 GMT+02:00 Jan Kiszka :
> 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
> > ---
> > 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)?
>
>
Yes, I think it would be clearer. Can you please change the name ?
> > + *
> > + * 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 no
Re: [PATCH 1/3] Driver: general comments in the source code
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
> ---
> 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
