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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 07/01/2025 17:42, Julien Grall wrote:
> 
> 
> Hi,
> 
> 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.

~Michal