libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[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.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>

 tools/libxl/libxl_console.c       | 2 +-
 xen/include/public/arch-arm.h     | 1 +
 xen/include/public/arch-x86/xen.h | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_console.c b/tools/libxl/libxl_console.c
index c05dc28..6bfc0e5 100644
--- a/tools/libxl/libxl_console.c
+++ b/tools/libxl/libxl_console.c
@@ -376,7 +376,7 @@ int libxl__device_vuart_add(libxl__gc *gc, uint32_t domid,
     flexarray_append(ro_front, "port");
     flexarray_append(ro_front, GCSPRINTF("%"PRIu32, state->vuart_port));
     flexarray_append(ro_front, "ring-ref");
-    flexarray_append(ro_front, GCSPRINTF("%"PRI_xen_pfn, state->vuart_gfn));
+    flexarray_append(ro_front, GCSPRINTF("%"PRIu_xen_pfn, state->vuart_gfn));
     flexarray_append(ro_front, "limit");
     flexarray_append(ro_front, GCSPRINTF("%d", LIBXL_XENCONSOLE_LIMIT));
     flexarray_append(ro_front, "type");
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 5708cd2..05fd11c 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -274,6 +274,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_core_regs_t);
 
 typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn PRIx64
+#define PRIu_xen_pfn PRIu64
 
 /* Maximum number of virtual CPUs in legacy multi-processor guests. */
 /* Only one. All other VCPUS must use VCPUOP_register_vcpu_info */
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index ff91831..3b0b1d6 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -75,6 +75,7 @@ __DeFiNe__ __DECL_REG_LO16(name) e ## name
 #ifndef __ASSEMBLY__
 typedef unsigned long xen_pfn_t;
 #define PRI_xen_pfn "lx"
+#define PRIu_xen_pfn "lu"
 #endif
 
 #define XEN_HAVE_PV_GUEST_ENTRY 1
-- 
2.7.4


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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Wei Liu]

On Fri, Oct 13, 2017 at 04:14:32PM +0530, 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.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>

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

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.

Is this generic or vuart specific code in xenstore that does so?
Could you perhaps simply point me at the consuming side?

> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
> decimal value.

I ask because I'm not really happy about this addition, i.e. I'd
prefer the read side to change.

Jan


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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Andrew Cooper]

On 13/10/17 13:08, Jan Beulich wrote:
>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
> Is this generic or vuart specific code in xenstore that does so?
> Could you perhaps simply point me at the consuming side?
>
>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>> decimal value.
> I ask because I'm not really happy about this addition, i.e. I'd
> prefer the read side to change.

Can the read side realistically change?

Its been decimal for a decade now, and there definitely is 3rd party
code which uses these values in xenstore (sadly).

Then again, the ring-ref key is listed as deprecated in our
documentation, without any reference describing which key should be used
instead.  It is also typically a grant reference, not a gfn, so
something wonky is definitely going on here.

~Andrew

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
> On 13/10/17 13:08, Jan Beulich wrote:
>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>> Is this generic or vuart specific code in xenstore that does so?
>> Could you perhaps simply point me at the consuming side?
>>
>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>> decimal value.
>> I ask because I'm not really happy about this addition, i.e. I'd
>> prefer the read side to change.
> 
> Can the read side realistically change?

Well, that's why I had asked whether this is generic or specific
code. I would have hoped/assumed that xenstore doesn't
generically try to translate strings into numbers - how would it
know a string is to represent a number in the first place? Hence
I was hoping for this to be specific (and hence) new code.

> Its been decimal for a decade now, and there definitely is 3rd party
> code which uses these values in xenstore (sadly).

Are you trying to tell me there's been a vuart frontend before
the device type introduction in libxl, or is the new device type
compatible with an existing one?

> Then again, the ring-ref key is listed as deprecated in our
> documentation, without any reference describing which key should be used
> instead.  It is also typically a grant reference, not a gfn, so
> something wonky is definitely going on here.

