Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 2 Apr 2019, Julien Grall wrote:
> Currently, OS developpers will have to look at Xen code in order to know
> the parameters of an hypercall and how it is meant to work.
> 
> This is not a trivial task as you may need to have a deep understanding
> of Xen internal.
> 
> This patch attempts to document the behavior of HYPERCALL_console_io() to
> help OS developer.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

I love this, thank you!


> ---
> 
> This is a first attempt to address the lack on documentation for
> hypercalls. We may want to decide a format to use in every hypercall so
> it can be readable for the OS developer and easily consummed by
> documentation tools.
> ---
>  xen/include/public/xen.h | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index ccdffc0ad1..7c119c6782 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>  #define __HYPERVISOR_set_timer_op         15
>  #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */
>  #define __HYPERVISOR_xen_version          17
> +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); 
> */

I agree with Jan: I wouldn't put this comment here. Maybe down below
with the other comment. The reason is that I think it would be best not
to break the simple list of hypercall numbers.


>  #define __HYPERVISOR_console_io           18
>  #define __HYPERVISOR_physdev_op_compat    19 /* compat since 0x00030202 */
>  #define __HYPERVISOR_grant_table_op       20
> @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>  /* ` } */
>  
>  /*
> - * Commands to HYPERVISOR_console_io().
> + * Commands to HYPERVISOR_console_io()
> + *
> + * @cmd: Command (see below)
> + * @count: Size of the buffer to read/write
> + * @buffer: Pointer in the guest memory
> + *
> + * List of commands:
> + *
> + *  * CONSOLEIO_write: Write the buffer on Xen console.
> + *      For the hardware domain, all the characters in the buffer will
> + *      be written. Characters will be printed to directly to the
> + *      console.
> + *      For all the other domains, only the printable characters will be
> + *      written. Characters may be buffered until a newline (i.e '\n') is
> + *      found.
> + *      Return 0 on success, otherwise return an error code.
> + *  * CONSOLE_read: Attempts to read up @count characters from Xen console.
> + *      Return the number of character read on success, otherwise return
> + *      an error code.

This is great! My only suggestion for improvement would be to use a
special annotation for the return value. I think it would make it easier
to spot. Maybe something like @return:

 * CONSOLEIO_write: Write the buffer on Xen console.
     For the hardware domain, all the characters in the buffer will
     be written. Characters will be printed to directly to the
     console.
     For all the other domains, only the printable characters will be
     written. Characters may be buffered until a newline (i.e '\n') is
     found.
     @return: 0 on success, otherwise return an error code.
 * CONSOLE_read: Attempts to read up @count characters from Xen console.
     @return: the number of character read on success, otherwise return
              an error code.


>  #define CONSOLEIO_write         0
>  #define CONSOLEIO_read          1
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel