bug-hurd
[Top][All Lists]
Advanced

[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



reply via email to

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