[PATCH XTF v4] Implement pv_read_some

[PATCH XTF v4] Implement pv_read_some

[Felix Schmoll]

Implement reading from PV console. Making use of polling.

Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com>

---
Changed since v3:
 * Add to comment: read function blocks if no data available
---
 common/console.c      | 26 ++++++++++++++++++++++++++
 include/xtf/console.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/common/console.c b/common/console.c
index 7cb2361..b1a35f4 100644
--- a/common/console.c
+++ b/common/console.c
@@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len)
 }
 
 /*
+ * Read out data from the pv ring, either until buffer is filled or no
+ * more data are available. Might result in partial strings, depending
+ * on how xenconsoled passes in data.
+ *
+ * Will block if no data are available.
+ */
+size_t pv_console_read_some(char *buf, size_t len)
+{
+    size_t s = 0;
+    uint32_t cons, prod;
+
+    while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) ||
+            (pv_ring->in_cons == pv_ring->in_prod) )
+        hypercall_poll(pv_evtchn);
+
+    cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod);
+
+    while ( (s < len) && (0 < (prod - cons)) )
+        buf[s++] = pv_ring->in[cons++ & (sizeof(pv_ring->in) - 1)];
+
+    STORE_RELEASE(&pv_ring->in_cons, cons);
+
+    return s;
+}
+
+/*
  * Write some data into the pv ring, synchronously waiting for all data to be
  * consumed.
  */
diff --git a/include/xtf/console.h b/include/xtf/console.h
index 2a93c06..caec790 100644
--- a/include/xtf/console.h
+++ b/include/xtf/console.h
@@ -25,6 +25,8 @@ void init_pv_console(xencons_interface_t *ring,
 void vprintk(const char *fmt, va_list args) __printf(1, 0);
 void printk(const char *fmt, ...) __printf(1, 2);
 
+size_t pv_console_read_some(char *buf, size_t len);
+
 #endif /* XTF_CONSOLE_H */
 
 /*
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v4] Implement pv_read_some

[Wei Liu]

On Mon, Jul 24, 2017 at 08:24:15AM +0200, Felix Schmoll wrote:
> Implement reading from PV console. Making use of polling.
> 
> Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com>
> 

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v4] Implement pv_read_some

[Wei Liu]

On Mon, Jul 24, 2017 at 08:24:15AM +0200, Felix Schmoll wrote:
> Implement reading from PV console. Making use of polling.
> 
> Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com>
> 
> ---
> Changed since v3:
>  * Add to comment: read function blocks if no data available
> ---
>  common/console.c      | 26 ++++++++++++++++++++++++++
>  include/xtf/console.h |  2 ++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/common/console.c b/common/console.c
> index 7cb2361..b1a35f4 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len)
>  }
>  
>  /*
> + * Read out data from the pv ring, either until buffer is filled or no
> + * more data are available. Might result in partial strings, depending
> + * on how xenconsoled passes in data.
> + *
> + * Will block if no data are available.
> + */
> +size_t pv_console_read_some(char *buf, size_t len)
> +{
> +    size_t s = 0;
> +    uint32_t cons, prod;
> +
> +    while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) ||
> +            (pv_ring->in_cons == pv_ring->in_prod) )
> +        hypercall_poll(pv_evtchn);
> +
> +    cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod);

Andrew pointed out on IRC this will degrade to

    cons = prod = LOAD_ACQUIRE(...);

I failed to spot this earlier.

See

http://en.cppreference.com/w/c/language/operator_precedence

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v4] Implement pv_read_some

[Andrew Cooper]

