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

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