Re: [PATCH] SMP initialization: detection and enumeration

[Almudena Garcia]

> ? git diff produces the same as format-patch, in terms of formatting
> mistakes... The disadvantage of git diff is that your patches then mix
> things altogether, see the additions to Makefrag.am. I just cannot apply

> the series as it is.

Yes, The problem is that I didn't write each file in a single commit. Then, I have to split the changes in different patches on my own.

This is the reason because the additions to Makefrag.am are not in a exact order, because I had to split the changes file to file.

> ? The whole point of the mach_ncpus variable is to contain the number
> of processors. I don't see why we could want mach_ncpus to be defined to
> a different value than NCPUS.

As I explained in a previous mail, in Mach source code the are many structures (arrays) which size is defined by NCPUS.

Added to this, all SMP special cases are controlled by preprocessor directives like "if NCPUS > 1". For this reason, removing NCPUS can be a very difficult task.

So, to avoid a big rewrite of the code, I simply use "mach_ncpus" as a switch, to enable or disable SMP support.

But, when its value is bigger than 1, I set NCPUS to the maximum number of cpus, to fix the size of the structures to a safety value.

Then, when SMP is enabled (mach_ncpus > 1), the arrays which store the information about cpus take 256 (NCPUS value) positions by default.

> Err, it's empty, so just drop the file.

I prefer to keep this file, to use that as an arch-independent interface for SMP.

> i386 file references go to i386/Makefrag.am, please.

Ok, I forgot this detail.

> Does it really need to be a separate function? Will we ever want to call
> it somewhere else than smp_init?  If not just fold it into smp_init, or
> make it static, there is no point in making it extern.

I put It in a separate function because It's possible that, in next steps, I will need to add more fields to the structure, which will need to be initialized together.

But I agree that this function might be st

> Is this function really useful?  I mean in the long run we will want a
> CPU number from 0, which will have to be knowing the apic enumeration,
> and thus that's probably acpi.c that will define it anyway, and that is
> the function whose declaration will belong to kernel/smp.h

The idea about this function is to know the number of cpus (this will be necessary to enable the cpus in next steps), without knowing the details about APIC.

In other terms, the SMP functions are a simple interface to manage and get info about SMP, without needing to know architecture internals (in this case, APIC).

> > +volatile ApicLocalUnit* lapic = NULL;

> [...]

If, by any reason, we can't find APIC structures, o we need to have a secure value for lapic pointer, to take notice that APIC search or parsing has failed.

And I simply prefered set this value at start. When we find the APIC table, if the parser is successful, this pointer will take the real value of the common Local APIC memory address.

> That one belongs to kern/smp.h: non-i386 code will probably want to use
> it.

Yes, this was my idea. But It was very difficult to declare smp_info structure in kern/apic.c , and then share this structure with i386/i386/smp.c to initialize its data.

I had many compilation problems, so I had to put this function there to ease the compilation.

> ? Is this really useful to expose to the rest of the kernel? Isn't that
> structure something specific to the i386 SMP implementation?

Really, the smp_data structure might be generic for all architectures. The only reason because I declared smp_info in i386 files is because I didn't get to compile the code declaring it in kern/smp.c.

The entire smp pseudoclass might be a generic smp interface for any architecture.

> ? I don't think we want to print this as an error?

Is there a function to print messages as errors? If yes, then I replace this print with It.

Thanks for the review. I will try to fix the mistakes in my code.

El mar., 11 ago. 2020 a las 0:25, Samuel Thibault (<samuel.thibault@gnu.org>) escribió:

Hello,

Almudena Garcia, le lun. 10 août 2020 20:56:13 +0200, a ecrit:
> I attach a new version of my patch, fixing some errors and following the latest
> comments about this.

That looks nice overall!

> This time, I've generated the files manually from "git diff", instead using
> "git format-patch", so the patches could contain little format mistakes.

