Re: [PATCH] SMP initialization: detection and enumeration

[Jessica Clarke]

On 19 Jul 2020, at 17:44, Almudena Garcia <liberame...@gmail.com> wrote:
>
> Hi all:
>
> I attach a patch, with the code to find the cpus and enumerate them, reading from ACPI tables and MADT (APIC) tables.
>
> I've tested it over Qemu, but I recommends to test It before committing, anyway.
>
> You can find the rest of the work in my GitHub repository
> https://github.com/AlmuHS/GNUMach_SMP/tree/smp-new
>
> Check this, and advice me about errors or other necessary fixes

Before posting patches, please learn to go through and check you've
conformed to the code style, i.e. whitespace, comment styles etc. I see
a huge number of cases where those are not adhered to in your patch at a
glance. Having someone else point out all the issues is a huge waste of
time; better to fix them all yourself and then ask someone to review it
when they're not going to be constantly having to point out sloppiness.

Jess

[Almudena Garcia]

Ok. I'll check the coding style. I'm trying to follow GNU style, but maybe I missed It in some files.

[jbr...@dismail.de]

Hey Almudena!

I quite admire your determination to work on the Hurd SMP! I would be terrified to try to work on something crazy cool like that. But as a friend told me recently, "No one cares about your theories and thoughts. People care about your actions. There were nights when I was doing a stand up comedy routine and failing miserably. BUT I KEPT AT IT! I learned though my mistakes. Courageous people act. They act often."

Thanks for being a role model for me to follow!

Joshua

[Jessica Clarke]

On 19 Jul 2020, at 18:06, Almudena Garcia <liberame...@gmail.com> wrote:
>
> I fixed tabulations and comments style. Now better?

Not hugely. Go read files in the same directory and see if they look
like what you've written.

Jess

> El dom., 19 jul. 2020 a las 18:49, Jessica Clarke (<jrt...@jrtc27.com>) escribió:
> On 19 Jul 2020, at 17:44, Almudena Garcia <liberame...@gmail.com> wrote:
> >
> > Hi all:
> >
> > I attach a patch, with the code to find the cpus and enumerate them, reading from ACPI tables and MADT (APIC) tables.
> >
> > I've tested it over Qemu, but I recommends to test It before committing, anyway.
> >
> > You can find the rest of the work in my GitHub repository
> > https://github.com/AlmuHS/GNUMach_SMP/tree/smp-new
> >
> > Check this, and advice me about errors or other necessary fixes
>
> Before posting patches, please learn to go through and check you've
> conformed to the code style, i.e. whitespace, comment styles etc. I see
> a huge number of cases where those are not adhered to in your patch at a
> glance. Having someone else point out all the issues is a huge waste of
> time; better to fix them all yourself and then ask someone to review it
> when they're not going to be constantly having to point out sloppiness.
>
> Jess
>

> <smp.patch>

[Almudena Garcia]

Thanks. I'm grateful for your support of my work.

It's a difficult task, and I'm very noob yet. But I'm trying to advance until my knowledge allows.

The Hurd devs support is very important to learn the necessary to get this objective.

El dom., 19 jul. 2020 a las 19:01, <jbr...@dismail.de> escribió:

Hey Almudena!

I quite admire your determination to work on the Hurd SMP! I would be terrified to try to work on something crazy cool like that. But as a friend told me recently, "No one cares about your theories and thoughts. People care about your actions. There were nights when I was doing a stand up comedy routine and failing miserably. BUT I KEPT AT IT! I learned though my mistakes. Courageous people act. They act often."

Thanks for being a role model for me to follow!

Joshua

July 19, 2020 12:54 PM, "Almudena Garcia" <liberame...@gmail.com> wrote:

Ok. I'll check the coding style. I'm trying to follow GNU style, but maybe I missed It in some files.

[Almudena Garcia]

There are many files with different code styles. I'm confused about what is the correct style.

El dom., 19 jul. 2020 a las 19:10, Jessica Clarke (<jrt...@jrtc27.com>) escribió:

On 19 Jul 2020, at 18:06, Almudena Garcia <liberame...@gmail.com> wrote:
>
> I fixed tabulations and comments style. Now better?

Not hugely. Go read files in the same directory and see if they look
like what you've written.

Jess

> El dom., 19 jul. 2020 a las 18:49, Jessica Clarke (<jrt...@jrtc27.com>) escribió:
> On 19 Jul 2020, at 17:44, Almudena Garcia <liberame...@gmail.com> wrote:
> >
> > Hi all:
> >
> > I attach a patch, with the code to find the cpus and enumerate them, reading from ACPI tables and MADT (APIC) tables.
> >
> > I've tested it over Qemu, but I recommends to test It before committing, anyway.
> >
> > You can find the rest of the work in my GitHub repository
> > https://github.com/AlmuHS/GNUMach_SMP/tree/smp-new
> >
> > Check this, and advice me about errors or other necessary fixes
>
> Before posting patches, please learn to go through and check you've
> conformed to the code style, i.e. whitespace, comment styles etc. I see
> a huge number of cases where those are not adhered to in your patch at a
> glance. Having someone else point out all the issues is a huge waste of
> time; better to fix them all yourself and then ask someone to review it
> when they're not going to be constantly having to point out sloppiness.
>
> Jess
>

> <smp.patch>

[Ricardo Wurmus]

Hi,

for any patch it’s best to not just show a single large diff but to
split the changes into logically related commits. You’re probably
working with Git, so the unit that we’re working with is a Git commit.

You should group related changes and commit them together. The commit
message should describe the changes in a GNU-style ChangeLog format; you
may also add additional descriptions. Here’s an example:

--8<---------------cut here---------------start------------->8---
kern: Frobnicate the jabberwocky.

In order to frobnicate the jabberwocky without confusion we only add the
core functionality here.

* kern/smp.c, kern/smp.h: New files.
* Makefrag.am (libkernel_a_SOURCES): Add them.
--8<---------------cut here---------------end--------------->8---

To commit only some changes and not others you can select lines of
interest with “git add -p” (or similar). Once all connected changes
have been staged you can commit them. Do this repeatedly until you have
a series of commits that are all small enough that a reviewer can
understand them (and thus your thinking) at a glance.

You can then turn that series of commits into a series of patches with
“git format-patch”. For example, “git format-patch -10” will generate
10 patch files from the last 10 commits. You can attach these patches
to an email, or if you have configured “git send-email” correctly you
could send them directly via email to this list. A reviewer can then
comment on each commit individually and apply them one by one if they
pass muster.

(This process is similar for most GNU packages.)

Hope this helps!

--
Ricardo

[Almudena Garcia]

Thanks for your explanation:

> To commit only some changes and not others you can select lines of
> interest with “git add -p” (or similar).  Once all connected changes
> have been staged you can commit them.  Do this repeatedly until you have
> a series of commits that are all small enough that a reviewer can
> understand them (and thus your thinking) at a glance.

I have already a commit list pushed in my GitHub repository. You can see It here: https://github.com/AlmuHS/GNUMach_SMP/commits/smp-new

But, in this case, my code is almost written from scratch, so It's complex to filter line by line.

The code only makes sense in a single piece. Otherway, the code doesn't compile or does nothing.

> You can then turn that series of commits into a series of patches with
> “git format-patch”.  For example, “git format-patch -10” will generate
> 10 patch files from the last 10 commits.

Ok, I'll try this. But there are so many commits.

[Almudena Garcia]

Anyway, my patch is short. Maybe I can split It manually, taking care about dependencies between blocks.

[Jessica Clarke]

On 19 Jul 2020, at 19:46, Almudena Garcia <liberame...@gmail.com> wrote:
>
> > for any patch it’s best to not just show a single large diff but to
> > split the changes into logically related commits

> I've just split the patch. I enumerated them following the dependencies order.

>
> > The commit
> > message should describe the changes in a GNU-style ChangeLog format; you
> > may also add additional descriptions.
>

> Where can I add this information? In the same patch or in a log file?

In the commit message. Then use git format-patch to create the patch
files, not git diff/show. Personally I also find it easier when people
use git send-email as then the patches are inline rather than as
attachments, it makes it easier to reply and annotate.

Jess

[Almudena Garcia]

ok. I had splitted It manually. Now I'll try again this way.

[Almudena Garcia]

But I've already committed these changes in my repository. How can I recommit them?

Added to this, I need to generate the patch using the "master" branch (which points to gnumach's upstream) to compare with the mine.

How can I solve this?

[Jessica Clarke]

On 19 Jul 2020, at 20:07, Almudena Garcia <liberame...@gmail.com> wrote:
>
> But I've already committed these changes in my repository. How can I recommit them?
> Added to this, I need to generate the patch using the "master" branch (which points to gnumach's upstream) to compare with the mine.
>
> How can I solve this?

Look up git rebase.

[Ricardo Wurmus]

Almudena Garcia <liberame...@gmail.com> writes:

> But I've already committed these changes in my repository. How can I
> recommit them?

You can change commits with “git rebase”. Use “git rebase -i” for
interactive rebasing, which gives you a list of commits to which you can
apply changes. If you want to change all commits and their contents you
might also just reset to a commit before your changes and then re-stage
changes as needed from your worktree.

> Added to this, I need to generate the patch using the "master" branch
> (which points to gnumach's upstream) to compare with the mine.

You can rebase your changes on top of the master branch of the gnumach
repository. Add the gnumach repository as a remote with “git remote add
upstream https://…” and then do “git fetch upstream; git rebase
upstream/master” to rebase your commits on top of that remote’s master
branch.

--
Ricardo

[Samuel Thibault]

Note: before doing rebases etc. I'd advise to push your current tree to
a separate branch that you can always restart from, to avoid risking
losing everything. There are ways to restore things, but it's way easier
to just keep a branch on the side where you know your changes are safe.

Samuel

[Almudena Garcia]

At first, I'm doing manually, without rebase. My commit history is very dirty, and the commit squash is complicated.

I have a clone of the "master" branch from upstream gnumach, and I'm creating a new branch derived from It, in which I'm adding manually the latest changes to commit.

I'll late a bit

[Joshua Branson]

I also find it really helpful to use magit. Emacs' plugin to help me do
git things.

This video is a short introduction for it:

https://www.youtube.com/watch?v=mtliRYQd0j4

--
Joshua Branson
Sent from Emacs and Gnus

[Samuel Thibault]

Hello,

Thanks for your work, we're getting closer!

Almudena Garcia, le lun. 27 juil. 2020 21:15:10 +0200, a ecrit:
> @@ -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");
> + }
> +#endif /* NCPUS > 1 */
> +

There's a bogus indent here. Tell your editor to show tab vs spaces to
avoid introducing incoherent combinations.

> Add smp.c, smp.h, apic.c, apic.h, acpi_parse_apic.c and acpi_parse_apic,h to compilation list
> ---
> Makefrag.am | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Makefrag.am b/Makefrag.am
> index ea612275..69ac31a1 100644
> --- a/Makefrag.am
> +++ b/Makefrag.am
> @@ -122,6 +122,18 @@ EXTRA_DIST += \
> ipc/notify.defs
>
>
> +#
> +# SMP implementation (APIC, ACPI, etc)
> +#
> +
> +libkernel_a_SOURCES += \
> + i386/i386at/acpi_parse_apic.h \
> + i386/i386at/acpi_parse_apic.c \
> + i386/i386/apic.h \
> + i386/i386/apic.c \
> + kern/smp.h \
> + kern/smp.c

Rather fold this into the commits that add these files. Otherwise the
code is not even getting compiled when checking out the other commits.

> --- /dev/null
> +++ b/kern/smp.c
> @@ -0,0 +1,72 @@

> +#ifdef __i386__
> +#include <i386/i386/apic.h>
> +#include <i386/i386at/acpi_parse_apic.h>

We avoid putting arch-specific code in kern/, and rather put it in
i386/i386. Note that you can have smp.h in i386/i386, and make other
files include <machine/smp.h>

> diff --git a/i386/i386at/acpi_parse_apic.c b/i386/i386at/acpi_parse_apic.c
> new file mode 100644
> index 00000000..4b579c5d
> --- /dev/null
> +++ b/i386/i386at/acpi_parse_apic.c
> @@ -0,0 +1,567 @@
> +/*
> + * Copyright 2018 Juan Bosco Garcia

Juan didn't assign copyright to the FSF. We usually do this, so we don't
have long-term licensing concerns. Could you send his email address so
we can send him the paper work? Alternatively the file could be put
under a BSD license, which poses way less concerns.

> + * Copyright 2018 2019 2020 Almudena Garcia Jurado-Centurion
> + * This file is part of Min_SMP.
> + * Min_SMP 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 of the License, or
> + * (at your option) any later version.
> + * Min_SMP 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 Min_SMP. If not, see <http://www.gnu.org/licenses/>.
> + */

Please use the same formatting as other GPL-licensed files. You did well
in the smp.[ch] files for instance.

> +#include <i386at/acpi_parse_apic.h>
> +#include <string.h> //memcmp, memcpy...
> +
> +#include <i386/apic.h> //lapic, ioapic...
> +#include <kern/printf.h> //printf
> +#include <include/stdint.h> //uint16_t, uint32_t...
> +#include <mach/machine.h> //machine_slot
> +#include <i386/vm_param.h> //phystokv
> +#include <kern/debug.h>
> +#include <vm/vm_kern.h>

We usually group includes by directory, see how other files do it.

> +#define BAD_CHECKSUM -1
> +#define NO_RSDP -2
> +#define NO_RSDT -2
> +#define BAD_SIGNATURE -3
> +#define NO_APIC -4

Rather use an enum, which you will then have to put in the .h file,
which makes sense anyway. Prepend the names with ACPI_, and add an
ACPI_SUCCESS equal to 0, so you can write return ACPI_SUCCESS; instead
of return 0;

> +struct acpi_apic *apic_madt = NULL;

Make it static?

> +static struct acpi_rsdp* acpi_get_rsdp(void);
> +static int acpi_check_rsdt(struct acpi_rsdt *);
> +static struct acpi_rsdt* acpi_get_rsdt(struct acpi_rsdp *rsdp, int* acpi_rsdt_n);
> +static struct acpi_apic* acpi_get_apic(struct acpi_rsdt *rsdt, int acpi_rsdt_n);
> +static int acpi_apic_setup(struct acpi_apic *apic);
> +static int acpi_apic_add_ioapic(struct acpi_apic_ioapic *ioapic_entry);
> +static int acpi_apic_add_lapic(struct acpi_apic_lapic *lapic_entry);

These forward declarations don't seem actually needed: you can just put
acpi_apic_init at the end of the file.

> +static int acpi_apic_parse_table(struct acpi_apic *apic);

This doesn't exist?

> +static int
> +acpi_checksum(void *addr, uint32_t length)
> +{
> + char *bytes = addr;
> + int checksum = 0;
> + unsigned int i;
> +
> + /* Sum all bytes of addr */
> + for (i = 0; i < length; i++)
> + checksum += bytes[i];
> +
> + return checksum & 0xff;
> +}

Rather use uint8_t everywhere here, even for the returned value. You
won't even need to use & 0xff then.

> +static int
> +acpi_check_rsdp(struct acpi_rsdp *rsdp)
> +{
> + uint32_t checksum;
> + int is_rsdp;
> +
> + int ret_value = 0;
> +
> + /* Check the integrity of RSDP. */
> + if (rsdp == NULL)
> + ret_value = NO_RSDP;

Simply rather "return NO_RSDP;"? That'll even avoid the "else".

> + else {
> + /* Check if rsdp signature match with the ACPI RSDP signature. */
> + is_rsdp = memcmp(rsdp->signature, ACPI_RSDP_SIG, sizeof(rsdp->signature));
> +
> + if (is_rsdp == 0) {

Invert the test, so you can just return, and avoid the else.

> + /* If match, calculates rdsp checksum and check It. */
> + checksum = acpi_checksum(rsdp, sizeof(struct acpi_rsdp));
> +
> + if (checksum != 0)
> + ret_value = BAD_CHECKSUM;

Ditto

> + }
> + else
> + ret_value = BAD_SIGNATURE;

Ditto

> + }
> +
> + return ret_value;

And then you can just return ACPI_SUCCESS instead of having a ret_value
variable.

> +static struct acpi_rsdp*
> +acpi_search_rsdp(void *addr, uint32_t length)
> +{
> + struct acpi_rsdp *rsdp = NULL;

You can move this to the block where it is actually used.

> + void *end;
> +
> + /* check alignment. */
> + if ( (uint32_t)addr & (ACPI_RSDP_ALIGN-1) )
> + return NULL;
> +
> + /* Search RDSP in memory space between addr and addr+lenght. */
> + for (end = addr+length; addr < end; addr += ACPI_RSDP_ALIGN) {
> +
> + /* Check if the current memory block stores the RDSP. */
> + if (acpi_check_rsdp(addr) == 0) {
> + /* If yes, store RSDP address */
> + rsdp = (struct acpi_rsdp*) addr;

You can just return it.

> +static int
> +acpi_check_rsdt(struct acpi_rsdt *rsdt)
> +{
> + uint32_t checksum;
> +
> + int ret_value = 0;
> +
> + if (rsdt == NULL)
> + ret_value = NO_RSDT;

Ditto, again avoiding the else.

> + else {
> + checksum = acpi_checksum(rsdt, rsdt->header.length);
> +
> + if (checksum != 0)
> + ret_value = BAD_CHECKSUM;

Ditto.

> + }
> +
> + return ret_value;

Ditto.

> +static struct acpi_apic*
> +acpi_get_apic(struct acpi_rsdt *rsdt, int acpi_rsdt_n)
> +{
> + struct acpi_apic *apic = NULL;
> + struct acpi_dhdr *descr_header;
> + int check_signature;

Move them to the blocks where they are used.

> + int i;
> +
> + if (rsdt != NULL) {

Invert the if to just return, and you'll avoid one level of indentation.

> + /* Search APIC entries in rsdt table. */
> + for (i = 0; i < acpi_rsdt_n; i++) {
> + descr_header = (struct acpi_dhdr*) kmem_map_aligned_table(rsdt->entry[i], sizeof(struct acpi_dhdr), VM_PROT_READ);
> +
> + /* Check if the entry contains an APIC. */
> + check_signature = memcmp(descr_header->signature, ACPI_APIC_SIG, sizeof(descr_header->signature));
> +
> + if (check_signature == 0) {
> + /* If yes, store the entry in apic. */
> + apic = (struct acpi_apic*) kmem_map_aligned_table(rsdt->entry[i], sizeof(struct acpi_apic), VM_PROT_READ | VM_PROT_WRITE);
> +
> + printf("found apic in address %x\n", apic);

That's perhaps too verbose for production?

> + break;

You can just return.

> + }
> + }
> + }
> +
> + return apic;

And here return NULL instead.

> +static int
> +acpi_apic_add_lapic(struct acpi_apic_lapic *lapic_entry)
> +{
> + int ret_value = 0;
> + int lapic_id;
> +
> + if (lapic_entry == NULL)
> + ret_value = -1;

Ditto, again avoiding the else.

> + else {
> + /* If cpu flag is correct */
> + if (lapic_entry->flags & 0x1) {

You can also invert the if here.

> + /* Get the CPU's APIC ID. */
> + lapic_id = lapic_entry->apic_id;
> +
> + /* Add cpu to processors' list. */
> + apic_add_cpu(lapic_id);
> +
> + printf("new cpu found with apic id %x\n", lapic_entry->apic_id);;
> + }
> + }
> +
> + return ret_value;

Ditto.

> +static int
> +acpi_apic_add_ioapic(struct acpi_apic_ioapic *ioapic_entry)
> +{
> + int ret_value = 0;
> + IoApicData io_apic;
> +
> + if (ioapic_entry == NULL)
> + ret_value = -1;

Ditto, again avoiding the else.

> + else {
> + /* Fill IOAPIC structure with its main fields */
> + io_apic.apic_id = ioapic_entry->apic_id;
> + io_apic.addr = ioapic_entry->addr;
> + io_apic.base = ioapic_entry->base;
> +
> + /* Insert IOAPIC in the list. */
> + apic_add_ioapic(io_apic);
> +
> + printf("new ioapic found with apic id %x\n", io_apic.apic_id);
> + }
> +
> + return ret_value;

Ditto.

> +static int
> +acpi_apic_add_irq_override(struct acpi_apic_irq_override* irq_override)
> +{
> + int ret_value = 0;
> + IrqOverrideData irq_over;
> +
> + if (irq_override == NULL)
> + ret_value = -1;

Ditto.

> + else {
> + /* Fills IRQ override structure with its fields */
> + irq_over.bus = irq_override->bus;
> + irq_over.irq = irq_override->irq;
> + irq_over.gsi = irq_override->gsi;
> + irq_over.flags = irq_override->flags;
> +
> + /* Insert IRQ override in the list */
> + apic_add_irq_override(irq_over);
> + }
> +
> + return ret_value;

Ditto.

> +static int
> +apic_parse_table(struct acpi_apic *apic)
> +{
> + int ret_value = 0;
> + struct acpi_apic_dhdr *apic_entry = NULL;
> + uint32_t end = 0;

You can make end a "struct acpi_apic_dhdr *" as well.

> + if (apic != NULL) {

Ditto, avoiding indentation.

> + /* Get the address of first APIC entry */
> + apic_entry = apic->entry;
> +
> + /* Get the end address of APIC table */
> + end = (uint32_t) apic + apic->header.length;

You indeed need to cast apic into uint32_t before doing computations,
and cast back to "struct acpi_apic_dhdr *" just before assigning to end,
like you did for api_entry here:

> + /* Get next APIC entry. */
> + apic_entry = (struct acpi_apic_dhdr*)((uint32_t) apic_entry
> + + apic_entry->length);
> + }
> + }

> +static int
> +acpi_apic_setup(struct acpi_apic *apic)
> +{
> + int apic_checksum;
> + int ret_value = 0;
> + ApicLocalUnit* lapic;
> + int ncpus, nioapics;
> + int init_success = 0;
> +
> +
> + if (apic != NULL) {

Ditto, avoiding indentation.

> + /* Check the checksum of the APIC */
> + apic_checksum = acpi_checksum(apic, apic->header.length);
> +
> + if(apic_checksum != 0)
> + ret_value = BAD_CHECKSUM;

Ditto.

> + else {
> + init_success = apic_data_init();
> +
> + if (init_success == 0) {

Ditto.

> + printf("lapic found in address %x\n", apic->lapic_addr);
> +
> + /* map common lapic address */
> + lapic = kmem_map_aligned_table(apic->lapic_addr, sizeof(ApicLocalUnit), VM_PROT_READ);
> +
> + if (lapic != NULL) {
> + printf("lapic mapped in address %x\n", lapic);

That's too verbose for production?

> + apic_lapic_init(lapic);
> + }
> +
> + apic_parse_table(apic);
> +
> + ncpus = apic_get_numcpus();
> + nioapics = apic_get_num_ioapics();
> +
> + if(ncpus == 0 || nioapics == 0)
> + ret_value = -1;

Ditto.

> diff --git a/i386/i386at/acpi_parse_apic.h b/i386/i386at/acpi_parse_apic.h
> new file mode 100644
> index 00000000..486ad7b2
> --- /dev/null
> +++ b/i386/i386at/acpi_parse_apic.h
> @@ -0,0 +1,144 @@
> +/*
> + * Copyright 2018 Juan Bosco Garcia
> + * This file is part of Min_SMP.
> + * Min_SMP 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 of the License, or
> + * (at your option) any later version.
> + * Min_SMP 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 Min_SMP. If not, see <http://www.gnu.org/licenses/>.
> + */

Ditto.

> diff --git a/vm/vm_kern.c b/vm/vm_kern.c
> index 2e333ee1..cb3955de 100644
> --- a/vm/vm_kern.c
> +++ b/vm/vm_kern.c
> @@ -66,14 +66,14 @@ vm_map_t kernel_pageable_map;
> /*
> * projected_buffer_allocate
> *
> - * Allocate a wired-down buffer shared between kernel and user task.
> + * Allocate a wired-down buffer shared between kernel and user task.
> * Fresh, zero-filled memory is allocated.
> * If persistence is false, this buffer can only be deallocated from
> - * user task using projected_buffer_deallocate, and deallocation
> + * user task using projected_buffer_deallocate, and deallocation
> * from user task also deallocates the buffer from the kernel map.
> * projected_buffer_collect is called from vm_map_deallocate to
> * automatically deallocate projected buffers on task_deallocate.
> - * Sharing with more than one user task is achieved by using
> + * Sharing with more than one user task is achieved by using
> * projected_buffer_map for the second and subsequent tasks.
> * The user is precluded from manipulating the VM entry of this buffer
> * (i.e. changing protection, inheritance or machine attributes).

These are unrelated changes. Yes, I know some editors introduce such
"cleanups", but in the end in the git history they become noise, so
please revert these parts. You can use "patch -p1 -R" to easily apply
the patch in reverse mode.

> @@ -635,6 +635,45 @@ retry:
> return KERN_SUCCESS;
> }
>
> +/*
> + * kmem_map_aligned_table: map a table or structure in a virtual memory page
> + * Align the table initial address with the page initial address.
> + *
> + * Parameters:
> + * phys_address: physical address, the start address of the table.
> + * size: size of the table.
> + * mode: access mode. VM_PROT_READ for read, VM_PROT_WRITE for write.
> + *
> + * Returns a reference to the virtual address if success, NULL if failure.
> + */
> +
> +void*
> +kmem_map_aligned_table(
> + unsigned long phys_address,
> + unsigned long size,

Use proper types: phys_addr_t and vm_size_t (see the functions you are
using, they are taking that, not unsigned long).

> + 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);

Ditto.

> @@ -55,6 +55,8 @@ extern kern_return_t kmem_alloc_pageable(vm_map_t, vm_offset_t *,
> extern kern_return_t kmem_valloc(vm_map_t, vm_offset_t *, vm_size_t);
> extern kern_return_t kmem_alloc_wired(vm_map_t, vm_offset_t *, vm_size_t);
> extern kern_return_t kmem_alloc_aligned(vm_map_t, vm_offset_t *, vm_size_t);
> +extern void* kmem_map_aligned_table(unsigned long phys_address, unsigned long size, int mode);
> +
> extern void kmem_free(vm_map_t, vm_offset_t, vm_size_t);
>
> extern void kmem_submap(vm_map_t, vm_map_t, vm_offset_t *,

Please try to stick with identation existing around.

> @@ -46,9 +46,12 @@ 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])
> +
> AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])

Err, this seems spurious?

> [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?

> --- /dev/null
> +++ b/i386/i386/apic.c
> +apic_data_init(void)
> +{
> + int success = 0;
> +
> + apic_data.cpu_lapic_list = NULL;
> + apic_data.ncpus = 0;
> + apic_data.nioapics = 0;
> + apic_data.nirqoverride = 0;
> +
> + /* Reserve the vector memory for the maximum number of processors. */
> + apic_data.cpu_lapic_list = (uint16_t*) kalloc(MAX_CPUS*sizeof(uint16_t));
> +
> + /* If the memory reserve fails, return -1 to advice about the error. */
> + if (apic_data.cpu_lapic_list == NULL)
> + success = -1;

Ditto.

> +void
> +apic_add_cpu(uint16_t apic_id)
> +{
> + int numcpus = apic_data.ncpus;
> + apic_data.cpu_lapic_list[numcpus] = apic_id;
> + apic_data.ncpus++;

Rather simply write it

apic_data.cpu_lapic_list[apic_data.ncpus++] = apic_id;

> +apic_add_ioapic(IoApicData ioapic)
> +{
> + int nioapic = apic_data.nioapics;
> +
> + apic_data.ioapic_list[nioapic] = ioapic;
> + apic_data.nioapics++;

Ditto.

> +void
> +apic_add_irq_override(IrqOverrideData irq_over)
> +{
> + int nirq = apic_data.nirqoverride;
> +
> + apic_data.irq_override_list[nirq] = irq_over;
> + apic_data.nirqoverride++;

Ditto.

> +uint16_t
> +apic_get_cpu_apic_id(int kernel_id)
> +{
> + uint16_t apic_id;
> +
> + if (kernel_id < MAX_CPUS)
> + apic_id = apic_data.cpu_lapic_list[kernel_id];
> + else
> + apic_id = -1;

Simply return rather than using a variable.

> +/*
> + * apic_get_ioapic: returns the IOAPIC identified by its kernel ID.
> + * Receives as input the IOAPIC's Kernel ID.
> + * Returns a ioapic_data structure with the IOAPIC's data.
> + */
> +struct IoApicData
> +apic_get_ioapic(int kernel_id)
> +{
> + IoApicData io_apic;
> +
> + if (kernel_id < MAX_IOAPICS)
> + io_apic = apic_data.ioapic_list[kernel_id];

Ditto, and notice that io_apic would be undefined otherwise. Pay
attention to compiler warnings.

> + return io_apic;

> +/*
> + * 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)
> + apic_id = 0;
> + else
> + apic_id = lapic->apic_id.r;

Ditto.

> + 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.

> +int apic_refit_cpulist(void)
> +{
> + uint16_t* old_list = apic_data.cpu_lapic_list;
> + uint16_t* new_list = (uint16_t*) kalloc(apic_data.ncpus*sizeof(uint16_t));
> + int i = 0;
> + int success = 0;
> +
> + if (new_list != NULL && old_list != NULL) {

Ditto, invert the if to avoid indentation. Also notice the memory leak
in that case, you'll want to make the kalloc only in the success case.

> + for (i = 0; i < apic_data.ncpus; i++)
> + new_list[i] = old_list[i];
> +
> + apic_data.cpu_lapic_list = new_list;
> + kfree(old_list);

> diff --git a/i386/i386/apic.h b/i386/i386/apic.h
> new file mode 100644
> index 00000000..fd5e830e
> --- /dev/null
> +++ b/i386/i386/apic.h
> @@ -0,0 +1,174 @@
> +/*
> + * Copyright (c) 1994 The University of Utah and
> + * the Computer Systems Laboratory at the University of Utah (CSL).
> + * All rights reserved.
> + *
> + * Permission to use, copy, modify and distribute this software is hereby
> + * granted provided that (1) source code retains these copyright, permission,
> + * and disclaimer notices, and (2) redistributions including binaries
> + * reproduce the notices in supporting documentation, and (3) all advertising
> + * materials mentioning features or use of this software display the following
> + * acknowledgement: ``This product includes software developed by the
> + * Computer Systems Laboratory at the University of Utah.''
> + *
> + * THE UNIVERSITY OF UTAH AND CSL ALLOW FREE USE OF THIS SOFTWARE IN ITS "AS
> + * IS" CONDITION. THE UNIVERSITY OF UTAH AND CSL DISCLAIM ANY LIABILITY OF
> + * ANY KIND FOR ANY DAMAGES WHATSOEVER RESULTING FROM THE USE OF THIS SOFTWARE.
> + *
> + * CSL requests users of this software to return to csl-...@cs.utah.edu any
> + * improvements that they make and grant CSL redistribution rights.
> + *
> + * Author: Bryan Ford, University of Utah CSL

So you picked it up from somewhere?
Since it's a BSD license it is fine, just making sure that it is the
proper copyright notice.

> +typedef struct ApicLocalUnit {
> + /* 0x000 */
> + ApicReg reserved0;
> + /* 0x010 */
> + ApicReg reserved1;
> + /* 0x020 */
> + ApicReg apic_id;
> + /* 0x030 */
> + ApicReg version;
[...]

Rather put the comment on the right of the corresponding field, that will look much better.

Samuel

[Jessica Clarke]

On 28 Jul 2020, at 00:44, Samuel Thibault <samuel....@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

[Almudena Garcia]

> 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.

[Almudena Garcia]

> @@ -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");
> +    }
> +#endif /* NCPUS > 1 */
> +

> There's a bogus indent here. Tell your editor to show tab vs spaces to
> avoid introducing incoherent combinations.

Thanks. I'll fix It. I'm using CodeBlocks as editor, and this doesn't show the tabs properly.

> Rather fold this into the commits that add these files. Otherwise the
> code is not even getting compiled when checking out the other commits.

Ok, I'll add It.

> Juan didn't assign copyright to the FSF. We usually do this, so we don't
> have long-term licensing concerns. Could you send his email address so
> we can send him the paper work? Alternatively the file could be put
> under a BSD license, which poses way less concerns.

Ok. I will tell him to solve this.

> +#ifdef __i386__

> +#include <i386/i386/apic.h>
> +#include <i386/i386at/acpi_parse_apic.h>

> We avoid putting arch-specific code in kern/, and rather put it in
> i386/i386. Note that you can have smp.h in i386/i386, and make other
> files include <machine/smp.h>

Ok. Then I will fix It. I will add an i386/i386/smp.h to add these includes.

> > +static int acpi_apic_parse_table(struct acpi_apic *apic);

> This doesn't exist?

It's possible. Maybe I renamed this function, and I forget to rename its declaration (or the inverse). I think I will rename the definition: It's more coherent.

> @@ -46,9 +46,12 @@ 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])
> +

>  AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])

> Err, this seems spurious?

Yes, It's a mistake. You can remove the duplicated line.

> Simply return rather than using a variable.

I prefer using a temporary variable to ease debugging in case of error. Direct return can be difficult to debug in this case.

> You can just return it.

I prefer to avoid multiple return, to ease the reading. A single return can be easy to read.

> That's too verbose for production?

I can remove the prints, If necessary.

> So you picked it up from somewhere?
> Since it's a BSD license it is fine, just making sure that it is the
> proper copyright notice.

This file existed in older versions of gnumach. Thomas removed It in 2007. Simply, I recovered It and updated.
http://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/i386/imps/apic.h?id=f20666fc6b0471738829363e20c27f282f65dbf2

> Rather put the comment on the right of the corresponding field, that will look much better.

Ok, I'll fix It.

> Ditto, invert the if to avoid indentation. Also notice the memory leak
> in that case, you'll want to make the kalloc only in the success case.

> +        for (i = 0; i < apic_data.ncpus; i++)
> +            new_list[i] = old_list[i];
> +
> +        apic_data.cpu_lapic_list = new_list;
> +        kfree(old_list);

Thanks, I'll solve this.

> 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.

The APIC ID is stored in the Local APIC of each cpu. This address is common for all Local APIC: accessing this from each cpu, it shows the Local APIC of this cpu.

By example, if you access this address from cpu1, you can see the Local APIC of cpu1.

About the rest of the comments, I will try to fix it soon.

Thanks

El mar., 28 jul. 2020 a las 1:45, Samuel Thibault (<samuel....@gnu.org>) escribió:

Hello,

Thanks for your work, we're getting closer!

Almudena Garcia, le lun. 27 juil. 2020 21:15:10 +0200, a ecrit:

>  AH_TEMPLATE([MULTIPROCESSOR], [set things up for a uniprocessor])
>  mach_ncpus=1
> +AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
> +

>  AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])

Err, this seems spurious?

>  [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?

> +{
> +    uint16_t apic_id;
> +

> +    if (kernel_id < MAX_CPUS)

> +        apic_id = apic_data.cpu_lapic_list[kernel_id];
> +    else
> +        apic_id = -1;

Simply return rather than using a variable.

> +/*
> + * apic_get_ioapic: returns the IOAPIC identified by its kernel ID.
> + * Receives as input the IOAPIC's Kernel ID.
> + * Returns a ioapic_data structure with the IOAPIC's data.
> + */
> +struct IoApicData
> +apic_get_ioapic(int kernel_id)
> +{
> +    IoApicData io_apic;
> +
> +    if (kernel_id < MAX_IOAPICS)
> +        io_apic = apic_data.ioapic_list[kernel_id];

Ditto, and notice that io_apic would be undefined otherwise. Pay
attention to compiler warnings.

> +    return io_apic;

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

> +        apic_id = 0;
> +    else
> +        apic_id = lapic->apic_id.r;

Ditto.

> +    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.

[Samuel Thibault]

Almudena Garcia, le mar. 28 juil. 2020 10:27:55 +0200, a ecrit:

> > Simply return rather than using a variable.
> I prefer using a temporary variable to ease debugging in case of error. Direct
> return can be difficult to debug in this case.

That's alright while debugging. But before we commit the source code, we
need it to be in a way which is the most readable, so people can easily
work it out.

> > You can just return it.
> I prefer to avoid multiple return, to ease the reading. A single return can be
> easy to read.

? No. Introducing a variable, remembering that it holds a value, and
eventually returning it, is much more brain overhead than just directly
returning the value.

Also, having a few returns early in the function, to eliminate the error
cases and trivial cases, allows to get rid of that from the mind, and
focus on the real stuff for the rest of the function.

> > So you picked it up from somewhere?
> > Since it's a BSD license it is fine, just making sure that it is the
> > proper copyright notice.
>
> This file existed in older versions of gnumach. Thomas removed It in 2007.
> Simply, I recovered It and updated.

> [1]http://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/i386/imps/apic.h?id=
> f20666fc6b0471738829363e20c27f282f65dbf2

Oh, interesting, it dates back so long ago :)

> > 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.
>
> The APIC ID is stored in the Local APIC of each cpu. This address is common for
> all Local APIC: accessing this from each cpu, it shows the Local APIC of this
> cpu.
> By example, if you access this address from cpu1, you can see the Local APIC of
> cpu1.

So it's a special address whose accesses are trapped within the chip and
don't actually get out on the memory bus?

Samuel

[Almudena Garcia]

> That's alright while debugging. But before we commit the source code, we
> need it to be in a way which is the most readable, so people can easily
> work it out.

Ok, then I will modify my code this way.

> ? No. Introducing a variable, remembering that it holds a value, and
> eventually returning it, is much more brain overhead than just directly
> returning the value.

Same

Also, having a few returns early in the function, to eliminate the error
cases and trivial cases, allows to get rid of that from the mind, and
focus on the real stuff for the rest of the function.

> So it's a special address whose accesses are trapped within the chip and

> don't actually get out on the memory bus?

I don't know the technical details at all. In OSDev (https://wiki.osdev.org/APIC) It explains this:

The local APIC's registers are memory-mapped in physical page FEE00xxx (as seen in table 8-1 of Intel P4 SPG). This address is the same for each local APIC that exists in a configuration, meaning you are only able to directly access the registers of the local APIC of the core that your code is currently executing on.

The physical address is not always the same: each machine can map the Local APIC in a different address. By this reason, I search It in ACPI tables, in MADT (APIC) table.

Thanks for your review :) . This weekend I will try to fix the code.

[Richard Braun]

On Thu, Jul 30, 2020 at 10:44:40PM +0200, Samuel Thibault wrote:
> > > 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.
> >
> > The APIC ID is stored in the Local APIC of each cpu. This address is common for
> > all Local APIC: accessing this from each cpu, it shows the Local APIC of this
> > cpu.
> > By example, if you access this address from cpu1, you can see the Local APIC of
> > cpu1.
>
> So it's a special address whose accesses are trapped within the chip and
> don't actually get out on the memory bus?

It's physically memory mapped to the local APIC address space, but
because of that, it's also not optimal. All systems I know use a scheme
similar to TLS, i.e. using the fs or gs segment register, to fetch
a per-CPU structure and from it, per-CPU data. This avoids relying on
hardware running at a lower frequency than the CPU.

--
Richard Braun

[Samuel Thibault]

Almudena Garcia, le jeu. 30 juil. 2020 22:59:02 +0200, a ecrit:

> The local APIC's registers are memory-mapped in physical page FEE00xxx (as seen
> in table 8-1 of Intel P4 SPG). This address is the same for each local APIC
> that exists in a configuration, meaning you are only able to directly access
> the registers of the local APIC of the core that your code is currently
> executing on.

Ok, so it indeed seems so.

Samuel

[Jessica Clarke]

You need to do that anyway if you want any guarantees over _what_ the
IDs are (normally you want 0 for the BSP, 1 to N-1 for the APs).

Jess

[Richard Braun]

On Thu, Jul 30, 2020 at 10:09:01PM +0100, Jessica Clarke wrote:
> > It's physically memory mapped to the local APIC address space, but
> > because of that, it's also not optimal. All systems I know use a scheme
> > similar to TLS, i.e. using the fs or gs segment register, to fetch
> > a per-CPU structure and from it, per-CPU data. This avoids relying on
> > hardware running at a lower frequency than the CPU.
>
> You need to do that anyway if you want any guarantees over _what_ the
> IDs are (normally you want 0 for the BSP, 1 to N-1 for the APs).

Not really. You can build a CPU ID table on the BSP, and associate CPU IDs
with all local APIC IDs discovered, and use that look-up table later
to obtain a CPU ID from the local APIC ID of the CPU running the code.
It's just too slow compared to using a segment register.

--
Richard Braun

[Almudena Garcia]

> You can build a CPU ID table on the BSP, and associate CPU IDs
> with all local APIC IDs discovered, and use that look-up table later

> to obtain a CPU ID from the local APIC ID of the CPU running the code.

It's my idea, of course. I don't know what you refer to exactly with "using a segment register". How Does It Work?

Maybe, in next versions, I can improve the implementation using that.

[Jessica Clarke]

On 30 Jul 2020, at 22:12, Richard Braun <rbr...@sceen.net> wrote:
> On Thu, Jul 30, 2020 at 10:09:01PM +0100, Jessica Clarke wrote:
>>> It's physically memory mapped to the local APIC address space, but
>>> because of that, it's also not optimal. All systems I know use a scheme
>>> similar to TLS, i.e. using the fs or gs segment register, to fetch
>>> a per-CPU structure and from it, per-CPU data. This avoids relying on
>>> hardware running at a lower frequency than the CPU.
>>
>> You need to do that anyway if you want any guarantees over _what_ the
>> IDs are (normally you want 0 for the BSP, 1 to N-1 for the APs).
>
> Not really. You can build a CPU ID table on the BSP, and associate CPU IDs
> with all local APIC IDs discovered, and use that look-up table later
> to obtain a CPU ID from the local APIC ID of the CPU running the code.
> It's just too slow compared to using a segment register.

Well, yes, though you probably don't want to be using a fancy hash
table in such low-level code, so you'd either have an array of 2^16 CPU
IDs which is a bit absurd (or at least _used_ to be, these days it's
not that big) or just do some kind of search. I kind of ruled both out
from the get-go because they're too absurd, especially on other systems
where the APIC ID equivalent is 32-bit or 64-bit and you might have
hundreds of cores' IDs to search through to find your own.

Jess

[Samuel Thibault]

As Richard said, we just want to have different GDTs on the different
processors, so that we wan use the fs segment register to implement TLS
in the kernel and have per-cpu data cost essentially the same as global
data.

Samuel

[Samuel Thibault]

Samuel Thibault, le jeu. 30 juil. 2020 23:21:02 +0200, a ecrit:

(I don't mean it has to be implemented so for commiting to mainline,
just saying how we can optimize after it)

Samuel

[Almudena Garcia]

> Well, yes, though you probably don't want to be using a fancy hash
> table in such low-level code, so you'd either have an array of 2^16 CPU

> IDs which is a bit absurd.

The maximum number of cpus in xAPIC is 256, not 2^16. My array is indexed by Kernel ID.

My array is refitted to the exact number of cpus after enumerating them, to save space.

[Almudena Garcia]

> As Richard said, we just want to have different GDTs on the different
> processors, so that we wan use the fs segment register to implement TLS
> in the kernel and have per-cpu data cost essentially the same as global

> data.

Yes, but how can I store the relation APIC ID - Kernel ID in the GDT?

Is there any way to avoid using an array for that?

> (I don't mean it has to be implemented so for commiting to mainline,
> just saying how we can optimize after it)

I'm still learning yet, so many concepts are difficult for me.

My idea is to get a basic and functional SMP implementation, and then get help from more experienced developers to improve it.

[Samuel Thibault]

Almudena Garcia, le jeu. 30 juil. 2020 23:30:55 +0200, a ecrit:

> > As Richard said, we just want to have different GDTs on the different
> > processors, so that we wan use the fs segment register to implement TLS
> > in the kernel and have per-cpu data cost essentially the same as global
> > data.
>

> Yes, but how can I store the relation APIC ID - Kernel ID in the GDT?

You don't. Read about GDT and segments register, and segmentation in
general. In the GDT you'd only store a pointer to the per-cpu data, and
processors will load it.

> Is there any way to avoid using an array for that?

Yes, you'd just store the ID in the per-cpu data.

Samuel

[Almudena Garcia]

> You don't. Read about GDT and segments register, and segmentation in
> general. In the GDT you'd only store a pointer to the per-cpu data, and
> processors will load it.

> > Is there any way to avoid using an array for that?

> Yes, you'd just store the ID in the per-cpu data.

Thanks, I'll take notes about this

[Samuel Thibault]

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 <liberame...@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 <liberame...@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 <liberame...@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 <liberame...@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?

[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.

[Samuel Thibault]

Almudena Garcia, le mar. 11 août 2020 01:23:49 +0200, a ecrit:

> > ? 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 use git rebase to rewrite your patch history?

> > ? 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.

I didn't write about removing NCPUS. I wrote about your code defining
NCPUS to a value that is different from mach_ncpus, while it could just
define mach_ncpus, and let NCPUS inherit the value from mach_ncpus so
it's all coherent.

That being said, instead of hardcoding the maximum number of CPUs to
be 256, you can just let the user choose whatever value is preferred.
That's what Linux does.

> > 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.

But there is nothing there to be compiled. Some compilation toolchains
would even emit a warning in such a case, or just plainly error out
because they cannot produce an empty .o file.

> > 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.

Ok, but since smp_init is almost empty, you can as well initialize the
data in there.

> > 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.

But the function returns an APIC id. Really not much can be done with
that without knowing details of the 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.

For global variables the default value is *already* asserted to be NULL.
But anyway I was not talking about that at all.

Re-read what I wrote. I was talking about lapic being qualified
volatile, and apic_get_lapic returning it as non-volatile. Aren't you
getting a warning there?

> > 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.

?????????????????????????????

I can't see how this declaration can't be in kern/smp.h. It's not even
returning a struct or whatever. What is the actual error message when
you put it in kern/smp.h?

> > ? 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.

What for?
num_cpus is something that you make returned by a smp_get_numcpus()
function, so it's not actually useful to also expose it in a struct.
What else would go into that structure? Won't we actually rather
use functions to return such information rather than imposing that
structure over all archs?

> 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.

Which is possibly another sign that it's generally simpler to keep it in
the arch-specific part, and only expose simple functions like
int smp_get_numcpus(void); which pose no declaration problems.

> > ? 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.

Re-read what I wrote. I do *not* think we want to print this as an
error like you did. Not having SMP is not an error. Just do not print
anything.

Samuel

[Almudena Garcia]

> That being said, instead of hardcoding the maximum number of CPUs to
> be 256, you can just let the user choose whatever value is preferred.

> That's what Linux does.

But this could cause coherency problems.

By example, if the user sets mach_ncpus=4 , and the processor has 8 cores, It can produce an out-of-index error in the access to the arrays which store the info about the cpus.

> Re-read what I wrote. I was talking about lapic being qualified
> volatile, and apic_get_lapic returning it as non-volatile. Aren't you
> getting a warning there?

Yes, I've just fixed It.

> I can't see how this declaration can't be in kern/smp.h. It's not even
> returning a struct or whatever. What is the actual error message when
> you put it in kern/smp.h?

I've just solved It declaring smp_info in kern/smp.c , and adding an extern declaration in its header.

> What for?
> num_cpus is something that you make returned by a smp_get_numcpus()
> function, so it's not actually useful to also expose it in a struct.
> What else would go into that structure?

By example, the master cpu (BSP in x86)

> But the function returns an APIC id. Really not much can be done with
> that without knowing details of the APIC.

It's true. I didn't remember this. I can search the kernel ID of this APIC ID, but cpu_number() already will do that.

Then, maybe it can be simpler to remove this function.
My idea was to avoid calling directly to the apic function when I will implement cpu_number(). But maybe this is not the best solution for this.

I have to rethink this.

> What for?
> num_cpus is something that you make returned by a smp_get_numcpus()
> function, so it's not actually useful to also expose it in a struct.
> What else would go into that structure? Won't we actually rather
> use functions to return such information rather than imposing that
> structure over all archs?

I will not use this struct to share the information, simply to ease the access to the functions which really return such information.

smp_get_numcpus() doesn't return NCPUS value, but returns the real number of cpus existent in the machine.

I simply use a struct to store this generic information instead access to apic's ApicInfo struct (which stores this value in x86).

> Re-read what I wrote. I do *not* think we want to print this as an
> error like you did. Not having SMP is not an error. Just do not print
> anything.

Really, the mistake is in the message. I wanted to advise that the processor detection has failed in any step (although maybe the cause is not that SMP doesn't exist in the machine...)

Then, I will have to change this message to a better explanation. But showing a different message for each error code can be very lazy... :(

[Jessica Clarke]

On 11 Aug 2020, at 01:17, Almudena Garcia <liberame...@gmail.com> wrote:
>
> > That being said, instead of hardcoding the maximum number of CPUs to
> > be 256, you can just let the user choose whatever value is preferred.
> > That's what Linux does.
>
> But this could cause coherency problems.

Coherency is a very specific thing in operating systems about the
properties of the underlying memory subsystem. It's not the word you're
looking for.

> By example, if the user sets mach_ncpus=4 , and the processor has 8 cores, It can produce an out-of-index error in the access to the arrays which store the info about the cpus.

So don't bring them online then? The user asked for 4 CPUs, so bring up
3 APs alongside the BSP and that's that.

Jess

[Almudena Garcia]

> So don't bring them online then? The user asked for 4 CPUs, so bring up

> 3 APs alongside the BSP and that's that.

It's not a bad idea. Then I will fix my cpu enumeration loop to add this new condition.

[Almudena Garcia]

oops!! I forgot add i386/i386/smp.h and i386/i386/smp.c to i386/Makefrag.am

I will try to fix the patch to add this

El mar., 11 ago. 2020 a las 23:03, Almudena Garcia (<liberame...@gmail.com>) escribió:

I attach a new set of patches. I wait this time will be better than last time

[Samuel Thibault]

Almudena Garcia, le mar. 11 août 2020 02:17:35 +0200, a ecrit:

> > What for?
> > num_cpus is something that you make returned by a smp_get_numcpus()
> > function, so it's not actually useful to also expose it in a struct.
> > What else would go into that structure?
> By example, the master cpu (BSP in x86)

More precisely?

(saying it like this, it looks like an x86-specific thing that we
wouldn't actually expose to the rest of the code...)

> > But the function returns an APIC id. Really not much can be done with
> > that without knowing details of the APIC.
> It's true. I didn't remember this. I can search the kernel ID of this APIC ID,
> but cpu_number() already will do that.
> Then, maybe it can be simpler to remove this function.

Yes.

> My idea was to avoid calling directly to the apic function when I will
> implement cpu_number(). But maybe this is not the best solution for this.

It'll be arch-dependent anyway, so it makes sense to just call it
directly.

> > What for?
> > num_cpus is something that you make returned by a smp_get_numcpus()
> > function, so it's not actually useful to also expose it in a struct.
> > What else would go into that structure? Won't we actually rather
> > use functions to return such information rather than imposing that
> > structure over all archs?
>
> I will not use this struct to share the information, simply to ease the access
> to the functions which really return such information.

Then there is no need to expose the struct content in the .h file, keep
it in the .c file.

> > Re-read what I wrote. I do *not* think we want to print this as an
> > error like you did. Not having SMP is not an error. Just do not print
> > anything.
> Really, the mistake is in the message. I wanted to advise that the processor
> detection has failed in any step (although maybe the cause is not that SMP
> doesn't exist in the machine...)

When a machine doesn't have apic/acpi at all we don't want to print an
error.

> Then, I will have to change this message to a better explanation. But showing a
> different message for each error code can be very lazy... :(

Users do need that to understand what's happening.

Samuel

[Samuel Thibault]

Almudena Garcia, le mer. 12 août 2020 16:03:06 +0200, a ecrit:
> Subject: [PATCH 1/5] smp: Add pseudoclass to manage APIC operations

Applied, thanks!

> Subject: [PATCH 2/5] vm_kern: Add kmem_alloc_aligned_table

Applied and fixed a bit, thanks!

> Subject: [PATCH 3/5] smp: Add APIC finder and parser

This will have to wait for the assignment paper for Juan.

> @@ -152,6 +152,8 @@ EXTRA_DIST += \
>
> if PLATFORM_at

> libkernel_a_SOURCES += \
> + i386/i386at/acpi_parse_apic.h \
> + i386/i386at/acpi_parse_apic.c \

> i386/i386/apic.h \
> i386/i386/apic.c \
> i386/i386/hardclock.c \

Add these along the other lines adding i386/i386at files, not along the
lines adding i386/i386 files.

> From d7c1f9d043d39500ebb39b6566f55da584832480 Mon Sep 17 00:00:00 2001
> From: Almudena Garcia <liberame...@gmail.com>
> Date: Wed, 12 Aug 2020 15:53:56 +0200
> Subject: [PATCH 4/5] smp: Add generic smp pseudoclass

>
> diff --git a/i386/i386/smp.c b/i386/i386/smp.c
> new file mode 100644

> index 00000000..09e4bafa
> --- /dev/null
> +++ b/i386/i386/smp.c

> + * smp_init: initialize the SMP support, starting the cpus searching
> + * and enumeration.
> + */
> +int smp_init(void)
> +{
> + int apic_success;
> +
> + apic_success = acpi_apic_init();
> + if (apic_success) {

But that will only enter if when api_success != 0, thus when we actually
have a failure. Compare explicitly against ACPI_SUCCESS

> + smp_data_init();
> + }
> +
> + return apic_success;
> +}

Samuel

[Almudena Garcia]

> Applied and fixed a bit, thanks!

Thanks. Now I only have to fix the next patches. We are closer now

> But that will only enter if when api_success != 0, thus when we actually

> have a failure. Compare explicitly against ACPI_SUCCESS

Oops, I forget this. I will fix It.

> Then there is no need to expose the struct content in the .h file, keep
> it in the .c file.

Ok, It's a good solution.

> This will have to wait for the assignment paper for Juan.

I advised him that he has to write yours to explain how to do the assignment.

> When a machine doesn't have apic/acpi at all we don't want to print an
> error.

Ok, then I can simply remove that print.

> Users do need that to understand what's happening.

But, if by any reason, the machine has more than cpu but the detection fails, could be interesting to advise about the system being running with an only cpu.

It's not?

Thanks by all. I'm impatient to finish this patch and start the work in cpus'  enabling and configuration.

[Samuel Thibault]

Almudena Garcia, le jeu. 13 août 2020 00:35:36 +0200, a ecrit:

> > Users do need that to understand what's happening.
> But, if by any reason, the machine has more than cpu but the detection fails,
> could be interesting to advise about the system being running with an only cpu.
> It's not?

Yes. But your current code is printing an error also in other cases.

Samuel