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

Reply via email to