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

[Julien Grall <julien.grall@linaro.org>]

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