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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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. 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel