Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Jan Beulich 
>>> On 16.10.17 at 17:47,  wrote:
> On 16 October 2017 at 15:53, Jan Beulich  wrote:
> On 16.10.17 at 11:02,  wrote:
>>>  static int console_create_ring(struct console *con)
>>>  {
>>> - int err, remote_port, ring_ref, rc;
>>> + int err, remote_port, rc;
>>> + xen_pfn_t ring_ref;
>>>   char *type, path[PATH_MAX];
>>>   struct domain *dom = con->d;
>>>
>>>   err = xs_gather(xs, con->xspath,
>>> - "ring-ref", "%u", _ref,
>>> + "ring-ref", "%i", _ref,
>>
>> How would you gather a 64-bit value using %i without any length
>> modifier? With just %i you're even going to use partially initialized
>> data, so unless somewhere else the upper 32 bits got clipped off
>> again the console wouldn't work anymore.
>>
> I should use "%lli" here to read it as a 64-bit value for all
> architectures. Correct?

No, that's break for a 32-bit tool stack on x86. You really need
an abstraction similar to PRIu_xen_pfn, just that I'd rather not
see SCNi_xen_pfn added to the public headers.

Jan


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

Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Bhupinder Thakur 
On 16 October 2017 at 15:53, Jan Beulich  wrote:
 On 16.10.17 at 11:02,  wrote:
>>  static int console_create_ring(struct console *con)
>>  {
>> - int err, remote_port, ring_ref, rc;
>> + int err, remote_port, rc;
>> + xen_pfn_t ring_ref;
>>   char *type, path[PATH_MAX];
>>   struct domain *dom = con->d;
>>
>>   err = xs_gather(xs, con->xspath,
>> - "ring-ref", "%u", _ref,
>> + "ring-ref", "%i", _ref,
>
> How would you gather a 64-bit value using %i without any length
> modifier? With just %i you're even going to use partially initialized
> data, so unless somewhere else the upper 32 bits got clipped off
> again the console wouldn't work anymore.
>
I should use "%lli" here to read it as a 64-bit value for all
architectures. Correct?

Regards,
Bhupinder

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

Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Jan Beulich 
>>> On 16.10.17 at 12:18,  wrote:
> On 16/10/17 10:02, Bhupinder Thakur wrote:
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>> @@ -80,6 +80,7 @@ static unsigned int current_array_size;
>>   static unsigned int nr_fds;
>>   
>>   #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & 
>> ~((1UL<<(_w))-1))
>> +#define INVALID_RING_REF(~(xen_pfn_t)0)
> 
> I am a bit surprised we don't have an INVALID_XEN_PFN definition in the 
> public header. Jan would you have an objection to introduce that?

If this was for a use with a hypercall, I probably wouldn't mind. But
here this is a completely local constant, unrelated to anything Xen
produces or consumes. Of course seeing mention of INVALID_GFN in
a public header comment, there likely are hypercalls that could
benefit from such a constant (or maybe more than one, for the
various frame number flavors).

Jan


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

Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Jan Beulich 
>>> On 16.10.17 at 11:02,  wrote:
>  static int console_create_ring(struct console *con)
>  {
> - int err, remote_port, ring_ref, rc;
> + int err, remote_port, rc;
> + xen_pfn_t ring_ref;
>   char *type, path[PATH_MAX];
>   struct domain *dom = con->d;
>  
>   err = xs_gather(xs, con->xspath,
> - "ring-ref", "%u", _ref,
> + "ring-ref", "%i", _ref,

How would you gather a 64-bit value using %i without any length
modifier? With just %i you're even going to use partially initialized
data, so unless somewhere else the upper 32 bits got clipped off
again the console wouldn't work anymore.

Jan


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

Re: [Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Julien Grall 

Hi Bhupinder,

On 16/10/17 10:02, Bhupinder Thakur wrote:

In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:


flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));


However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.  Also corrected the type of ring_ref to xen_pfn_t in xenconsole.


The commit message does not give any reason why xc_map_foreign_page(...) 
was switch to xen_paddr_t.


But I think this patch is doing too much. You try to fix too distinct 
errors here:
	1) Fixing the bug introduced by commit 2b668a84e5 "libxl: use correct 
type modifier for vuart_gfn"

2) The fact that the other side will at best truncate the value

With that you also modify the xc_map_foreign_page() to avoid truncating 
the value. So you at least need to split this patch in 3 if not more.




Signed-off-by: Bhupinder Thakur 
---

This patch was verified for arm64 and arm32 toolstack compilation.

CC: Ian Jackson 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Jan Beulich 
CC: Andrew Cooper 

  tools/console/daemon/io.c| 20 +++-
  tools/libxc/include/xenctrl_compat.h |  2 +-
  tools/libxc/xc_foreign_memory.c  |  2 +-
  tools/libxl/libxl_console.c  |  4 ++--
  tools/libxl/libxl_dom.c  |  2 +-
  tools/libxl/libxl_internal.h |  2 +-
  xen/include/public/arch-arm.h|  1 +
  xen/include/public/arch-x86/xen.h|  1 +
  8 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index e22009a..6369bf4 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -80,6 +80,7 @@ static unsigned int current_array_size;
  static unsigned int nr_fds;
  
  #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))

+#define INVALID_RING_REF(~(xen_pfn_t)0)


I am a bit surprised we don't have an INVALID_XEN_PFN definition in the 
public header. Jan would you have an objection to introduce that?


  
  struct buffer {

char *data;
@@ -98,7 +99,7 @@ struct console {
struct buffer buffer;
char *xspath;
char *log_suffix;
-   int ring_ref;
+   xen_pfn_t ring_ref;
xenevtchn_handle *xce_handle;
int xce_pollfd_idx;
int event_count;
@@ -651,22 +652,23 @@ static void console_unmap_interface(struct console *con)
  {
if (con->interface == NULL)
return;
-   if (xgt_handle && con->ring_ref == -1)
+   if (xgt_handle && con->ring_ref == INVALID_RING_REF)
xengnttab_unmap(xgt_handle, con->interface, 1);
else
munmap(con->interface, XC_PAGE_SIZE);
con->interface = NULL;
-   con->ring_ref = -1;
+   con->ring_ref = INVALID_RING_REF;
  }
   
  static int console_create_ring(struct console *con)

  {
-   int err, remote_port, ring_ref, rc;
+   int err, remote_port, rc;
+   xen_pfn_t ring_ref;
char *type, path[PATH_MAX];
struct domain *dom = con->d;
  
  	err = xs_gather(xs, con->xspath,

-   "ring-ref", "%u", _ref,
+   "ring-ref", "%i", _ref,


Hmmm, I think %i is wrong here. You don't fix the problem that the GFN 
can be wider than 32-bit.



"port", "%i", _port,
NULL);
  
@@ -690,7 +692,7 @@ static int console_create_ring(struct console *con)

free(type);
  
  	/* If using ring_ref and it has changed, remap */

-   if (ring_ref != con->ring_ref && con->ring_ref != -1)
+   if (ring_ref != con->ring_ref && con->ring_ref != INVALID_RING_REF)
console_unmap_interface(con);
  
  	if (!con->interface && xgt_handle && con->use_gnttab) {

@@ -698,14 +700,14 @@ static int console_create_ring(struct console *con)
con->interface = xengnttab_map_grant_ref(xgt_handle,
dom->domid, GNTTAB_RESERVED_CONSOLE,
PROT_READ|PROT_WRITE);
-   con->ring_ref = -1;
+   con->ring_ref = INVALID_RING_REF;
}
if (!con->interface) {
/* Fall back to xc_map_foreign_range */
con->interface = xc_map_foreign_range(
xc, dom->domid, XC_PAGE_SIZE,
PROT_READ|PROT_WRITE,
-   (unsigned long)ring_ref);
+   ring_ref);
if (con->interface == NULL) {
err = EINVAL;
goto out;
@@ -804,7 +806,7 @@ static int console_init(struct console *con, struct domain 
*dom, void **data)
con->master_pollfd_idx = -1;

[Xen-devel] libxl/xenconsole: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

2017-10-16 Thread Bhupinder Thakur 
In libxl__device_vuart_add vuart_gfn is getting stored as a hex value:

> flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));

However, xenstore reads this value as a decimal value and tries to map the
wrong address and fails.

Introduced a new format string "PRIu_xen_pfn" which formats the value as a
decimal value.  Also corrected the type of ring_ref to xen_pfn_t in xenconsole.

Signed-off-by: Bhupinder Thakur 
---

This patch was verified for arm64 and arm32 toolstack compilation.

CC: Ian Jackson 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Jan Beulich 
CC: Andrew Cooper 

 tools/console/daemon/io.c| 20 +++-
 tools/libxc/include/xenctrl_compat.h |  2 +-
 tools/libxc/xc_foreign_memory.c  |  2 +-
 tools/libxl/libxl_console.c  |  4 ++--
 tools/libxl/libxl_dom.c  |  2 +-
 tools/libxl/libxl_internal.h |  2 +-
 xen/include/public/arch-arm.h|  1 +
 xen/include/public/arch-x86/xen.h|  1 +
 8 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index e22009a..6369bf4 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -80,6 +80,7 @@ static unsigned int current_array_size;
 static unsigned int nr_fds;
 
 #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
+#define INVALID_RING_REF(~(xen_pfn_t)0)
 
 struct buffer {
char *data;
@@ -98,7 +99,7 @@ struct console {
struct buffer buffer;
char *xspath;
char *log_suffix;
-   int ring_ref;
+   xen_pfn_t ring_ref;
xenevtchn_handle *xce_handle;
int xce_pollfd_idx;
int event_count;
@@ -651,22 +652,23 @@ static void console_unmap_interface(struct console *con)
 {
if (con->interface == NULL)
return;
-   if (xgt_handle && con->ring_ref == -1)
+   if (xgt_handle && con->ring_ref == INVALID_RING_REF)
xengnttab_unmap(xgt_handle, con->interface, 1);
else
munmap(con->interface, XC_PAGE_SIZE);
con->interface = NULL;
-   con->ring_ref = -1;
+   con->ring_ref = INVALID_RING_REF;
 }
  
 static int console_create_ring(struct console *con)
 {
-   int err, remote_port, ring_ref, rc;
+   int err, remote_port, rc;
+   xen_pfn_t ring_ref;
char *type, path[PATH_MAX];
struct domain *dom = con->d;
 
err = xs_gather(xs, con->xspath,
-   "ring-ref", "%u", _ref,
+   "ring-ref", "%i", _ref,
"port", "%i", _port,
NULL);
 
@@ -690,7 +692,7 @@ static int console_create_ring(struct console *con)
free(type);
 
/* If using ring_ref and it has changed, remap */
-   if (ring_ref != con->ring_ref && con->ring_ref != -1)
+   if (ring_ref != con->ring_ref && con->ring_ref != INVALID_RING_REF)
console_unmap_interface(con);
 
if (!con->interface && xgt_handle && con->use_gnttab) {
@@ -698,14 +700,14 @@ static int console_create_ring(struct console *con)
con->interface = xengnttab_map_grant_ref(xgt_handle,
dom->domid, GNTTAB_RESERVED_CONSOLE,
PROT_READ|PROT_WRITE);
-   con->ring_ref = -1;
+   con->ring_ref = INVALID_RING_REF;
}
if (!con->interface) {
/* Fall back to xc_map_foreign_range */
con->interface = xc_map_foreign_range(
xc, dom->domid, XC_PAGE_SIZE,
PROT_READ|PROT_WRITE,
-   (unsigned long)ring_ref);
+   ring_ref);
if (con->interface == NULL) {
err = EINVAL;
goto out;
@@ -804,7 +806,7 @@ static int console_init(struct console *con, struct domain 
*dom, void **data)
con->master_pollfd_idx = -1;
con->slave_fd = -1;
con->log_fd = -1;
-   con->ring_ref = -1;
+   con->ring_ref = INVALID_RING_REF;
con->local_port = -1;
con->remote_port = -1;
con->xce_pollfd_idx = -1;
diff --git a/tools/libxc/include/xenctrl_compat.h 
b/tools/libxc/include/xenctrl_compat.h
index a655e47..2d930d8 100644
--- a/tools/libxc/include/xenctrl_compat.h
+++ b/tools/libxc/include/xenctrl_compat.h
@@ -26,7 +26,7 @@
  */
 void *xc_map_foreign_range(xc_interface *xch, uint32_t dom,
 int size, int prot,
-unsigned long mfn );
+xen_pfn_t mfn );
 
 void *xc_map_foreign_pages(xc_interface *xch, uint32_t dom, int prot,
const xen_pfn_t *arr, int num );
diff --git