Which put under question how an existing frontend could work
with this new device type.

Jan


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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi,

On 13/10/17 13:32, Jan Beulich wrote:
>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>> Is this generic or vuart specific code in xenstore that does so?
>>> Could you perhaps simply point me at the consuming side?
>>>
>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>> decimal value.
>>> I ask because I'm not really happy about this addition, i.e. I'd
>>> prefer the read side to change.
>>
>> Can the read side realistically change?
> 
> Well, that's why I had asked whether this is generic or specific
> code. I would have hoped/assumed that xenstore doesn't
> generically try to translate strings into numbers - how would it
> know a string is to represent a number in the first place? Hence
> I was hoping for this to be specific (and hence) new code.
> 
>> Its been decimal for a decade now, and there definitely is 3rd party
>> code which uses these values in xenstore (sadly).
> 
> Are you trying to tell me there's been a vuart frontend before
> the device type introduction in libxl, or is the new device type
> compatible with an existing one?
> 
>> Then again, the ring-ref key is listed as deprecated in our
>> documentation, without any reference describing which key should be used
>> instead.  It is also typically a grant reference, not a gfn, so
>> something wonky is definitely going on here.
> 
> Which put under question how an existing frontend could work
> with this new device type.

Well, vuart is replicating the behavior of console (see 
libxl__device_console_add). The console is passing a frame number in 
decimal in "ring-ref". Confusingly it is an MFN and would even break on 
32-bit toolstack using 64-bit Xen...

So this patch is just following the console behavior by passing a 
decimal value rather than an hexadecimal value.

Cheers,

-- 
Julien Grall

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 15:03, <julien.grall@linaro.org> wrote:
> On 13/10/17 13:32, Jan Beulich wrote:
>>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>>> Is this generic or vuart specific code in xenstore that does so?
>>>> Could you perhaps simply point me at the consuming side?
>>>>
>>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>>> decimal value.
>>>> I ask because I'm not really happy about this addition, i.e. I'd
>>>> prefer the read side to change.
>>>
>>> Can the read side realistically change?
>> 
>> Well, that's why I had asked whether this is generic or specific
>> code. I would have hoped/assumed that xenstore doesn't
>> generically try to translate strings into numbers - how would it
>> know a string is to represent a number in the first place? Hence
>> I was hoping for this to be specific (and hence) new code.
>> 
>>> Its been decimal for a decade now, and there definitely is 3rd party
>>> code which uses these values in xenstore (sadly).
>> 
>> Are you trying to tell me there's been a vuart frontend before
>> the device type introduction in libxl, or is the new device type
>> compatible with an existing one?
>> 
>>> Then again, the ring-ref key is listed as deprecated in our
>>> documentation, without any reference describing which key should be used
>>> instead.  It is also typically a grant reference, not a gfn, so
>>> something wonky is definitely going on here.
>> 
>> Which put under question how an existing frontend could work
>> with this new device type.
> 
> Well, vuart is replicating the behavior of console (see 
> libxl__device_console_add). The console is passing a frame number in 
> decimal in "ring-ref". Confusingly it is an MFN and would even break on 
> 32-bit toolstack using 64-bit Xen...
> 
> So this patch is just following the console behavior by passing a 
> decimal value rather than an hexadecimal value.

Well, that other code path should then also use PRIu_xen_pfn, at
the very least. It's of course interesting that the apparent consumer
of this (tools/console/daemon/io.c:domain_create_ring()) uses

	err = xs_gather(xs, dom->conspath,
			"ring-ref", "%u", &ring_ref,
			"port", "%i", &remote_port,
			NULL);

in order to then cast(!) the result to unsigned long in the
invocation of xc_map_foreign_range(). Suggests to me that
the console can't work reliably on a system with memory
extending past the 1Tb boundary.

