Parse DTB during startup, allowing memory banks and reserved
memory regions to be set up, along with early device tree node
( chosen, "xen,domain", "reserved-memory", etc ) handling.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V2:
- Drop local variable fdt_size as it is going to be used only once.
---
xen/arch/riscv/setup.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
index f531ca38ee..a345dbafeb 100644
--- a/xen/arch/riscv/setup.c
+++ b/xen/arch/riscv/setup.c
@@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
_end - _start, false) )
panic("Failed to add BOOTMOD_XEN\n");
+ BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
+
printk("All set up\n");
machine_halt();
--
2.46.2On 10.10.2024 17:30, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
> _end - _start, false) )
> panic("Failed to add BOOTMOD_XEN\n");
>
> + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
We generally aim at avoiding side effects in BUG_ON() (or ASSERT()). With
if (!boot_fdt_info(device_tree_flattened, dtb_addr))
BUG();
Acked-by: Jan Beulich <jbeulich@suse.com>
I can make the adjustment while committing, if desired.
Jan
On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
> On 10.10.2024 17:30, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
> > bootcpu_id,
> > _end - _start, false) )
> > panic("Failed to add BOOTMOD_XEN\n");
> >
> > + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
>
> We generally aim at avoiding side effects in BUG_ON() (or ASSERT()).
> With
>
> if (!boot_fdt_info(device_tree_flattened, dtb_addr))
> BUG();
>
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I can make the adjustment while committing, if desired.
It would be great if these changes could be made during the commit.
Thanks.
~ Oleksii
On 15.10.2024 11:11, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
>> On 10.10.2024 17:30, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
>>> bootcpu_id,
>>> _end - _start, false) )
>>> panic("Failed to add BOOTMOD_XEN\n");
>>>
>>> + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
>>
>> We generally aim at avoiding side effects in BUG_ON() (or ASSERT()).
>> With
>>
>> if (!boot_fdt_info(device_tree_flattened, dtb_addr))
>> BUG();
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> I can make the adjustment while committing, if desired.
> It would be great if these changes could be made during the commit.
I've committed it with the adjustment, yet once again I wondered: Why not
panic()?
Jan
On Tue, 2024-10-15 at 14:32 +0200, Jan Beulich wrote:
> On 15.10.2024 11:11, oleksii.kurochko@gmail.com wrote:
> > On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
> > > On 10.10.2024 17:30, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/setup.c
> > > > +++ b/xen/arch/riscv/setup.c
> > > > @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
> > > > bootcpu_id,
> > > > _end - _start, false) )
> > > > panic("Failed to add BOOTMOD_XEN\n");
> > > >
> > > > + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
> > >
> > > We generally aim at avoiding side effects in BUG_ON() (or
> > > ASSERT()).
> > > With
> > >
> > > if (!boot_fdt_info(device_tree_flattened, dtb_addr))
> > > BUG();
> > >
> > > Acked-by: Jan Beulich <jbeulich@suse.com>
> > >
> > > I can make the adjustment while committing, if desired.
> > It would be great if these changes could be made during the commit.
>
> I've committed it with the adjustment,
Thanks!
> yet once again I wondered: Why not
> panic()?
It could be panic() here; there's no specific reason. I agree that
using BUG() here isn't logically correct, as technically, a size of
zero for the FDT isn't a bug but rather indicates that someone provided
an incorrect FDT to Xen.
I will use panic() in the future for such cases.
It’s not always clear what should be used and where. Perhaps it would
be helpful to add some explanation somewhere.
~ Oleksii
On 16/10/2024 8:50 am, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-10-15 at 14:32 +0200, Jan Beulich wrote:
>> On 15.10.2024 11:11, oleksii.kurochko@gmail.com wrote:
>>> On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
>>>> On 10.10.2024 17:30, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/setup.c
>>>>> +++ b/xen/arch/riscv/setup.c
>>>>> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
>>>>> bootcpu_id,
>>>>> _end - _start, false) )
>>>>> panic("Failed to add BOOTMOD_XEN\n");
>>>>>
>>>>> + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
>>>> We generally aim at avoiding side effects in BUG_ON() (or
>>>> ASSERT()).
>>>> With
>>>>
>>>> if (!boot_fdt_info(device_tree_flattened, dtb_addr))
>>>> BUG();
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I can make the adjustment while committing, if desired.
>>> It would be great if these changes could be made during the commit.
>> I've committed it with the adjustment,
> Thanks!
>
>> yet once again I wondered: Why not
>> panic()?
> It could be panic() here; there's no specific reason. I agree that
> using BUG() here isn't logically correct, as technically, a size of
> zero for the FDT isn't a bug but rather indicates that someone provided
> an incorrect FDT to Xen.
>
> I will use panic() in the future for such cases.
>
> It’s not always clear what should be used and where. Perhaps it would
> be helpful to add some explanation somewhere.
panic() gets you a single line diagnostic. BUG() gets you a backtrace,
but no diagnostic information at all.
BUG() should be a last resort, because it requires someone to go digging
in the source code to even figure out what went wrong. panic() (with a
good diagnostic message, e.g. "unexpected $foo in device tree") is far
more meaningful.
There are ways of getting a backtrace in combination with a panic(), if
the situation warrants it.
~Andrew
On 16.10.2024 09:50, oleksii.kurochko@gmail.com wrote:
> On Tue, 2024-10-15 at 14:32 +0200, Jan Beulich wrote:
>> On 15.10.2024 11:11, oleksii.kurochko@gmail.com wrote:
>>> On Tue, 2024-10-15 at 08:33 +0200, Jan Beulich wrote:
>>>> On 10.10.2024 17:30, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/setup.c
>>>>> +++ b/xen/arch/riscv/setup.c
>>>>> @@ -50,6 +50,8 @@ void __init noreturn start_xen(unsigned long
>>>>> bootcpu_id,
>>>>> _end - _start, false) )
>>>>> panic("Failed to add BOOTMOD_XEN\n");
>>>>>
>>>>> + BUG_ON(!boot_fdt_info(device_tree_flattened, dtb_addr));
>>>>
>>>> We generally aim at avoiding side effects in BUG_ON() (or
>>>> ASSERT()).
>>>> With
>>>>
>>>> if (!boot_fdt_info(device_tree_flattened, dtb_addr))
>>>> BUG();
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I can make the adjustment while committing, if desired.
>>> It would be great if these changes could be made during the commit.
>>
>> I've committed it with the adjustment,
> Thanks!
>
>> yet once again I wondered: Why not
>> panic()?
> It could be panic() here; there's no specific reason. I agree that
> using BUG() here isn't logically correct, as technically, a size of
> zero for the FDT isn't a bug but rather indicates that someone provided
> an incorrect FDT to Xen.
>
> I will use panic() in the future for such cases.
>
> It’s not always clear what should be used and where. Perhaps it would
> be helpful to add some explanation somewhere.
My rule of thumb: During init and when the call stack / register state
aren't relevant to understand the details of the issue, use panic().
Jan
© 2016 - 2024 Red Hat, Inc.