Re: [PATCH v2 2/2] xen/arm: Restrict Kconfig configuration for LLC coloring

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

>>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>>> index c1f2d1b89d43..91fa579e73e5 100644
>>>> --- a/xen/arch/arm/setup.c
>>>> +++ b/xen/arch/arm/setup.c
>>>> @@ -328,6 +328,7 @@ void asmlinkage __init start_xen(unsigned long 
>>>> fdt_paddr)
>>>>                             (paddr_t)(uintptr_t)(_end - _start), false);
>>>>    BUG_ON(!xen_bootmodule);
>>>> 
>>>> +    /* This parses memory banks needed for LLC coloring */
>>> this comment is confusing. It reads as if boot_fdt_info was here only for 
>>> LLC
>>> coloring. Moreover, if you add such comment here, why not adding a comment 
>>> above
>>> boot_fdt_cmdline and cmdline_parse which are hard dependency for LLC 
>>> coloring
>>> code to read LLC cmdline options parsed by llc_coloring_init?
>> 
>> Yeah I get your point, do you think we should just go with the assert or 
>> maybe add a comment
>> on top of llc_coloring_init() to say it needs to be called after 
>> boot_fdt_info and boot_fdt_cmdline
>> in order to work? Also because the assert in get_xen_paddr (llc-coloring.c) 
>> won’t be compiled on
>> a setup not having cache coloring
> TBH I would not do anything. I assume such comment would target developers. 
> Then
> why are we special casing LLC coloring and not for example boot_fdt_cmdline 
> that
> also needs to be called after boot_fdt_info to parse legacy location for
> cmdline? There are dozens of examples in start_xen where we rely on a specific
> order and developer always needs to check if rearranging is possible. Adding a
> single comment for LLC would not improve the situation and would just result 
> in
> inconsistency leading to confusion. That's why I would only consider adding an
> ASSERT but in this case, there are more things than memory bank that LLC init
> relies on.

ok, yes of course there are a lot of things that rely on the right order of 
calls, however I felt
that an optional feature like LLC would benefit for the extra documentation. In 
other cases
rearranging calls could lead to Xen not booting, but in this case (as it 
happened to me) it
was not immediately clear.

Anyway if that’s your preference I will drop the comment, I would not add the 
assert in this
commit as it feels not the right place, but I can see that in get_xen_paddr if 
mem is empty
we would not touch paddr and have a panic later, so we would notice if 
something happen.

Cheers,
Luca