It of course escapes me why %i (or really %lli) wasn't used here
from the beginning, eliminating all radix concerns and matching
what is being done for the port.

Jan


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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi Jan,

On 13/10/17 15:03, Jan Beulich wrote:
>>>> On 13.10.17 at 15:03, <julien.grall@linaro.org> wrote:
>> On 13/10/17 13:32, Jan Beulich wrote:
>>>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>>>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>>>> Is this generic or vuart specific code in xenstore that does so?
>>>>> Could you perhaps simply point me at the consuming side?
>>>>>
>>>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>>>> decimal value.
>>>>> I ask because I'm not really happy about this addition, i.e. I'd
>>>>> prefer the read side to change.
>>>>
>>>> Can the read side realistically change?
>>>
>>> Well, that's why I had asked whether this is generic or specific
>>> code. I would have hoped/assumed that xenstore doesn't
>>> generically try to translate strings into numbers - how would it
>>> know a string is to represent a number in the first place? Hence
>>> I was hoping for this to be specific (and hence) new code.
>>>
>>>> Its been decimal for a decade now, and there definitely is 3rd party
>>>> code which uses these values in xenstore (sadly).
>>>
>>> Are you trying to tell me there's been a vuart frontend before
>>> the device type introduction in libxl, or is the new device type
>>> compatible with an existing one?
>>>
>>>> Then again, the ring-ref key is listed as deprecated in our
>>>> documentation, without any reference describing which key should be used
>>>> instead.  It is also typically a grant reference, not a gfn, so
>>>> something wonky is definitely going on here.
>>>
>>> Which put under question how an existing frontend could work
>>> with this new device type.
>>
>> Well, vuart is replicating the behavior of console (see
>> libxl__device_console_add). The console is passing a frame number in
>> decimal in "ring-ref". Confusingly it is an MFN and would even break on
>> 32-bit toolstack using 64-bit Xen...
>>
>> So this patch is just following the console behavior by passing a
>> decimal value rather than an hexadecimal value.
> 
> Well, that other code path should then also use PRIu_xen_pfn, at
> the very least.

By other code path, you mean the console code right? In that case, mfn 
should also be moved from unsigned long to xen_pfn_t.

> It's of course interesting that the apparent consumer
> of this (tools/console/daemon/io.c:domain_create_ring()) uses
> 
> 	err = xs_gather(xs, dom->conspath,
> 			"ring-ref", "%u", &ring_ref,
> 			"port", "%i", &remote_port,
> 			NULL);
> 
> in order to then cast(!) the result to unsigned long in the
> invocation of xc_map_foreign_range(). Suggests to me that
> the console can't work reliably on a system with memory
> extending past the 1Tb boundary.

It likely a latent bug. Probably a silly question, would there any 
compatibility issue to switch the format to the correct one?

> 
> It of course escapes me why %i (or really %lli) wasn't used here
> from the beginning, eliminating all radix concerns and matching
> what is being done for the port.

Why %i? Should not the GFN be unsigned? Although, I can see the field 
ring_reg is int and will store -1 as not mapped. This is quite confusing 
and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.

But then, xc_map_foreign_range is using unsigned long instead of 
xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.

Note that the implementation of xc_map_foreign_range is using xen_pfn_t.

Cheers,

-- 
Julien Grall

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Jan Beulich]

