| Message ID | 70c98147d7f977649ca3eb1a82cd76aeb0df1b14.1728481578.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Parse and handle command line from dtb | expand |
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. 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 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
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
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();
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(+)