| Message ID | 20190402164238.1815-4-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/console: Bug fixes and doc improvement | expand |
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>
>>> 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
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,
>>> 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
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,
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.
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 >
>>> 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
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,
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
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,
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 --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
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(-)