>>> On 13.10.17 at 16:35, <julien.grall@linaro.org> wrote:
> Hi Jan,
> 
> On 13/10/17 15:03, Jan Beulich wrote:
>>>>> On 13.10.17 at 15:03, <julien.grall@linaro.org> wrote:
>>> On 13/10/17 13:32, Jan Beulich wrote:
>>>>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>>>>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>>>>> Is this generic or vuart specific code in xenstore that does so?
>>>>>> Could you perhaps simply point me at the consuming side?
>>>>>>
>>>>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>>>>> decimal value.
>>>>>> I ask because I'm not really happy about this addition, i.e. I'd
>>>>>> prefer the read side to change.
>>>>>
>>>>> Can the read side realistically change?
>>>>
>>>> Well, that's why I had asked whether this is generic or specific
>>>> code. I would have hoped/assumed that xenstore doesn't
>>>> generically try to translate strings into numbers - how would it
>>>> know a string is to represent a number in the first place? Hence
>>>> I was hoping for this to be specific (and hence) new code.
>>>>
>>>>> Its been decimal for a decade now, and there definitely is 3rd party
>>>>> code which uses these values in xenstore (sadly).
>>>>
>>>> Are you trying to tell me there's been a vuart frontend before
>>>> the device type introduction in libxl, or is the new device type
>>>> compatible with an existing one?
>>>>
>>>>> Then again, the ring-ref key is listed as deprecated in our
>>>>> documentation, without any reference describing which key should be used
>>>>> instead.  It is also typically a grant reference, not a gfn, so
>>>>> something wonky is definitely going on here.
>>>>
>>>> Which put under question how an existing frontend could work
>>>> with this new device type.
>>>
>>> Well, vuart is replicating the behavior of console (see
>>> libxl__device_console_add). The console is passing a frame number in
>>> decimal in "ring-ref". Confusingly it is an MFN and would even break on
>>> 32-bit toolstack using 64-bit Xen...
>>>
>>> So this patch is just following the console behavior by passing a
>>> decimal value rather than an hexadecimal value.
>> 
>> Well, that other code path should then also use PRIu_xen_pfn, at
>> the very least.
> 
> By other code path, you mean the console code right? In that case, mfn 
> should also be moved from unsigned long to xen_pfn_t.

Yes.

>> It's of course interesting that the apparent consumer
>> of this (tools/console/daemon/io.c:domain_create_ring()) uses
>> 
>> 	err = xs_gather(xs, dom->conspath,
>> 			"ring-ref", "%u", &ring_ref,
>> 			"port", "%i", &remote_port,
>> 			NULL);
>> 
>> in order to then cast(!) the result to unsigned long in the
>> invocation of xc_map_foreign_range(). Suggests to me that
>> the console can't work reliably on a system with memory
>> extending past the 1Tb boundary.
> 
> It likely a latent bug. Probably a silly question, would there any 
> compatibility issue to switch the format to the correct one?

I don't think so.

>> It of course escapes me why %i (or really %lli) wasn't used here
>> from the beginning, eliminating all radix concerns and matching
>> what is being done for the port.
> 
> Why %i? Should not the GFN be unsigned?

Signedness is secondary here - the important thing is that %i is
the only one allowing all of decimal, hex, and octal formatting of
the string (the latter two of course with the usual 0 / 0x prefixes).
Port numbers are unsigned too, yet %i is being used there.

> Although, I can see the field 
> ring_reg is int and will store -1 as not mapped. This is quite confusing 
> and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.

Indeed.

> But then, xc_map_foreign_range is using unsigned long instead of 
> xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.

Yes.

> Note that the implementation of xc_map_foreign_range is using xen_pfn_t.

And yes again.

Jan

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Julien Grall]

Hi,

Sorry for the top-posting. Bhupinder, can you give a look?

Cheers,

