* libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
@ 2017-10-13 10:44 Bhupinder Thakur
2017-10-13 11:28 ` Wei Liu
2017-10-13 12:08 ` Jan Beulich
0 siblings, 2 replies; 11+ messages in thread
From: Bhupinder Thakur @ 2017-10-13 10:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Ian Jackson,
Julien Grall, Jan Beulich
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 10:44 libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add Bhupinder Thakur
@ 2017-10-13 11:28 ` Wei Liu
2017-10-13 12:08 ` Jan Beulich
1 sibling, 0 replies; 11+ messages in thread
From: Wei Liu @ 2017-10-13 11:28 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Ian Jackson,
Julien Grall, Jan Beulich, xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 10:44 libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add Bhupinder Thakur
2017-10-13 11:28 ` Wei Liu
@ 2017-10-13 12:08 ` Jan Beulich
2017-10-13 12:19 ` Andrew Cooper
1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2017-10-13 12:08 UTC (permalink / raw)
To: Bhupinder Thakur
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Ian Jackson,
Julien Grall, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 12:08 ` Jan Beulich
@ 2017-10-13 12:19 ` Andrew Cooper
2017-10-13 12:32 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2017-10-13 12:19 UTC (permalink / raw)
To: Jan Beulich, Bhupinder Thakur
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 12:19 ` Andrew Cooper
@ 2017-10-13 12:32 ` Jan Beulich
2017-10-13 13:03 ` Julien Grall
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2017-10-13 12:32 UTC (permalink / raw)
To: Andrew Cooper, Bhupinder Thakur
Cc: IanJackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 12:32 ` Jan Beulich
@ 2017-10-13 13:03 ` Julien Grall
2017-10-13 14:03 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2017-10-13 13:03 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Bhupinder Thakur
Cc: Wei Liu, Julien Grall, Stefano Stabellini, IanJackson, xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 13:03 ` Julien Grall
@ 2017-10-13 14:03 ` Jan Beulich
2017-10-13 14:35 ` Julien Grall
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2017-10-13 14:03 UTC (permalink / raw)
To: Andrew Cooper, Bhupinder Thakur, Julien Grall
Cc: IanJackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 14:03 ` Jan Beulich
@ 2017-10-13 14:35 ` Julien Grall
2017-10-13 15:06 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2017-10-13 14:35 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Bhupinder Thakur
Cc: IanJackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 14:35 ` Julien Grall
@ 2017-10-13 15:06 ` Jan Beulich
2017-10-13 16:32 ` Julien Grall
2017-10-16 9:03 ` Bhupinder Thakur
0 siblings, 2 replies; 11+ messages in thread
From: Jan Beulich @ 2017-10-13 15:06 UTC (permalink / raw)
To: Andrew Cooper, Bhupinder Thakur, Julien Grall
Cc: IanJackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 15:06 ` Jan Beulich
@ 2017-10-13 16:32 ` Julien Grall
2017-10-16 9:03 ` Bhupinder Thakur
1 sibling, 0 replies; 11+ messages in thread
From: Julien Grall @ 2017-10-13 16:32 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper, Bhupinder Thakur
Cc: IanJackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
2017-10-13 15:06 ` Jan Beulich
2017-10-13 16:32 ` Julien Grall
@ 2017-10-16 9:03 ` Bhupinder Thakur
1 sibling, 0 replies; 11+ messages in thread
From: Bhupinder Thakur @ 2017-10-16 9:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall,
IanJackson, Julien Grall, Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-10-16 9:03 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-13 10:44 libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add Bhupinder Thakur
2017-10-13 11:28 ` Wei Liu
2017-10-13 12:08 ` Jan Beulich
2017-10-13 12:19 ` Andrew Cooper
2017-10-13 12:32 ` Jan Beulich
2017-10-13 13:03 ` Julien Grall
2017-10-13 14:03 ` Jan Beulich
2017-10-13 14:35 ` Julien Grall
2017-10-13 15:06 ` Jan Beulich
2017-10-13 16:32 ` Julien Grall
2017-10-16 9:03 ` Bhupinder Thakur
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.