? git diff produces the same as format-patch, in terms of formatting
mistakes... The disadvantage of git diff is that your patches then mix
things altogether, see the additions to Makefrag.am. I just cannot apply
the series as it is.

Please try to attach patches in their order, so I don't have to reorder
while reviewing...

> From: Almudena Garcia <liberamenso10000@gmail.com>
> Date: Mon, 10 Aug 2020 19:52:44 +0200
> Subject: [PATCH 1/6] configfrag.ac: Define NCPUS to 256 if mach_ncpus > 1
>
> This allows to define the size of cpus structures to the maximum value allowed by xAPIC, keeping this size independant of mach_ncpus value

? The whole point of the mach_ncpus variable is to contain the number
of processors. I don't see why we could want mach_ncpus to be defined to
a different value than NCPUS.

> ---
> diff --git a/configfrag.ac b/configfrag.ac
> index 91d737ef..05c33a28 100644
> --- a/configfrag.ac
> +++ b/configfrag.ac
> @@ -46,9 +46,11 @@ AC_DEFINE([BOOTSTRAP_SYMBOLS], [0], [BOOTSTRAP_SYMBOLS])
>  # Multiprocessor support is still broken.
>  AH_TEMPLATE([MULTIPROCESSOR], [set things up for a uniprocessor])
>  mach_ncpus=1
> +
>  AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
>  [if [ $mach_ncpus -gt 1 ]; then]
>    AC_DEFINE([MULTIPROCESSOR], [1], [set things up for a multiprocessor])
> +  AC_DEFINE_UNQUOTED([NCPUS], [256], [number of CPUs])
>  [fi]
> 
>  # Restartable Atomic Sequences to get a really fast test-n-set.  Can't be


> From: Almudena Garcia <liberamenso10000@gmail.com>
> Date: Mon, 10 Aug 2020 19:54:02 +0200
> Subject: [PATCH 2/6] smp: Add pseudoclass to manage APIC operations
>

> diff --git a/Makefrag.am b/Makefrag.am
> index ea612275..03821d03 100644
> --- a/Makefrag.am
> +++ b/Makefrag.am
> @@ -122,6 +122,24 @@ EXTRA_DIST += \
>       ipc/notify.defs
> 
> 
> +#
> +# SMP implementation (APIC, ACPI, etc)
> +#
> +
> +     
> +libkernel_a_SOURCES += \
> +     i386/i386/apic.h \
> +     i386/i386/apic.c

i386 file references go to i386/Makefrag.am, please.

Also, no need to put them in a dedicated section, just file them in the
list of i386/i386/ files

Also, put them inside the list guarded by "if PLATFORM_at": acpi/apic do
not make sense on the Xen platform. You'll see the pic files referenced
there, so see that yes, apic definitely belongs there.


And similarly for the i386/i386at/acpi* files: file them into i386/Makefrag.am,
within the existing i386/i386at/ list. And same for i386/i386/smp*
files. kern/smp.h however belongs to the root Makefrag.am, since it's
arch-independent.

> +volatile ApicLocalUnit* lapic = NULL;

[...]

> +/* apic_get_lapic: returns a reference to the common memory address for Local APIC. */
> +ApicLocalUnit*
> +apic_get_lapic(void)
> +{
> +    return lapic;
> +}

Aren't you getting a warning here about the volatile qualifier?
Yes, apic_get_lapic should be made to return a volatile ApicLocalUnit*.
Always pay attention.

