[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 5/5] ioapic interrupts and lapic timer support
From: |
Samuel Thibault |
Subject: |
Re: [PATCH 5/5] ioapic interrupts and lapic timer support |
Date: |
Sat, 27 Mar 2021 17:09:31 +0100 |
User-agent: |
NeoMutt/20170609 (1.8.3) |
Damien Zammit, le ven. 26 mars 2021 20:48:50 +1100, a ecrit:
> @@ -155,6 +154,16 @@ EXTRA_DIST += \
> i386/i386/mach_i386.srv
>
> if PLATFORM_at
> +if enable_apic
> +libkernel_a_SOURCES += \
> + i386/i386at/ioapic.c
> +else
> +libkernel_a_SOURCES += \
> + i386/i386/pic.c \
> + i386/i386/pic.h \
> + i386/i386at/pic_isa.c
> +endif
Rather add theses lines after the following
> libkernel_a_SOURCES += \
> i386/i386/apic.h \
> i386/i386/apic.c \
> @@ -163,8 +172,6 @@ libkernel_a_SOURCES += \
> i386/i386/io_map.c \
> i386/i386/irq.c \
> i386/i386/irq.h \
> - i386/i386/pic.c \
> - i386/i386/pic.h \
> i386/i386/pit.c \
> i386/i386/pit.h
i.e. there
> endif
> -struct IoApicData
> +struct IoApicData *
> apic_get_ioapic(int kernel_id)
> {
> - IoApicData io_apic = {};
> -
> if (kernel_id < MAX_IOAPICS)
> - return apic_data.ioapic_list[kernel_id];
> - return io_apic;
> + return &apic_data.ioapic_list[kernel_id];
> + return (struct IoApicData *)0;
No need to cast, just return NULL.
> - ioapic_id = ioapic.apic_id;
> - printf(" IOAPIC %d - APIC ID %x\n", i, ioapic_id);
> + if (!ioapic) {
> + printf("ERROR: invalid IOAPIC ID %x\n", i);
> + }
> + ioapic_id = ioapic->apic_id;
if !ioapic, you can't continue and dereference ioapic...
> + printf(" IOAPIC %d - APIC ID %x - addr=0x%p\n", i, ioapic_id,
> ioapic->ioapic);
> }
> }
> #define APIC_IRQ_OVERRIDE_ACTIVE_LOW 2
> +#define APIC_IRQ_OVERRIDE_POLARITY_MASK 1
> #define APIC_IRQ_OVERRIDE_LEVEL_TRIGGERED 8
> +#define APIC_IRQ_OVERRIDE_TRIGGER_MASK 4
Why not keep them in order?
> diff --git a/i386/i386/pic.h b/i386/i386/pic.h
> index 6434bf08..0ccf1c9a 100644
> --- a/i386/i386/pic.h
> +++ b/i386/i386/pic.h
> @@ -96,7 +96,7 @@ WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> */
>
> #if defined(AT386) || defined(ATX86_64)
> -#define PICM_VECTBASE 0x40
> +#define PICM_VECTBASE 0x20
> #define PICS_VECTBASE PICM_VECTBASE + 0x08
> #endif /* defined(AT386) */
I'd say make this one a separate commit, since it has consequences on
the PIC case that could bring regressions, so we'd rather be able to
easily test this commit only.
> @@ -66,14 +66,44 @@ int pit0_mode = PIT_C0|PIT_SQUAREMODE|PIT_READMODE ;
> unsigned int clknumb = CLKNUM; /* interrupt interval for timer
> 0 */
>
> void
> -clkstart(void)
> +pit_prepare_sleep(int hz)
> {
> + /* Prepare to sleep for 1/hz seconds */
> + int val = 0;
> + int lsb, msb;
> +
> + val = (inb(0x61) & 0xfd) | 0x1;
> + outb(0x61, val);
> + outb(0x43, 0xb2);
Anonymous magic numbers, we don't want these :)
There are already macros in pit.h.
> void
> clkstart(void)
> {
> if (cpu_number() != 0)
> /* Only one PIT initialization is needed */
> return;
> - unsigned char byte;
> - unsigned long s;
> + unsigned long s;
> + unsigned char byte;
Please avoid unrelated spurious changes.
> diff --git a/i386/i386at/acpi_parse_apic.c b/i386/i386at/acpi_parse_apic.c
> index 9dbc6825..449dba71 100644
> --- a/i386/i386at/acpi_parse_apic.c
> +++ b/i386/i386at/acpi_parse_apic.c
> @@ -280,7 +280,7 @@ acpi_get_apic(struct acpi_rsdt *rsdt, int acpi_rsdt_n)
> /* Search APIC entries in rsdt table. */
> for (int 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 | VM_PROT_WRITE);
> +
> VM_PROT_READ);
>
> /* Check if the entry contains an APIC. */
> check_signature = acpi_check_signature(descr_header->signature,
> ACPI_APIC_SIG, 4*sizeof(uint8_t));
This looks like something we'd want to commit separately?
> @@ -414,6 +416,8 @@ acpi_apic_parse_table(struct acpi_apic *apic)
> acpi_apic_add_irq_override(irq_override_entry);
> break;
>
> + default:
> + break;
> }
This seems useless?
> diff --git a/i386/i386at/interrupt.S b/i386/i386at/interrupt.S
> index 23a2e582..50e0391d 100644
> --- a/i386/i386at/interrupt.S
> +++ b/i386/i386at/interrupt.S
> @@ -16,7 +16,11 @@
> #include <mach/machine/asm.h>
>
> #include <i386/ipl.h>
> -#include <i386/pic.h>
> +#ifdef APIC
> +# include <i386/apic.h>
> +#else
> +# include <i386/pic.h>
> +#endif
> #include <i386/i386asm.h>
>
> #define READ_ISR (OCW_TEMPLATE|READ_NEXT_RD|READ_IS_ONRD)
> @@ -29,6 +33,8 @@
> ENTRY(interrupt)
> pushl %eax /* save irq number */
> movl %eax,%ecx /* copy irq number */
> + cmpl $255,%eax /* was this a spurious intr? */
> + je 2f /* if so, null handler */
> shll $2,%ecx /* irq * 4 */
> call spl7 /* set ipl */
> movl EXT(iunit)(%ecx),%edx /* get device unit number */
> @@ -38,10 +44,9 @@ ENTRY(interrupt)
> addl $4,%esp /* pop unit number */
> call splx_cli /* restore previous ipl */
> addl $4,%esp /* pop previous ipl */
> -
> cli /* XXX no more nested interrupts */
> popl %ecx /* restore irq number */
> -
> +#ifndef APIC
We want an APIC-enabled kernel to also be able to work on non-APIC
hardware. So rather test for a global use_apic variable.
> +void (*ivect[NINTR])() = {
> + /* 00 */ hardclock, /* always */
> + /* 01 */ kdintr, /* kdintr, ... */
> + /* 02 */ intnull,
> + /* 03 */ intnull, /* lnpoll, comintr, ... */
> +
> + /* 04 */ intnull, /* comintr, ... */
> + /* 05 */ intnull, /* comintr, wtintr, ... */
> + /* 06 */ intnull, /* fdintr, ... */
> + /* 07 */ intnull, /* qdintr, ... */
> +
> + /* 08 */ intnull,
> + /* 09 */ intnull, /* ether */
> + /* 10 */ intnull,
> + /* 11 */ intnull,
> +
> + /* 12 */ intnull,
> + /* 13 */ fpintr, /* always */
> + /* 14 */ intnull, /* hdintr, ... */
> + /* 15 */ intnull, /* ??? */
> +
> + /* 16 */ intnull, /* PIRQA */
> + /* 17 */ intnull, /* PIRQB */
> + /* 18 */ intnull, /* PIRQC */
> + /* 19 */ intnull, /* PIRQD */
> + /* 20 */ intnull, /* PIRQE */
> + /* 21 */ intnull, /* PIRQF */
> + /* 22 */ intnull, /* PIRQG */
> + /* 23 */ intnull, /* PIRQH */
> +};
Please factorize with pic_isa.c, otherwise they'll diverge and confuse
people.
> +void
> +picdisable(void)
> +{
> + asm("cli");
> +
> + /*
> + ** Disable PIC
> + */
> + outb ( 0xa1, 0xff );
> + outb ( 0x21, 0xff );
Avoid magic numbers, you have PIC_MASTER_OCW, PIC_SLAVE_OCW, PICM_MASK,
PICS_MASK.
> +void
> +intnull(int unit_dev)
> +{
> + printf("intnull(%d)\n", unit_dev);
> +}
Rather move the pic.c implementation to irq.c
> +static void
> +ioapic_write_entry(int apic, int pin, struct ioapic_route_entry e)
> +{
> + union ioapic_route_entry_union entry = {{0, 0}};
No need to set it to 0?
> +static uint32_t
> +pit_measure_apic_hz(void)
> +{
Is there no way to just get the information?
> + /* Set up counter */
> + lapic->init_count.r = calibrated_ticks;
> + lapic->divider_config.r = LAPIC_TIMER_DIVIDE_16;
> +
> + /* Set the timer to interrupt periodically */
> + lapic->lvt_timer.r = IOAPIC_INT_BASE | LAPIC_TIMER_PERIODIC;
> +
> + /* Some buggy hardware requires this set again */
> + lapic->divider_config.r = LAPIC_TIMER_DIVIDE_16;
? perhaps a reference?
"some buggy hardware" is not so rarely used to hide an actually software
bug :)
> @@ -356,7 +359,11 @@ i386at_init(void)
> * Initialize the PIC prior to any possible call to an spl.
> */
> #ifndef MACH_HYP
> +# ifdef APIC
> + picdisable();
> +# else
> picinit();
> +# endif
Note the comment: mach code will call spl(), so spl() must be ready for
that.
> @@ -682,7 +689,9 @@ timemmap(dev, off, prot)
> void
> startrtclock(void)
> {
> +#ifndef APIC
> clkstart();
> +#endif
> }
I'd say it'd be safer to do the same with APIC: enable the clock only
here. Otherwise you might be making some existing assumptions wrong
(e.g. the clock interrupt handler)
> diff --git a/linux/dev/arch/i386/kernel/irq.c
> b/linux/dev/arch/i386/kernel/irq.c
> index 06889e58..656c1470 100644
> --- a/linux/dev/arch/i386/kernel/irq.c
> +++ b/linux/dev/arch/i386/kernel/irq.c
I'd say commit the NINTR cleanup separately.
Samuel
- [PATCH 2/5] fixup acpi base table search, (continued)
- [PATCH 2/5] fixup acpi base table search, Damien Zammit, 2021/03/26
- [PATCH 1/5] Make linux drivers optional, Damien Zammit, 2021/03/26
- [PATCH 3/5] fix timer bug, Damien Zammit, 2021/03/26
- [PATCH 4/5] fix EISA check in init_IRQ, Damien Zammit, 2021/03/26
- [PATCH 5/5] ioapic interrupts and lapic timer support, Damien Zammit, 2021/03/26
- Re: gnumach: IOAPIC and LAPIC support, Samuel Thibault, 2021/03/26