Re: [PATCH v2 2/6] x86/APIC: drop clustered_apic_check() hook

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.11.2021 12:02, Roger Pau Monné wrote:
> On Fri, Nov 05, 2021 at 01:34:12PM +0100, Jan Beulich wrote:
>> The hook functions have been empty forever (x2APIC) or issuing merely a
>> printk() for a long time (xAPIC). Since that printk() is (a) generally
>> useful (i.e. also in the x2APIC case) and (b) would better only be
>> issued once the final APIC driver to use was determined, move (and
>> generalize) it into connect_bsp_APIC().
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -243,6 +243,12 @@ void __init connect_bsp_APIC(void)
>>          outb(0x70, 0x22);
>>          outb(0x01, 0x23);
>>      }
>> +
>> +    printk("Enabling APIC mode:  %s.  Using %d I/O APICs\n",
> 
> I don't think it makes sense to prefix APIC with 'x' or 'x2' here, as
> we already print the APIC mode elsewhere?

I was indeed pondering that, and decided that the extra yet redundant
information wouldn't be worth the extra logic here (the more that
there's no good way to optionally print a single character, as sadly
%c does not print nothing when passed '\0', and I find single-char
string literals kind of ugly / wasteful). But I have no strong
opinion here, so if you think it would be better to add the extra
bits, I'll happily do so.

>> +           !INT_DEST_MODE ? "Physical"
>> +                          : init_apic_ldr == init_apic_ldr_flat ? "Flat"
>> +                                                                : 
>> "Clustered",
>> +           nr_ioapics);
>>      enable_apic_mode();
> 
> This also seem to be completely unneeded? I guess it would be cleaned
> in a further patch.

I have to admit I didn't even check. There's so much more cleanup to
do here ...

Jan