On 13/10/17 16:06, Jan Beulich wrote:
>>>> On 13.10.17 at 16:35, <julien.grall@linaro.org> wrote:
>> Hi Jan,
>>
>> On 13/10/17 15:03, Jan Beulich wrote:
>>>>>> On 13.10.17 at 15:03, <julien.grall@linaro.org> wrote:
>>>> On 13/10/17 13:32, Jan Beulich wrote:
>>>>>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>>>>>> Is this generic or vuart specific code in xenstore that does so?
>>>>>>> Could you perhaps simply point me at the consuming side?
>>>>>>>
>>>>>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>>>>>> decimal value.
>>>>>>> I ask because I'm not really happy about this addition, i.e. I'd
>>>>>>> prefer the read side to change.
>>>>>>
>>>>>> Can the read side realistically change?
>>>>>
>>>>> Well, that's why I had asked whether this is generic or specific
>>>>> code. I would have hoped/assumed that xenstore doesn't
>>>>> generically try to translate strings into numbers - how would it
>>>>> know a string is to represent a number in the first place? Hence
>>>>> I was hoping for this to be specific (and hence) new code.
>>>>>
>>>>>> Its been decimal for a decade now, and there definitely is 3rd party
>>>>>> code which uses these values in xenstore (sadly).
>>>>>
>>>>> Are you trying to tell me there's been a vuart frontend before
>>>>> the device type introduction in libxl, or is the new device type
>>>>> compatible with an existing one?
>>>>>
>>>>>> Then again, the ring-ref key is listed as deprecated in our
>>>>>> documentation, without any reference describing which key should be used
>>>>>> instead.  It is also typically a grant reference, not a gfn, so
>>>>>> something wonky is definitely going on here.
>>>>>
>>>>> Which put under question how an existing frontend could work
>>>>> with this new device type.
>>>>
>>>> Well, vuart is replicating the behavior of console (see
>>>> libxl__device_console_add). The console is passing a frame number in
>>>> decimal in "ring-ref". Confusingly it is an MFN and would even break on
>>>> 32-bit toolstack using 64-bit Xen...
>>>>
>>>> So this patch is just following the console behavior by passing a
>>>> decimal value rather than an hexadecimal value.
>>>
>>> Well, that other code path should then also use PRIu_xen_pfn, at
>>> the very least.
>>
>> By other code path, you mean the console code right? In that case, mfn
>> should also be moved from unsigned long to xen_pfn_t.
> 
> Yes.
> 
>>> It's of course interesting that the apparent consumer
>>> of this (tools/console/daemon/io.c:domain_create_ring()) uses
>>>
>>> 	err = xs_gather(xs, dom->conspath,
>>> 			"ring-ref", "%u", &ring_ref,
>>> 			"port", "%i", &remote_port,
>>> 			NULL);
>>>
>>> in order to then cast(!) the result to unsigned long in the
>>> invocation of xc_map_foreign_range(). Suggests to me that
>>> the console can't work reliably on a system with memory
>>> extending past the 1Tb boundary.
>>
>> It likely a latent bug. Probably a silly question, would there any
>> compatibility issue to switch the format to the correct one?
> 
> I don't think so.
> 
>>> It of course escapes me why %i (or really %lli) wasn't used here
>>> from the beginning, eliminating all radix concerns and matching
>>> what is being done for the port.
>>
>> Why %i? Should not the GFN be unsigned?
> 
> Signedness is secondary here - the important thing is that %i is
> the only one allowing all of decimal, hex, and octal formatting of
> the string (the latter two of course with the usual 0 / 0x prefixes).
> Port numbers are unsigned too, yet %i is being used there.
> 
>> Although, I can see the field
>> ring_reg is int and will store -1 as not mapped. This is quite confusing
>> and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.
> 
> Indeed.
> 
>> But then, xc_map_foreign_range is using unsigned long instead of
>> xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.
> 
> Yes.
> 
>> Note that the implementation of xc_map_foreign_range is using xen_pfn_t.
> 
> And yes again.
> 
> Jan
> 

-- 
Julien Grall

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

Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add

[Bhupinder Thakur]

