bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] SMP initialization: detection and enumeration


From: Almudena Garcia
Subject: Re: [PATCH] SMP initialization: detection and enumeration
Date: Tue, 28 Jul 2020 09:50:02 +0200

> 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
I agree. It's a mistake.

> Though I do
> question what the point of mach_ncpus is if it isn't being used to
> determine NCPUS.

Currently, mach_ncpus is used to define NCPUS. I keep It, because It can be useful to enable/disable the SMP support, and this patch is low-invasive, avoiding to broke more things.

> AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
This define can be removed, of course. I don't know because I keep It there.

>> +    return apic_id;

This line doesn't get the Kernel ID, but the APIC ID.
The APIC ID can be obtained from the common Local APIC address, which points to the Local APIC of the current CPU (if you access this address from cpu1, you get the APIC ID of cpu1).

The ACPI tables also stores the APIC ID of each CPU, so I enumerate the processors in an array using this. The array is indexed by Kernel ID (the logical ID), and stores the APIC ID for each cpu.



El mar., 28 jul. 2020 a las 2:19, Jessica Clarke (<jrtc27@jrtc27.com>) escribió:
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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]