diff mbox series

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

Message ID 20190402164238.1815-4-julien.grall@arm.com
State New
Headers show
Series xen/console: Bug fixes and doc improvement | expand

Commit Message

Julien Grall April 2, 2019, 4:42 p.m. UTC
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@arm.com>

---

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(-)

Comments

Wei Liu April 3, 2019, 11:41 a.m. UTC | #1
On Tue, Apr 02, 2019 at 05:42:37PM +0100, 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@arm.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>
Jan Beulich April 3, 2019, 1:04 p.m. UTC | #2
>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote:
> --- 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); */
>  #define __HYPERVISOR_console_io           18

I pretty strongly object to this comment going where you put it.
I'd be fine if you put it ...

> @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>  /* ` } */
>  
>  /*
> - * Commands to HYPERVISOR_console_io().
> + * Commands to HYPERVISOR_console_io()

... into here, just like done for HYPERVISOR_mmuext_op() and
HYPERVISOR_update_va_mapping*() immediately above. This
would then also help ...

> + * @cmd: Command (see below)
> + * @count: Size of the buffer to read/write
> + * @buffer: Pointer in the guest memory

... associating these names.

Also please note the quotation used by the mentioned
existing doc comments, as well as a few other formal aspects
(like them also making clear what the return type is). I think
that's a model used elsewhere as well, so I think you should
follow it here.

The other thing is: As mentioned elsewhere, I don't think the
first two parameters should be plain int. I'm not happy to see
this proliferate into documentation as well, i.e. I'd prefer if
this was corrected before adding documentation. Would you
be willing to do this, or should I add it to my todo list?

Jan
Julien Grall April 9, 2019, 11:26 a.m. UTC | #3
Hi Jan,

On 03/04/2019 14:04, Jan Beulich wrote:
>>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote:
>> --- 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); */
>>   #define __HYPERVISOR_console_io           18
> 
> I pretty strongly object to this comment going where you put it.
> I'd be fine if you put it ...
> 
>> @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>   /* ` } */
>>   
>>   /*
>> - * Commands to HYPERVISOR_console_io().
>> + * Commands to HYPERVISOR_console_io()
> 
> ... into here, just like done for HYPERVISOR_mmuext_op() and
> HYPERVISOR_update_va_mapping*() immediately above. This
> would then also help ...
> 
>> + * @cmd: Command (see below)
>> + * @count: Size of the buffer to read/write
>> + * @buffer: Pointer in the guest memory
> 
> ... associating these names.

Ok.

> 
> Also please note the quotation used by the mentioned
> existing doc comments, as well as a few other formal aspects
> (like them also making clear what the return type is). I think
> that's a model used elsewhere as well, so I think you should
> follow it here.

I haven't replicated the ` because I have no idea what they are used for. I 
would appreciate if you provide pointer how to use them.

> 
> The other thing is: As mentioned elsewhere, I don't think the
> first two parameters should be plain int. I'm not happy to see
> this proliferate into documentation as well, i.e. I'd prefer if
> this was corrected before adding documentation. Would you
> be willing to do this, or should I add it to my todo list?

While switching from cmd from signed to unsigned should be ok. This would 
introduce a different behavior of for count.

Are we happy with that, or shall we add a check ((int)count) > 0?

In any case, I am happy to have a look at it.

Cheers,
Jan Beulich April 9, 2019, 11:42 a.m. UTC | #4
>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
> On 03/04/2019 14:04, Jan Beulich wrote:
>> Also please note the quotation used by the mentioned
>> existing doc comments, as well as a few other formal aspects
>> (like them also making clear what the return type is). I think
>> that's a model used elsewhere as well, so I think you should
>> follow it here.
> 
> I haven't replicated the ` because I have no idea what they are used for. I 
> would appreciate if you provide pointer how to use them.

Well, I can only point you at the history of things, e.g.
262e118a37 "docs/html/: Annotations for two hypercalls".

>> The other thing is: As mentioned elsewhere, I don't think the
>> first two parameters should be plain int. I'm not happy to see
>> this proliferate into documentation as well, i.e. I'd prefer if
>> this was corrected before adding documentation. Would you
>> be willing to do this, or should I add it to my todo list?
> 
> While switching from cmd from signed to unsigned should be ok. This would 
> introduce a different behavior of for count.

Since this removes an error condition, I think this is an okay change
to happen, without ...

> Are we happy with that, or shall we add a check ((int)count) > 0?

... any such extra check. This also isn't going to introduce any new
real risk of a long running operation or some such - if 2Gb of input
data are fine, I can't see why 4Gb wouldn't be, too.

Jan
Julien Grall April 16, 2019, 9:54 a.m. UTC | #5
Hi Jan,

On 4/9/19 12:42 PM, Jan Beulich wrote:
>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
>> On 03/04/2019 14:04, Jan Beulich wrote:
>>> Also please note the quotation used by the mentioned
>>> existing doc comments, as well as a few other formal aspects
>>> (like them also making clear what the return type is). I think
>>> that's a model used elsewhere as well, so I think you should
>>> follow it here.
>>
>> I haven't replicated the ` because I have no idea what they are used for. I
>> would appreciate if you provide pointer how to use them.
> 
> Well, I can only point you at the history of things, e.g.
> 262e118a37 "docs/html/: Annotations for two hypercalls".

Thank you for the pointer. However, we don't seem to use this formatting 
everywhere. For instance, the formatting used in my patch seems to be 
consistent with xen/include/public/vcpu.h.

Furthermore, the description of the hypercall is done on top of the 
definition of the hypercall number. Before I rework this patch, could we 
agree on what formatting we want?

> 
>>> The other thing is: As mentioned elsewhere, I don't think the
>>> first two parameters should be plain int. I'm not happy to see
>>> this proliferate into documentation as well, i.e. I'd prefer if
>>> this was corrected before adding documentation. Would you
>>> be willing to do this, or should I add it to my todo list?
>>
>> While switching from cmd from signed to unsigned should be ok. This would
>> introduce a different behavior of for count.
> 
> Since this removes an error condition, I think this is an okay change
> to happen, without ...
> 
>> Are we happy with that, or shall we add a check ((int)count) > 0?
> 
> ... any such extra check. This also isn't going to introduce any new
> real risk of a long running operation or some such - if 2Gb of input
> data are fine, I can't see why 4Gb wouldn't be, too.

Fair point. I will update the prototype accordingly.

Cheers,
Ian Jackson April 16, 2019, 10:29 a.m. UTC | #6
Jan Beulich writes ("Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io()"):
> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
> > I haven't replicated the ` because I have no idea what they are used for. I 
> > would appreciate if you provide pointer how to use them.
> 
> Well, I can only point you at the history of things, e.g.
> 262e118a37 "docs/html/: Annotations for two hypercalls".

That is a reasonable example, but:

There is in fact some documentation for the ` syntax.
See docs/xen-headers, which is the script which scans the .h
files and makes the online hypercall documentation.  The syntax
doc is rather terse, I'm afraid, but it's only a 400-line
script...

I wrote that script because I wanted some kind of browseable
documentation, and I didn't want to totally overhaul the headers.

Ian.
Stefano Stabellini April 16, 2019, 8:42 p.m. UTC | #7
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@arm.com>

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
>
Jan Beulich April 25, 2019, 10:09 a.m. UTC | #8
>>> On 16.04.19 at 11:54, <julien.grall@arm.com> wrote:
> On 4/9/19 12:42 PM, Jan Beulich wrote:
>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
>>> On 03/04/2019 14:04, Jan Beulich wrote:
>>>> Also please note the quotation used by the mentioned
>>>> existing doc comments, as well as a few other formal aspects
>>>> (like them also making clear what the return type is). I think
>>>> that's a model used elsewhere as well, so I think you should
>>>> follow it here.
>>>
>>> I haven't replicated the ` because I have no idea what they are used for. I
>>> would appreciate if you provide pointer how to use them.
>> 
>> Well, I can only point you at the history of things, e.g.
>> 262e118a37 "docs/html/: Annotations for two hypercalls".
> 
> Thank you for the pointer. However, we don't seem to use this formatting 
> everywhere. For instance, the formatting used in my patch seems to be 
> consistent with xen/include/public/vcpu.h.
> 
> Furthermore, the description of the hypercall is done on top of the 
> definition of the hypercall number. Before I rework this patch, could we 
> agree on what formatting we want?

Well, FAOD (see Ian's earlier reply) the style to be used is as suggested,
and eventually vcpu.h should be switched too.

Jan
Julien Grall Aug. 5, 2019, 9:40 a.m. UTC | #9
Hi,

On 09/04/2019 12:42, Jan Beulich wrote:
>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
>> On 03/04/2019 14:04, Jan Beulich wrote:
>>> Also please note the quotation used by the mentioned
>>> existing doc comments, as well as a few other formal aspects
>>> (like them also making clear what the return type is). I think
>>> that's a model used elsewhere as well, so I think you should
>>> follow it here.
>>
>> I haven't replicated the ` because I have no idea what they are used for. I
>> would appreciate if you provide pointer how to use them.
> 
> Well, I can only point you at the history of things, e.g.
> 262e118a37 "docs/html/: Annotations for two hypercalls".
> 
>>> The other thing is: As mentioned elsewhere, I don't think the
>>> first two parameters should be plain int. I'm not happy to see
>>> this proliferate into documentation as well, i.e. I'd prefer if
>>> this was corrected before adding documentation. Would you
>>> be willing to do this, or should I add it to my todo list?
>>
>> While switching from cmd from signed to unsigned should be ok. This would
>> introduce a different behavior of for count.
> 
> Since this removes an error condition, I think this is an okay change
> to happen, without ...
> 
>> Are we happy with that, or shall we add a check ((int)count) > 0?
> 
> ... any such extra check. This also isn't going to introduce any new
> real risk of a long running operation or some such - if 2Gb of input
> data are fine, I can't see why 4Gb wouldn't be, too.

Actually, it will not be fine at least for the read case. The return value is a 
32-bit value (on AArch32 and if you want to keep compat between 64-bit and 
32-bit). A negative value indicates an error. As we return the number of 
characters read, it means we can only handle 2GB.

So I would rather limit the buffer to 2GB for everyone.

Cheers,
Jan Beulich Aug. 5, 2019, 10:07 a.m. UTC | #10
On 05.08.2019 11:40, Julien Grall wrote:
> Hi,

> 

> On 09/04/2019 12:42, Jan Beulich wrote:

>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:

>>> On 03/04/2019 14:04, Jan Beulich wrote:

>>>> Also please note the quotation used by the mentioned

>>>> existing doc comments, as well as a few other formal aspects

>>>> (like them also making clear what the return type is). I think

>>>> that's a model used elsewhere as well, so I think you should

>>>> follow it here.

>>>

>>> I haven't replicated the ` because I have no idea what they are used for. I

>>> would appreciate if you provide pointer how to use them.

>>

>> Well, I can only point you at the history of things, e.g.

>> 262e118a37 "docs/html/: Annotations for two hypercalls".

>>

>>>> The other thing is: As mentioned elsewhere, I don't think the

>>>> first two parameters should be plain int. I'm not happy to see

>>>> this proliferate into documentation as well, i.e. I'd prefer if

>>>> this was corrected before adding documentation. Would you

>>>> be willing to do this, or should I add it to my todo list?

>>>

>>> While switching from cmd from signed to unsigned should be ok. This would

>>> introduce a different behavior of for count.

>>

>> Since this removes an error condition, I think this is an okay change

>> to happen, without ...

>>

>>> Are we happy with that, or shall we add a check ((int)count) > 0?

>>

>> ... any such extra check. This also isn't going to introduce any new

>> real risk of a long running operation or some such - if 2Gb of input

>> data are fine, I can't see why 4Gb wouldn't be, too.

> 

> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB.


Hmm, valid point.

> So I would rather limit the buffer to 2GB for everyone.


Probably fair enough then (if explicitly stated). Personally I would
nevertheless allow sizes up to 4Gb-4kb, but I can see why this may
not be liked by everyone. I'd like to point out though that the
PTR_ERR() machinery we've inherited from Linux utilizes exactly that.

Jan
Julien Grall Aug. 5, 2019, 10:17 a.m. UTC | #11
Hi Jan,

On 05/08/2019 11:07, Jan Beulich wrote:
> On 05.08.2019 11:40, Julien Grall wrote:
>> Hi,
>>
>> On 09/04/2019 12:42, Jan Beulich wrote:
>>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:
>>>> On 03/04/2019 14:04, Jan Beulich wrote:
>>>>> Also please note the quotation used by the mentioned
>>>>> existing doc comments, as well as a few other formal aspects
>>>>> (like them also making clear what the return type is). I think
>>>>> that's a model used elsewhere as well, so I think you should
>>>>> follow it here.
>>>>
>>>> I haven't replicated the ` because I have no idea what they are used for. I
>>>> would appreciate if you provide pointer how to use them.
>>>
>>> Well, I can only point you at the history of things, e.g.
>>> 262e118a37 "docs/html/: Annotations for two hypercalls".
>>>
>>>>> The other thing is: As mentioned elsewhere, I don't think the
>>>>> first two parameters should be plain int. I'm not happy to see
>>>>> this proliferate into documentation as well, i.e. I'd prefer if
>>>>> this was corrected before adding documentation. Would you
>>>>> be willing to do this, or should I add it to my todo list?
>>>>
>>>> While switching from cmd from signed to unsigned should be ok. This would
>>>> introduce a different behavior of for count.
>>>
>>> Since this removes an error condition, I think this is an okay change
>>> to happen, without ...
>>>
>>>> Are we happy with that, or shall we add a check ((int)count) > 0?
>>>
>>> ... any such extra check. This also isn't going to introduce any new
>>> real risk of a long running operation or some such - if 2Gb of input
>>> data are fine, I can't see why 4Gb wouldn't be, too.
>>
>> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB.
> 
> Hmm, valid point.
> 
>> So I would rather limit the buffer to 2GB for everyone.
> 
> Probably fair enough then (if explicitly stated). Personally I would
> nevertheless allow sizes up to 4Gb-4kb, but I can see why this may
> not be liked by everyone. I'd like to point out though that the
> PTR_ERR() machinery we've inherited from Linux utilizes exactly that.

Well, that's a console buffer. The chance you have write/read more than 2GB in a 
row is very unlikely.

So it feels to me that exposing PTR_ERR(...) like interface is not worth it.

Cheers,
Jan Beulich Aug. 5, 2019, 10:21 a.m. UTC | #12
On 05.08.2019 12:17, Julien Grall wrote:
> Hi Jan,

> 

> On 05/08/2019 11:07, Jan Beulich wrote:

>> On 05.08.2019 11:40, Julien Grall wrote:

>>> Hi,

>>>

>>> On 09/04/2019 12:42, Jan Beulich wrote:

>>>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote:

>>>>> On 03/04/2019 14:04, Jan Beulich wrote:

>>>>>> Also please note the quotation used by the mentioned

>>>>>> existing doc comments, as well as a few other formal aspects

>>>>>> (like them also making clear what the return type is). I think

>>>>>> that's a model used elsewhere as well, so I think you should

>>>>>> follow it here.

>>>>>

>>>>> I haven't replicated the ` because I have no idea what they are used for. I

>>>>> would appreciate if you provide pointer how to use them.

>>>>

>>>> Well, I can only point you at the history of things, e.g.

>>>> 262e118a37 "docs/html/: Annotations for two hypercalls".

>>>>

>>>>>> The other thing is: As mentioned elsewhere, I don't think the

>>>>>> first two parameters should be plain int. I'm not happy to see

>>>>>> this proliferate into documentation as well, i.e. I'd prefer if

>>>>>> this was corrected before adding documentation. Would you

>>>>>> be willing to do this, or should I add it to my todo list?

>>>>>

>>>>> While switching from cmd from signed to unsigned should be ok. This would

>>>>> introduce a different behavior of for count.

>>>>

>>>> Since this removes an error condition, I think this is an okay change

>>>> to happen, without ...

>>>>

>>>>> Are we happy with that, or shall we add a check ((int)count) > 0?

>>>>

>>>> ... any such extra check. This also isn't going to introduce any new

>>>> real risk of a long running operation or some such - if 2Gb of input

>>>> data are fine, I can't see why 4Gb wouldn't be, too.

>>>

>>> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB.

>>

>> Hmm, valid point.

>>

>>> So I would rather limit the buffer to 2GB for everyone.

>>

>> Probably fair enough then (if explicitly stated). Personally I would

>> nevertheless allow sizes up to 4Gb-4kb, but I can see why this may

>> not be liked by everyone. I'd like to point out though that the

>> PTR_ERR() machinery we've inherited from Linux utilizes exactly that.

> 

> Well, that's a console buffer. The chance you have write/read more than 2GB in a row is very unlikely.

> 

> So it feels to me that exposing PTR_ERR(...) like interface is not worth it.


Sure, hence me having said "personally I would ..." - I don't expect
you to go that route.

Jan
diff mbox series

Patch

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); */
 #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.
  */
 #define CONSOLEIO_write         0
 #define CONSOLEIO_read          1