Re: [PATCH v3 3/3] xsm: properly handle error from XSM init

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 6/1/22 02:14, Jan Beulich wrote:
> On 31.05.2022 17:08, Daniel P. Smith wrote:
>> @@ -1690,7 +1691,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>  
>>      open_softirq(NEW_TLBFLUSH_CLOCK_PERIOD_SOFTIRQ, 
>> new_tlbflush_clock_period);
>>  
>> -    if ( opt_watchdog ) 
>> +    if ( opt_watchdog )
>>          nmi_watchdog = NMI_LOCAL_APIC;
>>  
>>      find_smp_config();
> 
> Please omit formatting changes to entirely unrelated pieces of code.

Ack. this was in simliar vein of the other patches where I cleaned
nearby trailing space.

>> @@ -1700,7 +1701,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>      mmio_ro_ranges = rangeset_new(NULL, "r/o mmio ranges",
>>                                    RANGESETF_prettyprint_hex);
>>  
>> -    xsm_multiboot_init(module_map, mbi);
>> +    if ( xsm_multiboot_init(module_map, mbi) )
>> +        warning_add("WARNING: XSM failed to initialize.\n"
>> +                    "This has implications on the security of the system,\n"
>> +                    "as uncontrolled communications between trusted and\n"
>> +                    "untrusted domains may occur.\n");
> 
> Uncontrolled communication isn't the only thing that could occur, aiui.
> So at the very least "e.g." or some such would want adding imo.

Agreed, this was a reuse of the existing message and honestly I would
like to believe this was the original author's attempt at brevity versus
writing a detailed message of every implication to the security of the
system.

> Now that return values are checked, I think that in addition to what
> you already do the two function declarations may want decorating with
> __must_check.

Understood but likely not necessary based on Andy's review suggestion to
move these functions to void.

v/r,
dps