|
| From: | Almudena Garcia |
| Subject: | Re: [PATCH] SMP initialization: detection and enumeration |
| Date: | Tue, 28 Jul 2020 10:27:55 +0200 |
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-dist@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
| [Prev in Thread] | Current Thread | [Next in Thread] |