Hi Jan,

2016-10-05 17:28 GMT+02:00 Jan Kiszka <jan.kis...@siemens.com>:

> 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)?
>
>
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 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 */ ;)
>


Yeah, I had noted down the comments while reading the code, and I didn't
recall that some of them were really trivial. Sorry.

I'll remove the silly comments, make the changes you have proposed and send
them again on list.

Many thanks and best regards,

              Claudio

-- 
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.

Reply via email to