On 24/07/17 07:24, Felix Schmoll wrote:
> Implement reading from PV console. Making use of polling.
>
> Signed-off-by: Felix Schmoll <eggi.innovations@gmail.com>
>
> ---
> Changed since v3:
>  * Add to comment: read function blocks if no data available
> ---
>  common/console.c      | 26 ++++++++++++++++++++++++++
>  include/xtf/console.h |  2 ++
>  2 files changed, 28 insertions(+)
>
> diff --git a/common/console.c b/common/console.c
> index 7cb2361..b1a35f4 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -46,6 +46,32 @@ static size_t pv_console_write_some(const char *buf, size_t len)
>  }
>  
>  /*
> + * Read out data from the pv ring, either until buffer is filled or no
> + * more data are available. Might result in partial strings, depending
> + * on how xenconsoled passes in data.
> + *
> + * Will block if no data are available.
> + */
> +size_t pv_console_read_some(char *buf, size_t len)
> +{
> +    size_t s = 0;
> +    uint32_t cons, prod;
> +
> +    while ( !test_and_clear_bit(pv_evtchn, shared_info.evtchn_pending) ||
> +            (pv_ring->in_cons == pv_ring->in_prod) )
> +        hypercall_poll(pv_evtchn);
> +
> +    cons = pv_ring->in_cons, prod = LOAD_ACQUIRE(&pv_ring->in_prod);

Given the confusion this has caused on IRC, I'd prefer that the code was
explicit.  Is the following ok?

cons = pv_ring->in_cons;
prod = LOAD_ACQUIRE(&pv_ring->in_prod);

If so, I can fix up on commit.

~Andrew

> +
> +    while ( (s < len) && (0 < (prod - cons)) )
> +        buf[s++] = pv_ring->in[cons++ & (sizeof(pv_ring->in) - 1)];
> +
> +    STORE_RELEASE(&pv_ring->in_cons, cons);
> +
> +    return s;
> +}
> +
> +/*
>   * Write some data into the pv ring, synchronously waiting for all data to be
>   * consumed.
>   */
> diff --git a/include/xtf/console.h b/include/xtf/console.h
> index 2a93c06..caec790 100644
> --- a/include/xtf/console.h
> +++ b/include/xtf/console.h
> @@ -25,6 +25,8 @@ void init_pv_console(xencons_interface_t *ring,
>  void vprintk(const char *fmt, va_list args) __printf(1, 0);
>  void printk(const char *fmt, ...) __printf(1, 2);
>  
> +size_t pv_console_read_some(char *buf, size_t len);
> +
>  #endif /* XTF_CONSOLE_H */
>  
>  /*


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v4] Implement pv_read_some

[Felix Schmoll]

[-- Attachment #1.1: Type: text/plain, Size: 723 bytes --]

2017-07-24 12:38 GMT+02:00 Andrew Cooper <andrew.cooper3@citrix.com>:
>
>
> Given the confusion this has caused on IRC, I'd prefer that the code was
> explicit.  Is the following ok?
>
> cons = pv_ring->in_cons;
> prod = LOAD_ACQUIRE(&pv_ring->in_prod);
>
> If so, I can fix up on commit.
>
> ~Andrew
>
>
I'm not really convinced by this: If this was really happening, then you
could never do multiple declarations in the same line because it would
always degrade.

According to for example Wikipedia,

/** *  Commas act as separators in this line, not as an operator. *
Results: a=1, b=2, c=3, i=0 */int a=1, b=2, c=3, i=0;

Also, read_some is obviously modeled after write_some, so you would have to
change that one too.

[-- Attachment #1.2: Type: text/html, Size: 2729 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH XTF v4] Implement pv_read_some

[Felix Schmoll]

[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]

2017-07-25 8:58 GMT+02:00 Felix Schmoll <eggi.innovations@gmail.com>:

> 2017-07-24 12:38 GMT+02:00 Andrew Cooper <andrew.cooper3@citrix.com>:
>>
>>
>> Given the confusion this has caused on IRC, I'd prefer that the code was
>> explicit.  Is the following ok?
>>
>> cons = pv_ring->in_cons;
>> prod = LOAD_ACQUIRE(&pv_ring->in_prod);
>>
>> If so, I can fix up on commit.
>>
>> ~Andrew
>>
>>
> I'm not really convinced by this: If this was really happening, then you
> could never do multiple declarations in the same line because it would
> always degrade.
>
> According to for example Wikipedia,
>
> /** *  Commas act as separators in this line, not as an operator. *  Results: a=1, b=2, c=3, i=0 */int a=1, b=2, c=3, i=0;
>
> Also, read_some is obviously modeled after write_some, so you would have
> to change that one too.
>

Nevermind, sure, go ahead.

[-- Attachment #1.2: Type: text/html, Size: 3724 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel