[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] libxl: vpl011: Fix hex to dec conversion of vuart_gfn in libxl__device_vuart_add
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@xxxxxxxxxx> wrote:
Hi Jan,
On 13/10/17 15:03, Jan Beulich wrote:
On 13.10.17 at 15:03, <julien.grall@xxxxxxxxxx> wrote:
On 13/10/17 13:32, Jan Beulich wrote:
On 13.10.17 at 14:19, <andrew.cooper3@xxxxxxxxxx> wrote:
On 13/10/17 13:08, Jan Beulich wrote:
On 13.10.17 at 12:44, <bhupinder.thakur@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
