On 23/12/2022 10:01, Ayan Kumar Halder wrote:
Hi Julien/Stefano,
I want to make sure I understand correctly.
On 22/12/2022 23:20, Stefano Stabellini wrote:
On Sat, 17 Dec 2022, Julien Grall wrote:
On 17/12/2022 00:46, Stefano Stabellini wrote:
On Fri, 16 Dec 2022, Julien Grall wrote:
Hi Ayan,
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
paddr_t may be u64 or u32 depending of the type of architecture.
Thus, while translating between u64 and paddr_t, one should check
that
the
truncated bits are 0. If not, then raise an appropriate error.
I am not entirely convinced this extra helper is worth it. If the user
can't
provide 32-bit address in the DT, then there are also a lot of
other part
that
can go wrong.
Bertrand, Stefano, what do you think?
In general, it is not Xen's job to protect itself against bugs in
device
tree. However, if Xen spots a problem in DT and prints a helpful
message
that is better than just crashing because it gives a hint to the
developer about what the problem is.
I agree with the principle. In practice this needs to be weight out
with the
long-term maintenance.
In this case, I think a BUG_ON would be sufficient.
BUG_ON() is the same as panic(). They both should be used only when
there are
no way to recover (see CODING_STYLE).
If we parse the device-tree at boot, then BUG_ON() is ok. However, if
we need
to parse it after boot (e.g. the DT overlay from Vikram), then we should
definitely not call BUG_ON() for checking DT input.
yeah, I wasn't thinking of that series
The correct way is like Ayan did by checking returning an error and let
the caller deciding what to do.
My concern with his approach is the extra amount of code in each
caller to
check it (even with a new helper).
I am not fully convinced that checking the addresses in the DT is
that useful.
I am also happy not to do it to be honest
So are you suggesting that we do the truncation, but do not check if any
non zero bits have been truncated.
As an example, currently with this patch :-
- device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase,
&gbase);
- psize = dt_read_number(cells, size_cells);
+ device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase,
&dt_gbase);
+ ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase,
&gbase);
+ if ( ret )
+ return ret;
+ dt_psize = dt_read_number(cells, size_cells);
+ ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);
With your proposed change, it should be
- device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase,
&gbase);
- psize = dt_read_number(cells, size_cells);
+ device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase,
&dt_gbase);
+ dt_psize = dt_read_number(cells, size_cells);
+ pbase = (paddr_t) dt_pbase;
+ gbase = (paddr_t) dt_gbase;
+ psize = (paddr_t) dt_psize;
-ETOOMANY cast and lines (the more if this is expected to be
duplicated). But the last one seems unnecessary as the only reason you
need separate variable for pbase and gbase is because the function are
taking a reference rather than returning the value.
Because, we still need some way to convert u64 dt address/size to
paddr_t (u64/u32) address/size.
How about following the approach I suggested in my previous e-mail to
Stefano:
- Convert device_tree_get_reg() to use paddr_t.
- Introduce dt_device_get_address_checked() (Assuming you want to
still add the check)
We may need an helper to wrap around dt_read_number() but I don't have a
good idea for a name. There are only a couple of use. So I think it is
fine to open-code. But you would not need a separate local variable. For
the cast, I am in two mind. In one way, I don't like unnecessary
explicit cast, but on the other way it serves as documentation.
Stefano, any opinions?
Cheers,
--
Julien Grall
