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 <jbeul...@suse.com>
>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
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
