Re: [for-4.20] Re: [PATCH v12 12/12] xen/arm: add cache coloring support for Xen image

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2025 18:13, Andrea Bastoni wrote:
> On 07/01/2025 18:01, Jan Beulich wrote:
>> On 07.01.2025 17:51, Michal Orzel wrote:
>>> On 07/01/2025 17:42, Julien Grall wrote:
>>>> On 16/12/2024 14:36, Jan Beulich wrote:
>>>>> On 16.12.2024 15:28, Carlo Nonato wrote:
>>>>>> On Mon, Dec 16, 2024 at 2:56 PM Michal Orzel <michal.orzel@xxxxxxx> 
>>>>>> wrote:
>>>>>>> On 13/12/2024 17:28, Carlo Nonato wrote:
>>>>>>>> --- a/xen/arch/arm/arm64/mmu/mm.c
>>>>>>>> +++ b/xen/arch/arm/arm64/mmu/mm.c
>>>>>>>> @@ -1,6 +1,7 @@
>>>>>>>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>>>>>>>
>>>>>>>>   #include <xen/init.h>
>>>>>>>> +#include <xen/llc-coloring.h>
>>>>>>>>   #include <xen/mm.h>
>>>>>>>>   #include <xen/pfn.h>
>>>>>>>>
>>>>>>>> @@ -138,8 +139,36 @@ void update_boot_mapping(bool enable)
>>>>>>>>   }
>>>>>>>>
>>>>>>>>   extern void switch_ttbr_id(uint64_t ttbr);
>>>>>>>> +extern void relocate_xen(uint64_t ttbr, void *src, void *dst, size_t 
>>>>>>>> len);
>>>>>>>>
>>>>>>>>   typedef void (switch_ttbr_fn)(uint64_t ttbr);
>>>>>>>> +typedef void (relocate_xen_fn)(uint64_t ttbr, void *src, void *dst, 
>>>>>>>> size_t len);
>>>>>>>> +
>>>>>>>> +void __init relocate_and_switch_ttbr(uint64_t ttbr) {
>>>>>>> CODING_STYLE: { needs to be on its own line
>>>>>>>
>>>>>>> Also, this function is only executed in case of LLC coloring, so 
>>>>>>> shouldn't it
>>>>>>> be #ifdef protected.
>>>>>>
>>>>>> Here and in other places (patch #8) I'm relying on DCE to remove 
>>>>>> functions
>>>>>> that are not called. This was a suggestion from Jan in that patch. Can we
>>>>>> adopt the same here?
>>>>>
>>>>> Yet how would the compiler spot that the function is unused? That would 
>>>>> only
>>>>> work with LTO / WPO. DCE (as I did suggest elsewhere) requires the 
>>>>> functions
>>>>> in question to be static (allowing the compiler to see enough of the whole
>>>>> picture).
>>>>
>>>> Sorry for the late answer. I was away with limited e-mail access. While
>>>> looking what was committing recently, I noticed that a dummy function
>>>> was introduced:
>>>>
>>>> void __init relocate_and_switch_ttbr(uint64_t ttbr) {}
>>>>
>>>> If a function is not supposed to be called, then it should contain a
>>>> BUG_ON() to catch any misusage. Otherwise, this is a recipe for
>>>> disaster. In this case, it would not be trivial to notice the TTBR was
>>>> not switched...
>>>>
>>>> That said I would have actually considered to remove the empty stub. I
>>>> am a bit surprised that DCE wouldn't work in this case because the call
>>>> is protected with "if ( llc_coloring_enabled )". When cache coloring is
>>>> not enabled, this would turn to an "if ( false )" and therefore all the
>>>> code should be removed. What did I miss?
>>>>
>>>> Note that this is what we already rely on for arm32 because there is no
>>>> stub... So if this is problem then we definitely need to fix it on arm32
>>>> as well...
>>>>
>>>> IOW, we either introduce a stub (including the BUG_ON) for both arm32
>>>> and arm64 in the header or we remove the stub completely.
>>>>
>>>> Marco, Michal, can you have a look? Ideally, this should be fixed for 4.20.
>>> I did a test with GCC 13.2 and I can compile it fine with stub removed. 
>>> That said,
>>> I'm not a compiler expert and I'm not sure if this behavior stays the same 
>>> with different
>>> compiler options/optimizations. So it's more like a question to Jan. I'm 
>>> happy either way.
>>
>> We use the same (if(...) func();) in various places, relying on said DCEing
>> of the call when the condition is compile-time-false. I see no reason why
>> it couldn't be used here as well.
> 
> IIRC the point was that his function is extern and DCE as used in other 
> places doesn't necessarily work.

But the called function being extern _is_ the common pattern where merely a
declaration needs to be visible, but no definition (and we specifically have
a Misra deviation to cover this case). If the function was static, no further
provisions would be necessary at all, as then the compiler can DCE not only
the function call, but also the function itself, without any further help by
us.

Jan