Re: [PATCH v3 2/4] xen/x86: introduce self modifying code test

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2023 16:28, Roger Pau Monné wrote:
> On Thu, Dec 14, 2023 at 02:57:11PM +0100, Jan Beulich wrote:
>> On 14.12.2023 14:47, Roger Pau Monné wrote:
>>> On Thu, Dec 14, 2023 at 12:55:22PM +0100, Jan Beulich wrote:
>>>> On 14.12.2023 11:17, Roger Pau Monne wrote:
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -58,6 +58,7 @@
>>>>>  #include <asm/microcode.h>
>>>>>  #include <asm/prot-key.h>
>>>>>  #include <asm/pv/domain.h>
>>>>> +#include <asm/test-smoc.h>
>>>>>  
>>>>>  /* opt_nosmp: If true, secondary processors are ignored. */
>>>>>  static bool __initdata opt_nosmp;
>>>>> @@ -1951,6 +1952,8 @@ void asmlinkage __init noreturn 
>>>>> __start_xen(unsigned long mbi_p)
>>>>>  
>>>>>      alternative_branches();
>>>>>  
>>>>> +    test_smoc(XEN_SYSCTL_TEST_SMOC_ALL, NULL);
>>>>
>>>> I realize I'm at risk of causing scope creep, but I'd still like to at
>>>> least ask: As further self-tests are added, we likely don't want to
>>>> alter __start_xen() every time. Should there perhaps better be a wrapper
>>>> (going forward: multiple ones, depending on the time tests want invoking),
>>>> together with a Kconfig control to allow suppressing all of these tests in
>>>> at least release builds?
>>>
>>> Right now I only had in mind that livepatch related tests won't be
>>> executed as part of the call in __start_xen(), but all the other ones
>>> would, and hence wasn't expecting the code to change from the form in
>>> the next patch.
>>
>> Well, I was thinking of there more stuff appearing in test/, not self-
>> modifying-code related, and hence needing further test_*() alongside.
>> test_smoc().
> 
> Oh, I see.  I think it might be best to introduce such wrapper when we
> have at least 2 different self tests?  Otherwise it would be weird IMO
> to have another function (ie: execute_self_tests()?) that's just a
> wrapper around test_smoc().

That's precisely why I said "risk of causing scope creep, but I'd still
like to at least ask". I'm okay-ish, as long as it's clear that this
way more code churn may happen down the road. Same ...

>>>>> --- a/xen/common/kernel.c
>>>>> +++ b/xen/common/kernel.c
>>>>> @@ -386,13 +386,14 @@ char *print_tainted(char *str)
>>>>>  {
>>>>>      if ( tainted )
>>>>>      {
>>>>> -        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c",
>>>>> +        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c%c",
>>>>>                   tainted & TAINT_MACHINE_INSECURE ? 'I' : ' ',
>>>>>                   tainted & TAINT_MACHINE_CHECK ? 'M' : ' ',
>>>>>                   tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ',
>>>>>                   tainted & TAINT_ERROR_INJECT ? 'E' : ' ',
>>>>>                   tainted & TAINT_HVM_FEP ? 'H' : ' ',
>>>>> -                 tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ');
>>>>> +                 tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ',
>>>>> +                 tainted & TAINT_ERROR_SMOC ? 'A' : ' ');
>>>>
>>>> How well is this going to scale as other selftests are added? IOW should
>>>> this taint really be self-modifying-code-specific?
>>>
>>> I'm afraid I'm not sure I'm following.  Would you instead like to make
>>> the taint per-test selectable?
>>
>> The other way around actually: Taint generally for failed selftests,
>> not just for the self-modifying-code one (which ends up being the only
>> one right now).
> 
> So the suggestion would be to use TAINT_ERROR_SELFTEST instead of
> TAINT_ERROR_SMOC?  I can do that, but it might also be more
> appropriate when there are more self tests.

... here - of course we can also rename later.

Jan