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

[Andrea Bastoni <andrea.bastoni@xxxxxxxxxxxxxxx>]

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:
>>>>
>>>>
>>>> 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.
>>
>> 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 in this case I can confirm what Michal reported:

> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> index 26361c4fe4..c1efa1348a 100644
> --- a/xen/arch/arm/arm64/mmu/mm.c
> +++ b/xen/arch/arm/arm64/mmu/mm.c
> @@ -171,8 +171,6 @@ void __init relocate_and_switch_ttbr(uint64_t ttbr)
>       */
>      update_identity_mapping(false);
>  }
> -#else
> -void __init relocate_and_switch_ttbr(uint64_t ttbr) {}
>  #endif
> 
>  void __init switch_ttbr(uint64_t ttbr)

When compiled (aarch64-linux-gnu-gcc (Debian 10.2.1-6)) without LLC_COLORING: 
compiles, does not result in undefined symbols, and relocate_and_switch_ttbr() 
is not present in the object 

Thanks,
Andrea