diff mbox series

[Xen-devel,v2,2/4] xen/console: Rework HYPERCALL_console_io interface

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

Commit Message

Julien Grall Aug. 5, 2019, 1:29 p.m. UTC
At the moment, HYPERCALL_console_io is using signed int to describe the
command (@cmd) and the size of the buffer (@count).
    * @cmd does not need to be signed this used as a set of named value.
    None of them are negative. If new one are introduced they can be
    positive.
    * @count is used to know the size of the buffer. It makes little
    sense to have a negative value here.

So both variables are now switched to use unsigned int.

Changing @count to unsigned type will result in a change of behavior for
the existing commands:
    - write: Any buffer bigger than 2GB will now be printed rather than
      been ignored (the command return 0).
    - read: The return value is a signed 32-bit value for 32-bit Xen.
      To keep compatibility between 32-bit and 64-bit ABI, it
      effectively means the return value is 32-bit (despite been long
      on 64-bit). Negative value are used for error and positive value
      for the number of characters read. To avoid clash between the two
      sets, the buffer is still limited to 2GB. The only difference is
      an error is returned rather than claiming there are no characters.

The behavior is only affecting unlikely use of the current interface, so
this is not a big concern regarding backward compatibility.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    This will be documented in the public interface in the next patch.

    Changes in v2:
        - Patch added
---
 xen/drivers/char/console.c  | 15 +++++++++++++--
 xen/include/xen/hypercall.h |  4 ++--
 2 files changed, 15 insertions(+), 4 deletions(-)

Comments

Jan Beulich Aug. 8, 2019, 1:57 p.m. UTC | #1
On 05.08.2019 15:29, Julien Grall wrote:
> @@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)

>           rc = guest_console_write(buffer, count);

>           break;

>       case CONSOLEIO_read:

> +        /*

> +         * The return value is either the number of characters read or

> +         * a negative value in case of error. So we need to prevent

> +         * overlap between the two sets.

> +         */

> +        rc = -E2BIG;

> +        if ( (int)count < 0 )

> +            break;


A more portable (afaict) approach would be to check against INT_MAX.
Either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>


Jan
Julien Grall Aug. 8, 2019, 2:03 p.m. UTC | #2
On 08/08/2019 14:57, Jan Beulich wrote:
> On 05.08.2019 15:29, Julien Grall wrote:
>> @@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
>>            rc = guest_console_write(buffer, count);
>>            break;
>>        case CONSOLEIO_read:
>> +        /*
>> +         * The return value is either the number of characters read or
>> +         * a negative value in case of error. So we need to prevent
>> +         * overlap between the two sets.
>> +         */
>> +        rc = -E2BIG;
>> +        if ( (int)count < 0 )
>> +            break;
> 
> A more portable (afaict) approach would be to check against INT_MAX.

It would be better than the cast. I will update it.

> Either way
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you!

Cheers,
diff mbox series

Patch

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 752a11f6c5..fa8f5cff4b 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -524,7 +524,8 @@  static inline void xen_console_write_debug_port(const char *buf, size_t len)
 }
 #endif
 
-static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
+static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
+                                unsigned int count)
 {
     char kbuf[128];
     unsigned int kcount = 0;
@@ -612,7 +613,8 @@  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count)
     return 0;
 }
 
-long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
+long do_console_io(unsigned int cmd, unsigned int count,
+                   XEN_GUEST_HANDLE_PARAM(char) buffer)
 {
     long rc;
     unsigned int idx, len;
@@ -627,6 +629,15 @@  long do_console_io(int cmd, int count, XEN_GUEST_HANDLE_PARAM(char) buffer)
         rc = guest_console_write(buffer, count);
         break;
     case CONSOLEIO_read:
+        /*
+         * The return value is either the number of characters read or
+         * a negative value in case of error. So we need to prevent
+         * overlap between the two sets.
+         */
+        rc = -E2BIG;
+        if ( (int)count < 0 )
+            break;
+
         rc = 0;
         while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
         {
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index fc00a67448..ad8ad27b23 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -83,8 +83,8 @@  do_xen_version(
 
 extern long
 do_console_io(
-    int cmd,
-    int count,
+    unsigned int cmd,
+    unsigned int count,
     XEN_GUEST_HANDLE_PARAM(char) buffer);
 
 extern long