Re: [PATCH] SMP initialization: detection and enumeration

[Jessica Clarke]

On 28 Jul 2020, at 00:44, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Almudena Garcia, le lun. 27 juil. 2020 21:15:10 +0200, a ecrit:
>> [if [ $mach_ncpus -gt 1 ]; then]
>>   AC_DEFINE([MULTIPROCESSOR], [1], [set things up for a multiprocessor])
>> +  AC_DEFINE_UNQUOTED([NCPUS], [255], [number of CPUs])
> 
> Perhaps useless to define to so many by default?

That's fairly normal, having some static per-CPU data structures with
an upper limit on the number of CPUs supported. 256 is a common default
(I know that's what FreeBSD uses for x86) as the APIC IDs are 8-bit,
and I think this should be 256 rather than 255 too. Though I do
question what the point of mach_ncpus is if it isn't being used to
determine NCPUS. Either always do `AC_DEFINE_UNQUOTED([NCPUS], [255],
[number of CPUs])` (in which case you don't need the extra define) or
make it mach_smp instead. Also, I personally don't like having two
"calls" to AC_DEFINE_UNQUOTED; I know it's autoconf and m4 and all that
so calls aren't always quite what you think, but it still just looks
weird and confusing.

>> +/*
>> + * apic_get_current_cpu: returns the apic_id of current cpu.
>> + */
>> +uint16_t
>> +apic_get_current_cpu(void)
>> +{
>> +    uint16_t apic_id;
>> +
>> +    if(lapic == NULL)

You have a bunch of statements like this with missing whitespace.

>> +    return apic_id;
> 
> I'm wondering: is it really *that* simple to get the current cpu number,
> just read a memory location?  I'm surprised that this would provide
> different results on different cpus.

No, it's not. They can be non-sequential, and there's no guarantee that
the BSP's is even 0, but there'll be a bunch of simple chips where both
are true (especially in emulators etc). Normally the current CPU's ID
would be stored in per-CPU data, and each AP is told (or knows how to
look up based on its hardware ID) its logical ID when being brought
online.

Jess