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.or...@amd.com> 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.
Andrea
