|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t
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 seriesThe 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;Because, we still need some way to convert u64 dt address/size to paddr_t (u64/u32) address/size. Please correct me if I misunderstand something. - Ayan However, if you both want to do it, then I think it would be best to introduce wrappers over the Device-Tree ones that will check the truncation. For example, we could introduce dt_device_get_address_checked() that will call dt_device_get_address() and then check for 32-bit truncation. For the function device_tree_get_reg(), this helper was aimed to deal with just Physical address 'reg' very early. So we can modify the prototype and update the function to check for 32-bit truncation. Note that I am suggesting a different approach for the two helpers because the former is generic and it is not clear to me whether this could be used in another context than physical address.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |