[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.13] xen/arm: Don't use _end in is_xen_fixed_mfn()



Hi George,

On 16/10/2019 11:38, George Dunlap wrote:
On 10/16/19 11:31 AM, Julien Grall wrote:
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?

And if they're the same, then you can just use `<` instead of `<=`, and
not have to worry about casting before subtracting.

_end is not part of the binary but points one past it. So there is no guarantee this address will be mapped in the virtual address space.

As I pointed out in my previous e-mail the result of virt_to_maddr() will be undefined in this case. On Arm, this will actually crash Xen.

So you can't use '<' here.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.