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: Samuel Thibault
Subject: Re: [PATCH] SMP initialization: detection and enumeration
Date: Tue, 11 Aug 2020 00:25:36 +0200
User-agent: NeoMutt/20170609 (1.8.3)

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 */



reply via email to

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