> +typedef struct ApicLocalUnit {
> +        /* 0x000 */
> +        ApicReg reserved0;
> +        /* 0x010 */
> +        ApicReg reserved1;

As I already said, please put the comments on the right, that will look
much better.

> +void*
> +kmem_map_aligned_table(
> +     phys_addr_t             phys_address,
> +     vm_size_t       size,
> +     int             mode)
> +{
> +     vm_offset_t virt_addr;
> +     kern_return_t ret;
> +     uintptr_t into_page = phys_address % PAGE_SIZE;
> +     uintptr_t nearest_page = (uintptr_t)trunc_page(phys_address);

Both of these need to be a phys_addr_t: uintptr_t is the size of a
native pointer only. On a 32bit arch, we can have 32bit pointers but
40bit physical addresses for instance.

> From: Almudena Garcia <liberamenso10000@gmail.com>
> Date: Mon, 10 Aug 2020 19:59:00 +0200
> Subject: [PATCH 5/6] smp: Add generic smp pseudoclass
>
> diff --git a/i386/i386/smp.c b/i386/i386/smp.c
> new file mode 100644
> index 00000000..9fbc1ca1
> --- /dev/null
> +++ b/i386/i386/smp.c

> +/*
> + * smp_data_init: initialize smp_data structure
> + * Must be called after smp_init(), once all APIC structures
> + * has been initialized
> + */
> +void smp_data_init(void)
> +{
> +    smp_info.num_cpus = apic_get_numcpus();
> +}

Does it really need to be a separate function? Will we ever want to call
it somewhere else than smp_init?  If not just fold it into smp_init, or
make it static, there is no point in making it extern.

> +/*
> + * smp_get_current_cpu: return the hardware identifier (APIC ID in x86)
> + * of current CPU
> + */
> +int smp_get_current_cpu(void)
> +{
> +    return apic_get_current_cpu();
> +}

Is this function really useful?  I mean in the long run we will want a
CPU number from 0, which will have to be knowing the apic enumeration,
and thus that's probably acpi.c that will define it anyway, and that is
the function whose declaration will belong to kernel/smp.h

> diff --git a/i386/i386/smp.h b/i386/i386/smp.h
> new file mode 100644
> index 00000000..97684335
> --- /dev/null
> +++ b/i386/i386/smp.h

> +int smp_get_numcpus(void);

That one belongs to kern/smp.h: non-i386 code will probably want to use
it.

> diff --git a/kern/smp.c b/kern/smp.c
> new file mode 100644
> index 00000000..0a684774
> --- /dev/null
> +++ b/kern/smp.c
> @@ -0,0 +1,26 @@
> +/* smp.c - Template for generic SMP controller for Mach.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   Written by Almudena Garcia Jurado-Centurion
> +
> +   This file is part of GNU Mach.
> +
> +   GNU Mach is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   GNU Mach is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111, USA. */
> +
> +#include <kern/smp.h>
> +#include <machine/smp.h>
> +

Err, it's empty, so just drop the file.

> diff --git a/kern/smp.h b/kern/smp.h
> new file mode 100644
> index 00000000..f837dab9
> --- /dev/null
> +++ b/kern/smp.h
> @@ -0,0 +1,24 @@
> +/* smp.h - Template for generic SMP controller for Mach. Header file
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   Written by Almudena Garcia Jurado-Centurion
> +
> +   This file is part of GNU Mach.
> +
> +   GNU Mach is free software; you can redistribute it and/or modify it
> +   under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   GNU Mach is distributed in the hope that it will be useful, but
> +   WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111, USA. */
> +
> +struct smp_data {
> +    int num_cpus;
> +};

? Is this really useful to expose to the rest of the kernel? Isn't that
structure something specific to the i386 SMP implementation?

> From: Almudena Garcia <liberamenso10000@gmail.com>
> Date: Mon, 10 Aug 2020 20:02:59 +0200
> Subject: [PATCH 6/6] model_dep.c: Add smp_init call
>
> @@ -170,6 +172,14 @@ void machine_init(void)
>       linux_init();
>  #endif
> 
> +#if NCPUS > 1
> +     int smp_success = smp_init();
> +
> +     if(smp_success != 0) {
> +             printf("Error: no SMP found");

? I don't think we want to print this as an error?

> +     }
> +#endif /* NCPUS > 1 */