On 10/16/19 11:41 AM, Jürgen Groß wrote:
> On 16.10.19 12:38, George Dunlap wrote:
>> On 10/16/19 11:31 AM, Julien Grall wrote:
>>> Hi George,
>>>
>>> On 16/10/2019 11:22, George Dunlap wrote:
>>>> On 10/16/19 11:18 AM, Ian Jackson wrote:
>>>>> Stefano Stabellini writes ("Re: [PATCH for-4.13] xen/arm: Don't use
>>>>> _end in is_xen_fixed_mfn()"):
>>>>>> My suggestion is going to work: "the compiler sees through casts"
>>>>>> referred to comparisons between pointers, where we temporarily casted
>>>>>> both pointers to integers and back to pointers via a MACRO. That case
>>>>>> was iffy because the MACRO was clearly a workaround the spec.
>>>>>>
>>>>>> Here the situation is different. For one, we are doing arithmetic.
>>>>>> Also
>>>>>> virt_to_maddr already takes a vaddr_t as argument. So instead of
>>>>>> doing
>>>>>> pointers arithmetic, then converting to vaddr_t, we are converting to
>>>>>> vaddr_t first, then doing arithmetics, which is fine both from a C99
>>>>>> point of view and even a MISRA C point of view. I can't see a problem
>>>>>> with that. I am sure as I reasonable can be :-)
>>>>>
>>>>> FTAOD I think you are suggesting this:
>>>>> - + (mfn_to_maddr(mfn) <= virt_to_maddr(_end - 1)))
>>>>> + + (mfn_to_maddr(mfn) <= virt_to_maddr(((vaddr_t)_end - 1)))
>>>>>
>>>>> virt_to_maddr(va) is a macro which expands to
>>>>> __virt_to_maddr((vaddr_t)(va))
>>>>>
>>>>> So what is happening here is that the cast to an integer type is being
>>>>> done before the subtraction.
>>>>>
>>>>> Without the cast, you are calculating the pointer value _end-1 from
>>>>> the value _end, which is UB. With the cast you are calculating an
>>>>> integer value. vaddr_t is unsigned, so all arithmetic operations are
>>>>> defined. Nothing casts the result back to the "forbidden" (with this
>>>>> provenance) pointer value, so all is well.
>>>>>
>>>>> (With the macro expansion the cast happens twice. This is probably
>>>>> better than using __virt_to_maddr here.)
>>>>>
>>>>> Ie, in this case I agree with Stefano. The cast is both necessary and
>>>>> sufficient.
>>>>
>>>> Maybe I missed something, but why are we using `<=` at all? Why not
>>>> use
>>>> `<`?
>>>>
>>>> Or is this some weird C pointer comparison UB thing?
>>>
>>> _end may not be mapped in the virtual address space. This is the case
>>> when the size of Xen is page-aligned. So _end will point to the next
>>> page.
>>>
>>> virt_to_maddr() cannot fail so it should only be called on valid virtual
>>> address. The behavior is undefined in all the other cases.
>>>
>>> On x86, you might be able to get away because you translate the virtual
>>> address to physical address in software.
>>>
>>> On Arm, we are using the hardware instruction to do the translation. As
>>> _end is not always mapped, then the translation may fail. In other word,
>>> Xen will crash.
>>
>> None of this explains my question.
>>
>> Is it not the case that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)`
>> is true, then `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will be true,
>> and that if `mfn_to_maddr(mfn) <= virt_to_maddr(_end-1)` is false, then
>> `mfn_to_maddr(mfn) < virt_to_maddr(_end)` will also be false?
>>
>> Under what conditions would they be different?
>
> In case virt_to_maddr(_end) is undefined due to no translation being
> available?
Ah, gotcha. Sorry for the noise.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