On 13 October 2017 at 20:36, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 13.10.17 at 16:35, <julien.grall@linaro.org> wrote:
>> Hi Jan,
>>
>> On 13/10/17 15:03, Jan Beulich wrote:
>>>>>> On 13.10.17 at 15:03, <julien.grall@linaro.org> wrote:
>>>> On 13/10/17 13:32, Jan Beulich wrote:
>>>>>>>> On 13.10.17 at 14:19, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 13/10/17 13:08, Jan Beulich wrote:
>>>>>>>>>> On 13.10.17 at 12:44, <bhupinder.thakur@linaro.org> 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.
>>>>>>> Is this generic or vuart specific code in xenstore that does so?
>>>>>>> Could you perhaps simply point me at the consuming side?
>>>>>>>
>>>>>>>> Introduced a new format string "PRIu_xen_pfn" which formats the value as a
>>>>>>>> decimal value.
>>>>>>> I ask because I'm not really happy about this addition, i.e. I'd
>>>>>>> prefer the read side to change.
>>>>>>
>>>>>> Can the read side realistically change?
>>>>>
>>>>> Well, that's why I had asked whether this is generic or specific
>>>>> code. I would have hoped/assumed that xenstore doesn't
>>>>> generically try to translate strings into numbers - how would it
>>>>> know a string is to represent a number in the first place? Hence
>>>>> I was hoping for this to be specific (and hence) new code.
>>>>>
>>>>>> Its been decimal for a decade now, and there definitely is 3rd party
>>>>>> code which uses these values in xenstore (sadly).
>>>>>
>>>>> Are you trying to tell me there's been a vuart frontend before
>>>>> the device type introduction in libxl, or is the new device type
>>>>> compatible with an existing one?
>>>>>
>>>>>> Then again, the ring-ref key is listed as deprecated in our
>>>>>> documentation, without any reference describing which key should be used
>>>>>> instead.  It is also typically a grant reference, not a gfn, so
>>>>>> something wonky is definitely going on here.
>>>>>
>>>>> Which put under question how an existing frontend could work
>>>>> with this new device type.
>>>>
>>>> Well, vuart is replicating the behavior of console (see
>>>> libxl__device_console_add). The console is passing a frame number in
>>>> decimal in "ring-ref". Confusingly it is an MFN and would even break on
>>>> 32-bit toolstack using 64-bit Xen...
>>>>
>>>> So this patch is just following the console behavior by passing a
>>>> decimal value rather than an hexadecimal value.
>>>
>>> Well, that other code path should then also use PRIu_xen_pfn, at
>>> the very least.
>>
>> By other code path, you mean the console code right? In that case, mfn
>> should also be moved from unsigned long to xen_pfn_t.
>
> Yes.
>
ok.

>>> It's of course interesting that the apparent consumer
>>> of this (tools/console/daemon/io.c:domain_create_ring()) uses
>>>
>>>      err = xs_gather(xs, dom->conspath,
>>>                      "ring-ref", "%u", &ring_ref,
>>>                      "port", "%i", &remote_port,
>>>                      NULL);
>>>
>>> in order to then cast(!) the result to unsigned long in the
>>> invocation of xc_map_foreign_range(). Suggests to me that
>>> the console can't work reliably on a system with memory
>>> extending past the 1Tb boundary.
>>
>> It likely a latent bug. Probably a silly question, would there any
>> compatibility issue to switch the format to the correct one?
>
> I don't think so.
>
>>> It of course escapes me why %i (or really %lli) wasn't used here
>>> from the beginning, eliminating all radix concerns and matching
>>> what is being done for the port.
>>
>> Why %i? Should not the GFN be unsigned?
>
> Signedness is secondary here - the important thing is that %i is
> the only one allowing all of decimal, hex, and octal formatting of
> the string (the latter two of course with the usual 0 / 0x prefixes).
> Port numbers are unsigned too, yet %i is being used there.
>
>> Although, I can see the field
>> ring_reg is int and will store -1 as not mapped. This is quite confusing
>> and likely we want to turned into xen_pfn_t + use ~(xen_pfn_t)0.
>
> Indeed.
>
ok. I will modify the ring-ref type to xen_pfn_t.

>> But then, xc_map_foreign_range is using unsigned long instead of
>> xen_pfn_t. So I guess we should also switch the parameter to xen_pfn_t.
>
> Yes.
>
ok.

Regards,
Bhupinder

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