LinuxLists.cc - Updated MSI Patches

[permalink] [raw]

Subject: Re: Updated MSI Patches

pcihpd-discuss? Don't you mean the linux-pci mailing list instead?

A few initial comments on your patch:
- Is there any way to dynamically detect if hardware can support MSI?
- If you enable this option, will boxes that do not support it stop
working?

A driver has to be modified to use this option, right? Do you have any
drivers that have been modified? Without that, I don't think we can
test this patch out, right?

> diff -X excludes -urN linux-2.6.0-test2/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2/arch/i386/kernel/io_apic.c 2003-07-27 13:00:21.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400

You are cluttering up this file with a lot of #ifdefs. Is there any way
you can not do this?

Does this break the summit and/or NUMA builds?

> diff -X excludes -urN linux-2.6.0-test2/include/asm-i386/mach-default/irq_vectors.h linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h
> --- linux-2.6.0-test2/include/asm-i386/mach-default/irq_vectors.h 2003-07-27 12:58:54.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h 2003-08-05 09:25:54.000000000 -0400
> @@ -76,9 +76,14 @@
> * Since vectors 0x00-0x1f are used/reserved for the CPU,
> * the usable vector space is 0x20-0xff (224 vectors)
> */
> +#define NR_VECTORS 256

Will this _always_ be the value? Can boxes have bigger numbers (I
haven't seen the spec, so I don't know...)

Care to add a comment about this value?

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/i386_ksyms.c linux-2.6.0-test2-create-msi/arch/i386/kernel/i386_ksyms.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/i386_ksyms.c 2003-07-27 13:11:42.000000000 -0400
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/i386_ksyms.c 2003-08-05 09:45:25.000000000 -0400
> @@ -166,6 +166,11 @@
> EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
> #endif
>
> +#ifdef CONFIG_PCI_MSI
> +EXPORT_SYMBOL(msix_alloc_vectors);
> +EXPORT_SYMBOL(msix_free_vectors);
> +#endif
> +
> #ifdef CONFIG_MCA
> EXPORT_SYMBOL(machine_id);
> #endif

Put the EXPORT_SYMBOL in the files that have the functions. Don't
clutter up the ksyms.c files anymore.

> +u32 device_nomsi_list[DRIVER_NOMSI_MAX] = {0, };

Shouldn't this be static?

> +u32 device_msi_list[DRIVER_NOMSI_MAX] = {0, };

Same with this one?

> + base = (u32*)ioremap_nocache(phys_addr,
> + dev_msi_cap*PCI_MSIX_ENTRY_SIZE*sizeof(u32));
> + if (base == NULL)
> + goto free_region;

...

> + *base = address.lo_address.value;
> + *(base + 1) = address.hi_address;
> + *(base + 2) = *((u32*)&data);

Don't do direct writes to memory, use the proper function calls. You do
this in a few other places too.

That's enough to get the conversation started :)

thanks,

greg k-h

2003-08-07 22:45:05

by Jeff Garzik

[permalink] [raw]

Subject: Re: Updated MSI Patches

long wrote:

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c 2003-08-06 09:47:02.000000000 -0400

Seems like a lot of this file could go into a new file,
drivers/pci/msi.c. We'll want to share as much code as possible across
all Linux architectures.

Jeff

2003-08-07 23:03:44

by Matt Porter

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Thu, Aug 07, 2003 at 06:44:46PM -0400, Jeff Garzik wrote:
> long wrote:
>
>
> > diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c
> > --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c 1969-12-31 19:00:00.000000000 -0500
> > +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c 2003-08-06 09:47:02.000000000 -0400
>
> Seems like a lot of this file could go into a new file,
> drivers/pci/msi.c. We'll want to share as much code as possible across
> all Linux architectures.

Yes. The on-chip PCI-X host bridge on PPC44x parts has MSI support, so
I can make use of this if it goes in the right place.

--
Matt Porter
[email protected]

2003-08-07 23:08:04

[permalink] [raw]

Subject: RE: Updated MSI Patches

> Seems like a lot of this file could go into a new file,
> drivers/pci/msi.c. We'll want to share as much code as possible
across
> all Linux architectures.
Agree.

At the same time, Long, you need to provide more info so that more
people can play with MSI.

Jun

> -----Original Message-----
> From: Jeff Garzik [mailto:[email protected]]
> Sent: Thursday, August 07, 2003 3:45 PM
> To: long
> Cc: [email protected]; Nakajima, Jun; Nguyen, Tom L;
> [email protected]
> Subject: Re: Updated MSI Patches
>
> long wrote:
>
>
> > diff -X excludes -urN linux-2.6.0-test2-create-
> vectorbase/arch/i386/kernel/pci_msi.c linux-2.6.0-test2-create-
> msi/arch/i386/kernel/pci_msi.c
> > --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c
> 1969-12-31 19:00:00.000000000 -0500
> > +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c 2003-08-
> 06 09:47:02.000000000 -0400
>
> Seems like a lot of this file could go into a new file,
> drivers/pci/msi.c. We'll want to share as much code as possible
across
> all Linux architectures.
>
> Jeff
>

2003-08-08 02:48:08

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Thu, 7 Aug 2003, long wrote:

> - Change the interface name from msi_free_vectors to msix_free_vectors since
> this interface is used for MSI-X device drivers, which request for releasing
> the number of vector back to the PCI subsystem.
> - Change the function name from remove_hotplug_vectors to
> msi_remove_pci_irq_vectors to have a close match with function name
> msi_get_pci_irq_vector.

I think the vector allocator code can all be arch specific generic,
there is no particular reason as to why it has to be MSI specific.

> diff -X excludes -urN linux-2.6.0-test2/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2/arch/i386/kernel/io_apic.c 2003-07-27 13:00:21.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400
> @@ -76,6 +76,20 @@
> int apic, pin, next;
> } irq_2_pin[PIN_MAP_SIZE];
>
> +#ifdef CONFIG_PCI_USE_VECTOR
> +int vector_irq[NR_IRQS] = { [0 ... NR_IRQS -1] = -1};
> +
> +static int platform_irq(int irq)
> +{
> + if (platform_legacy_irq(irq))
> + return irq;
> + else
> + return vector_irq[irq];
> +}
> +#else
> +#define platform_irq(irq) (irq)
> +#endif

I wish you wouldn't mix up vector and irq like this :( This is really
beginning to look like IPF. When is an irq an irq and when is a vector a
vector? This can get very confusing very quickly. I spent a fair
amount of time debugging this last time due to the confusion in the
common_interrupt -> do_IRQ path, i'd hate for this to be a permanent
fixture. If you're going to change things just change everything
(including variable names) instead of making it conditional.

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-msi/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/io_apic.c 2003-08-05 09:45:25.000000000 -0400
> @@ -427,6 +427,11 @@
> /* Is this an active IRQ? */
> if (!irq_desc[j].action)
> continue;
> +#ifdef CONFIG_PCI_MSI
> + /* Is this an active MSI? */
> + if (msi_desc[j])
> + continue;
> +#endif

Is there any locking for msi_desc? Or are you relying on something else?

> +static int nr_alloc_vectors = 0;
> +static int nr_released_vectors = 0;

No need to initialise to 0 it'll be done for you when .bss gets zeroed.

> + if (current_vector == FIRST_SYSTEM_VECTOR)
> + panic("ran out of interrupt sources!");

Please just fail and return -ENOSPC or something like that.

Besides my whining thanks for cranking this out.

Zwane

2003-08-08 14:41:55

[permalink] [raw]

Subject: RE: Updated MSI Patches

> Seems like a lot of this file could go into a new file,
> drivers/pci/msi.c. We'll want to share as much code as possible across
> all Linux architectures.
Agree. Next update release will incorporate your feedback.

Thanks,
Long

-----Original Message-----
From: Jeff Garzik [mailto:[email protected]]
Sent: Thursday, August 07, 2003 3:45 PM
To: long
Cc: [email protected]; Nakajima, Jun; Nguyen, Tom L;
[email protected]
Subject: Re: Updated MSI Patches

long wrote:

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/pci_msi.c 1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/pci_msi.c 2003-08-06 09:47:02.000000000 -0400

Seems like a lot of this file could go into a new file,
drivers/pci/msi.c. We'll want to share as much code as possible across
all Linux architectures.

Jeff

2003-08-08 19:34:15

by long

[permalink] [raw]

Subject: Re: Updated MSI Patches

>pcihpd-discuss? Don't you mean the linux-pci mailing list instead?
pcihpd-discuss is my first assumption of what you mean the linux-pci mailing list. Please forward me the email address of the linux-pci mailing list.

> - Is there any way to dynamically detect if hardware can support MSI?
The MSI patch dynamically detects whether hardware devices can support MSI once the CONFIG_PCI_MSI is set during kernel built. By default, the CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES is also set to provide users an option to enable MSI on specific devices with the use of boot parameter "device_msi=". With both CONFIG_PCI_MSI and CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES set, the PCI subsystem dynamically detects if hardware devices can support MSI. However, enabling MSI on these devices is determined based on whether these devices are explicitly listed in the boot parameter "device_msi=". The reason is that users may not know how many MSI capable hardware devices are populated in their systems. The software device drivers of these devices may break their system once the patches are installed and users want the PCI subsystem to enable MSI automatically on all MSI capable devices. The second reason is that it may be difficult to debug all MSI capable devices at the same time. Users can de!
bug on individual MSI capable device with its existing software driver. When all are fully validated, users can rebuild the kernel with the CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES cleared; the PCI subsystem will enable MSI on all MSI capable devices automatically and the boot parameter "device_msi=" is no longer necessary.

> - If you enable this option, will boxes that do not support it stop
working?
Once users enable this option, all devices that do not support MSI are still working. These devices are by default using IRQ pin assertion as interrupt generated mechanism since all pre-assigned vectors to any enabled IOxAPICs are reserved.

>A driver has to be modified to use this option, right? Do you have any
>drivers that have been modified? Without that, I don't think we can
>test this patch out, right?
MSI support may be transparent to some device drivers. This transparency is dependent on whether the existing driver can provide the hardware/software synchronization; that means, once the hardware/firmware of the device signals a Vector A, it cannot signal Vector A again until it is explicitly enabled to do so by its device driver. Such example of this transparency is the Intel Pro(R) Ethernet (10000 MBit> driver, which works with these patches without requiring any changes in the existing driver. The second driver we are validating is Adaptec AIC79XX PCI-X SCSI HBA driver (Rev 1.3.9). This existing software driver is tested as a module driver (such as mount/unmount /dev/sda2 /mnt/temp and then read/write to files to this device). However, if its hardware device is a boot device, then the results are unpredictable. We are currently working with the driver engineer to root cause the issues.

> diff -X excludes -urN linux-2.6.0-test2/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2/arch/i386/kernel/io_apic.c 2003-07-27 13:00:21.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400

>You are cluttering up this file with a lot of #ifdefs. Is there any way
>you can not do this?
Agree. We will make appropriate changes in the next update release.

>Does this break the summit and/or NUMA builds?
We have not tested with these builds, I will add it to my list of builds to test. Thanks.

> diff -X excludes -urN linux-2.6.0-test2/include/asm-i386/mach-default/irq_vectors.h linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h
> --- linux-2.6.0-test2/include/asm-i386/mach-default/irq_vectors.h 2003-07-27 12:58:54.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h 2003-08-05 09:25:54.000000000 -0400
> @@ -76,9 +76,14 @@
> * Since vectors 0x00-0x1f are used/reserved for the CPU,
> * the usable vector space is 0x20-0xff (224 vectors)
> */
> +#define NR_VECTORS 256

>Will this _always_ be the value? Can boxes have bigger numbers (I
>haven't seen the spec, so I don't know...)
Yes, this will always be 256 since the current CPU architecture supports a maximum of 256 vectors.

>Care to add a comment about this value?
Agree. Will add a comment about this value in the next update release.

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/i386_ksyms.c linux-2.6.0-test2-create-msi/arch/i386/kernel/i386_ksyms.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/i386_ksyms.c 2003-07-27 13:11:42.000000000 -0400
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/i386_ksyms.c 2003-08-05 09:45:25.000000000 -0400
> @@ -166,6 +166,11 @@
> EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
> #endif
>
> +#ifdef CONFIG_PCI_MSI
> +EXPORT_SYMBOL(msix_alloc_vectors);
> +EXPORT_SYMBOL(msix_free_vectors);
> +#endif
> +
> #ifdef CONFIG_MCA
> EXPORT_SYMBOL(machine_id);
> #endif

>Put the EXPORT_SYMBOL in the files that have the functions. Don't
>clutter up the ksyms.c files anymore.
Agree. Will put these EXPORT_SYMBOL(s) in the file ../include/asm-i386/hw_irq.h where these functions are declared as extern. Thanks.

> +u32 device_nomsi_list[DRIVER_NOMSI_MAX] = {0, };

>Shouldn't this be static?

> +u32 device_msi_list[DRIVER_NOMSI_MAX] = {0, };

>Same with this one?

In the initial design draft, I am thinking of defining DRIVER_NOMSI_MAX as 32 since it may be impossible to have more than 32 MSI capable devices populated in the same system. I will add some comments to it and look forward for your comments.

> + base = (u32*)ioremap_nocache(phys_addr,
> + dev_msi_cap*PCI_MSIX_ENTRY_SIZE*sizeof(u32));
> + if (base == NULL)
> + goto free_region;

...

> + *base = address.lo_address.value;
> + *(base + 1) = address.hi_address;
> + *(base + 2) = *((u32*)&data);

>Don't do direct writes to memory, use the proper function calls. You do
>this in a few other places too.
Thanks. I will replace all direct writes/reads with the proper functions calls such as writel()/readl().

thanks,
Long

2003-08-08 20:16:09

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Fri, Aug 08, 2003 at 12:33:01PM -0700, long wrote:
> >pcihpd-discuss? Don't you mean the linux-pci mailing list instead?
> pcihpd-discuss is my first assumption of what you mean the linux-pci
> mailing list. Please forward me the email address of the linux-pci
> mailing list.

[email protected]

Also, please wrap your emails at a sane size, you have lines here that
are longer than 900 characters long... I've reformatted them here to be
able to quote them properly.

> > - Is there any way to dynamically detect if hardware can support MSI?
> The MSI patch dynamically detects whether hardware devices can support
> MSI once the CONFIG_PCI_MSI is set during kernel built.

So why would we ever want CONFIG_PCI_MSI to not be set? I'm just trying
to make options dynamically happen without users having to worry about
setting a kernel option or not.

> > - If you enable this option, will boxes that do not support it stop
> > working?
> Once users enable this option, all devices that do not support MSI are
> still working. These devices are by default using IRQ pin assertion as
> interrupt generated mechanism since all pre-assigned vectors to any
> enabled IOxAPICs are reserved.

Good, so we can always enable this option, so it doesn't even need to be
an option :)

> >A driver has to be modified to use this option, right? Do you have any
> >drivers that have been modified? Without that, I don't think we can
> >test this patch out, right?
> MSI support may be transparent to some device drivers.

Yet you provide an api for drivers to use, right? Why would a driver
want to use this api instead of just having the hardware work properly
on its own? I guess I'm confused as to how to modify drivers to support
this, or if that's even needed to be done.

> > +++ linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h 2003-08-05 09:25:54.000000000 -0400
> > @@ -76,9 +76,14 @@
> > * Since vectors 0x00-0x1f are used/reserved for the CPU,
> > * the usable vector space is 0x20-0xff (224 vectors)
> > */
> > +#define NR_VECTORS 256
>
> >Will this _always_ be the value? Can boxes have bigger numbers (I
> >haven't seen the spec, so I don't know...)
> Yes, this will always be 256 since the current CPU architecture
> supports a maximum of 256 vectors.

The "current" one does. What happens in a few months when I go to help
bring up the next gen hardware platform? Will I run into the problem
that this number needs to be increased? Can we determine it at
run-time?

> > +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/i386_ksyms.c 2003-08-05 09:45:25.000000000 -0400
> > @@ -166,6 +166,11 @@
> > EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
> > #endif
> >
> > +#ifdef CONFIG_PCI_MSI
> > +EXPORT_SYMBOL(msix_alloc_vectors);
> > +EXPORT_SYMBOL(msix_free_vectors);
> > +#endif
> > +
> > #ifdef CONFIG_MCA
> > EXPORT_SYMBOL(machine_id);
> > #endif
>
> >Put the EXPORT_SYMBOL in the files that have the functions. Don't
> >clutter up the ksyms.c files anymore.
> Agree. Will put these EXPORT_SYMBOL(s) in the file
> ../include/asm-i386/hw_irq.h where these functions are declared as
> extern. Thanks.

No, it needs to go into the .c file where the function is.

> > +u32 device_nomsi_list[DRIVER_NOMSI_MAX] = {0, };
>
> >Shouldn't this be static?
>
> > +u32 device_msi_list[DRIVER_NOMSI_MAX] = {0, };
>
> >Same with this one?
>
> In the initial design draft, I am thinking of defining
> DRIVER_NOMSI_MAX as 32 since it may be impossible to have more than 32
> MSI capable devices populated in the same system. I will add some
> comments to it and look forward for your comments.

Um, I ment the variable is in the global symbol table, and it doesn't
look like it needs to be.

But your comment about number of devices is also a good one. Any way we
can figure that out at run-time too? What happens when I hot-add the 33
MSI capable device? Or just boot with that many devices (I've seen
boxes with 100s of pci devices all attached to one system)?

thanks,

greg k-h

2003-08-11 20:51:48

by long

[permalink] [raw]

Subject: Re: Updated MSI Patches

> Is there any way to dynamically detect if hardware can support MSI?
> The MSI patch dynamically detects whether hardware devices can support
> MSI once the CONFIG_PCI_MSI is set during kernel built.

> So why would we ever want CONFIG_PCI_MSI to not be set? I'm just
> trying to make options dynamically happen without users having to
> worry about setting a kernel option or not.

> If you enable this option, will boxes that do not support it stop
> working? Once users enable this option, all devices that do not
> support MSI are still working. These devices are by default using
> IRQ pin assertion as interrupt generated mechanism since all
> pre-assigned vectors to any enabled IOxAPICs are reserved.

> Good, so we can always enable this option, so it doesn't even need
> to be an option :)
Agree. Will eliminate CONFIG_PCI_MSI to make this option dynamically
happens.

> A driver has to be modified to use this option, right? Do you have
> any drivers that have been modified? Without that, I don't think we
> can test this patch out, right?
> MSI support may be transparent to some device drivers.

> Yet you provide an api for drivers to use, right? Why would a driver
> want to use this api instead of just having the hardware work properly
> on its own? I guess I'm confused as to how to modify drivers to
> support this, or if that's even needed to be done.
There are two types of MSI capable devices: one supports the MSI
capability structure and other supports the MSI-X capability structure.
The patches provide two APIs msix_alloc_vectors/msix_free_vectors for
only devices, which support the MSI-X capability structure, to request
for additional messages. By default, each MSI/MSI-X capable device
function is allocated with one vector for below reasons:
- To achieve a backward compatibility with existing drivers if possible.
- Due to the current implementation of vector assignment, all devices
that support the MSI-capability structure work with no more than one
allocated vector.
- The hardware devices, which support the MSI-X capability structure,
may indicate the maximum capable number of vectors supported (32
vectors as example). However, the device drivers may require only
four. With provided APIs, the optimization of MSI vector allocation
is achievable.

The existing driver is required to be modified if and only if its
hardware/software synchronization is not provided; that means, the
hardware device may generate multiple messages from the same vector.
These messages may be lost; the results may be unpredictable. To
overcome this issue, the existing driver driver requires to provide
the hardware/software synchronization to ask its hardware device to
generate one message at a time.

> > +++ linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h 2003-08-05 09:25:54.000000000 -0400
> > @@ -76,9 +76,14 @@
> > * Since vectors 0x00-0x1f are used/reserved for the CPU,
> > * the usable vector space is 0x20-0xff (224 vectors)
> > */
> > +#define NR_VECTORS 256
>
> >Will this _always_ be the value? Can boxes have bigger numbers (I
> >haven't seen the spec, so I don't know...)
> Yes, this will always be 256 since the current CPU architecture
> supports a maximum of 256 vectors.

> The "current" one does. What happens in a few months when I go to help
> bring up the next gen hardware platform? Will I run into the problem
> that this number needs to be increased? Can we determine it at
> run-time?
I hope there may be a way to determine the number of vectors supported
by CPU during the run-time. I look at the file ../include/asm-i386/mach-
default/irq_vectors.h, the maximum of vectors (256) is already well
commented.

> > +u32 device_nomsi_list[DRIVER_NOMSI_MAX] = {0, };
>
> >Shouldn't this be static?
>
> > +u32 device_msi_list[DRIVER_NOMSI_MAX] = {0, };
>
> >Same with this one?
>
> In the initial design draft, I am thinking of defining
> DRIVER_NOMSI_MAX as 32 since it may be impossible to have more than 32
> MSI capable devices populated in the same system. I will add some
> comments to it and look forward for your comments.

> Um, I ment the variable is in the global symbol table, and it doesn't
> look like it needs to be.

> But your comment about number of devices is also a good one. Any way
> we can figure that out at run-time too? What happens when I hot-add
> the 33 MSI capable device? Or just boot with that many devices (I've
> seen boxes with 100s of pci devices all attached to one system)?
The defined variable DRIVER_NOMSI_MAX in device_msi_list[] and
device_nomsi_list[] are used to serve mainly as debug purposes since
most IHVs have not yet validated their hardware devices. If one of
the system has more than 32 MSI capable devices, users can enable MSI
on all devices by default (by setting CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES
to 'n') and use boot parameter "device_nomsi=" to disable some devices,
which have bugs in MSI. In summary, there are two options provided:
Option 1: Enable MSI on all MSI capable devices by default and use boot
parameter "device_nomsi=" to disable specific devices, which have bugs
in MSI.
This option is selected by setting CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES to
'n'. Note that if this option is chosen, then the use of "device_msi="
is no longer necessary.
Option 2 (default): Enable individual MSI capable devices as validation
purpose by using the boot parameter "device_msi=" to select which
devices the kernel supports MSI. Note that if this option is chosen,
then the use of "device_nomsi" id no longer necessary. This is by
default because users may not know how many MSI capable devices
populated in their systems and how many of these have bugs in MSI.

Either one of these options is chosen, I do not think you have any
issue of hot-adding the 33 MSI capable device.

Thanks,
Long

2003-08-11 21:19:54

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Mon, Aug 11, 2003 at 01:51:09PM -0700, long wrote:
> > Good, so we can always enable this option, so it doesn't even need
> > to be an option :)
> Agree. Will eliminate CONFIG_PCI_MSI to make this option dynamically
> happens.

Thank you.

> > A driver has to be modified to use this option, right? Do you have
> > any drivers that have been modified? Without that, I don't think we
> > can test this patch out, right?
> > MSI support may be transparent to some device drivers.
>
> > Yet you provide an api for drivers to use, right? Why would a driver
> > want to use this api instead of just having the hardware work properly
> > on its own? I guess I'm confused as to how to modify drivers to
> > support this, or if that's even needed to be done.
> There are two types of MSI capable devices: one supports the MSI
> capability structure and other supports the MSI-X capability structure.
> The patches provide two APIs msix_alloc_vectors/msix_free_vectors for
> only devices, which support the MSI-X capability structure, to request
> for additional messages. By default, each MSI/MSI-X capable device
> function is allocated with one vector for below reasons:
> - To achieve a backward compatibility with existing drivers if possible.
> - Due to the current implementation of vector assignment, all devices
> that support the MSI-capability structure work with no more than one
> allocated vector.
> - The hardware devices, which support the MSI-X capability structure,
> may indicate the maximum capable number of vectors supported (32
> vectors as example). However, the device drivers may require only
> four. With provided APIs, the optimization of MSI vector allocation
> is achievable.
>
> The existing driver is required to be modified if and only if its
> hardware/software synchronization is not provided; that means, the
> hardware device may generate multiple messages from the same vector.
> These messages may be lost; the results may be unpredictable. To
> overcome this issue, the existing driver driver requires to provide
> the hardware/software synchronization to ask its hardware device to
> generate one message at a time.

Ok, that makes a little more sense. I'm guessing that new hardware will
need to have their drivers modified to support this if it is needed.

> > > +++ linux-2.6.0-test2-create-vectorbase/include/asm-i386/mach-default/irq_vectors.h 2003-08-05 09:25:54.000000000 -0400
> > > @@ -76,9 +76,14 @@
> > > * Since vectors 0x00-0x1f are used/reserved for the CPU,
> > > * the usable vector space is 0x20-0xff (224 vectors)
> > > */
> > > +#define NR_VECTORS 256
> >
> > >Will this _always_ be the value? Can boxes have bigger numbers (I
> > >haven't seen the spec, so I don't know...)
> > Yes, this will always be 256 since the current CPU architecture
> > supports a maximum of 256 vectors.
>
> > The "current" one does. What happens in a few months when I go to help
> > bring up the next gen hardware platform? Will I run into the problem
> > that this number needs to be increased? Can we determine it at
> > run-time?
> I hope there may be a way to determine the number of vectors supported
> by CPU during the run-time. I look at the file ../include/asm-i386/mach-
> default/irq_vectors.h, the maximum of vectors (256) is already well
> commented.

Yeah, but that's in reference to APIC interrupt sources, right? Does
that correspond to these "vectors" too? If so, why not just use the
existing NR_IRQS value instead of creating your own?

> > > +u32 device_nomsi_list[DRIVER_NOMSI_MAX] = {0, };
> >
> > >Shouldn't this be static?
> >
> > > +u32 device_msi_list[DRIVER_NOMSI_MAX] = {0, };
> >
> > >Same with this one?
> >
> > In the initial design draft, I am thinking of defining
> > DRIVER_NOMSI_MAX as 32 since it may be impossible to have more than 32
> > MSI capable devices populated in the same system. I will add some
> > comments to it and look forward for your comments.
>
> > Um, I ment the variable is in the global symbol table, and it doesn't
> > look like it needs to be.
>
> > But your comment about number of devices is also a good one. Any way
> > we can figure that out at run-time too? What happens when I hot-add
> > the 33 MSI capable device? Or just boot with that many devices (I've
> > seen boxes with 100s of pci devices all attached to one system)?
> The defined variable DRIVER_NOMSI_MAX in device_msi_list[] and
> device_nomsi_list[] are used to serve mainly as debug purposes since
> most IHVs have not yet validated their hardware devices.

Heh, but what if I have 33 of the same kind of card in a machine (and
that card has already been "validated")?

> If one of the system has more than 32 MSI capable devices, users can
> enable MSI on all devices by default (by setting
> CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES to 'n') and use boot parameter
> "device_nomsi=" to disable some devices, which have bugs in MSI.

Ugh, I'm trying to get across the point that a user does _not_ want to
have to reconfig their kernel. Distros do not want to ship a whole
different kernel just for this one option. That's not acceptable if we
can handle everything dynamically with no problems for the small boxes
as well as the big ones.

> In summary, there are two options provided:
> Option 1: Enable MSI on all MSI capable devices by default and use boot
> parameter "device_nomsi=" to disable specific devices, which have bugs
> in MSI.

Ok, that's almost acceptable. How about fixing the bugs in MSI so this
is not needed? :)

> Option 2 (default): Enable individual MSI capable devices as validation
> purpose by using the boot parameter "device_msi=" to select which
> devices the kernel supports MSI.

Ick. How do you specify which "device" this option pertains to? Do I
have to know the pci id? What about pci hotplug boxes which decide to
re-arrange the pci ids every other time the box boots (yeah, I've seen
this, no fun...)

How about (if MSI is so troublesome), we just disable MSI all the time,
and provide a sysfs file for MSI to be "enabled" for every device if it
is wanted? The init scripts could go and enable this as needed. But
I'm guessing that you can't switch interrupt modes in a device after it
has been initialized, right? Oh well...

> Note that if this option is chosen,
> then the use of "device_nomsi" id no longer necessary. This is by
> default because users may not know how many MSI capable devices
> populated in their systems and how many of these have bugs in MSI.
>
> Either one of these options is chosen, I do not think you have any
> issue of hot-adding the 33 MSI capable device.

So then there is no limit on the number of MSI devices if
CONFIG_PCI_MSI_ON_SPECIFIC_DEVICES is not selected?

thanks,

greg k-h

2003-08-12 01:53:38

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Mon, 11 Aug 2003, Greg KH wrote:

> > There are two types of MSI capable devices: one supports the MSI
> > capability structure and other supports the MSI-X capability structure.
> > The patches provide two APIs msix_alloc_vectors/msix_free_vectors for
> > only devices, which support the MSI-X capability structure, to request
> > for additional messages. By default, each MSI/MSI-X capable device
> > function is allocated with one vector for below reasons:
> > - To achieve a backward compatibility with existing drivers if possible.
> > - Due to the current implementation of vector assignment, all devices
> > that support the MSI-capability structure work with no more than one
> > allocated vector.
> > - The hardware devices, which support the MSI-X capability structure,
> > may indicate the maximum capable number of vectors supported (32
> > vectors as example). However, the device drivers may require only
> > four. With provided APIs, the optimization of MSI vector allocation
> > is achievable.

IMO Multiplexing would be preferred, we can't be allocating that many
vectors for one device/device driver

> > I hope there may be a way to determine the number of vectors supported
> > by CPU during the run-time. I look at the file ../include/asm-i386/mach-
> > default/irq_vectors.h, the maximum of vectors (256) is already well
> > commented.
>
> Yeah, but that's in reference to APIC interrupt sources, right? Does
> that correspond to these "vectors" too? If so, why not just use the
> existing NR_IRQS value instead of creating your own?

There isn't a 1:1 relationship between NR_IRQS and NR_VECTORS we really
shouldn't mix them together. NR_IRQS can be much higher than 256 whilst
on i386 we're fixed to 256 vectors due to that being the Interrupt
Descriptor Table capacity. It is still possible to service more interrupt
lines than 256 on SMP however by making IDTs per 'interrupt servicing
node'

Zwane

2003-08-12 05:53:02

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Mon, Aug 11, 2003 at 09:41:30PM -0400, Zwane Mwaikambo wrote:
> > > I hope there may be a way to determine the number of vectors supported
> > > by CPU during the run-time. I look at the file ../include/asm-i386/mach-
> > > default/irq_vectors.h, the maximum of vectors (256) is already well
> > > commented.
> >
> > Yeah, but that's in reference to APIC interrupt sources, right? Does
> > that correspond to these "vectors" too? If so, why not just use the
> > existing NR_IRQS value instead of creating your own?
>
> There isn't a 1:1 relationship between NR_IRQS and NR_VECTORS we really
> shouldn't mix them together. NR_IRQS can be much higher than 256 whilst
> on i386 we're fixed to 256 vectors due to that being the Interrupt
> Descriptor Table capacity. It is still possible to service more interrupt
> lines than 256 on SMP however by making IDTs per 'interrupt servicing
> node'

Ah, so for processors other than i386, NR_VECTORS can be bigger?

I think a nice description is still needed for this value for just this
reason :)

thanks,

greg k-h

2003-08-12 15:23:16

by long

[permalink] [raw]

Subject: Re: Updated MSI Patches

> > > I hope there may be a way to determine the number of vectors supported
> > > by CPU during the run-time. I look at the file ../include/asm-i386/mach-
> > > default/irq_vectors.h, the maximum of vectors (256) is already well
> > > commented.
> >
> > Yeah, but that's in reference to APIC interrupt sources, right? Does
> > that correspond to these "vectors" too? If so, why not just use the
> > existing NR_IRQS value instead of creating your own?
>
> There isn't a 1:1 relationship between NR_IRQS and NR_VECTORS we really
> shouldn't mix them together. NR_IRQS can be much higher than 256 whilst
> on i386 we're fixed to 256 vectors due to that being the Interrupt
> Descriptor Table capacity. It is still possible to service more interrupt
> lines than 256 on SMP however by making IDTs per 'interrupt servicing
> node'

> Ah, so for processors other than i386, NR_VECTORS can be bigger?

> I think a nice description is still needed for this value for just this
> reason :)

Agree. Will put some comments describing this value.

Thanks,
Long

2003-08-12 17:04:49

[permalink] [raw]

Subject: RE: Updated MSI Patches

> diff -X excludes -urN linux-2.6.0-test2/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2/arch/i386/kernel/io_apic.c 2003-07-27 13:00:21.000000000 -0400
> +++ linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400
> @@ -76,6 +76,20 @@
> int apic, pin, next;
> } irq_2_pin[PIN_MAP_SIZE];
>
> +#ifdef CONFIG_PCI_USE_VECTOR
> +int vector_irq[NR_IRQS] = { [0 ... NR_IRQS -1] = -1};
> +
> +static int platform_irq(int irq)
> +{
> + if (platform_legacy_irq(irq))
> + return irq;
> + else
> + return vector_irq[irq];
> +}
> +#else
> +#define platform_irq(irq) (irq)
> +#endif

> I wish you wouldn't mix up vector and irq like this :( This is really
> beginning to look like IPF. When is an irq an irq and when is a vector a
> vector? This can get very confusing very quickly. I spent a fair
> amount of time debugging this last time due to the confusion in the
> common_interrupt -> do_IRQ path, i'd hate for this to be a permanent
> fixture. If you're going to change things just change everything
> (including variable names) instead of making it conditional.
I understand that mixing up vector and irq is very confusing. However, to support non-PCI legacy devices with IRQ less than 16, such as keyboard and mouse for example, may be impossilbe to achieve without mixing up. Some existing driver of legacy keyboard/mouse devices, for example, may use fixed IO ranges and fixed IRQs (as assigned to 1 for keyboard and 12 for mouse). If these device drivers use these fixed legacy IRQs and the interrupt routings for these non-PCI legacy devices use vectors, then the system may break. As you know, MSI support requires vector allocation instead of IRQ allocation since MSI does not require a support of BIOS IRQ table. Mixing vector with IRQ to be compatible with non-PCI legacy devices must be achieved. Last time, your suggestion of changing variable name from irq to vector is the good approach. I am looking at restructuring the code of the vector-base patch. I will send you an update when I am done for your feedback.

> diff -X excludes -urN linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c linux-2.6.0-test2-create-msi/arch/i386/kernel/io_apic.c
> --- linux-2.6.0-test2-create-vectorbase/arch/i386/kernel/io_apic.c 2003-08-05 09:25:54.000000000 -0400
> +++ linux-2.6.0-test2-create-msi/arch/i386/kernel/io_apic.c 2003-08-05 09:45:25.000000000 -0400
> @@ -427,6 +427,11 @@
> /* Is this an active IRQ? */
> if (!irq_desc[j].action)
> continue;
> +#ifdef CONFIG_PCI_MSI
> + /* Is this an active MSI? */
> + if (msi_desc[j])
> + continue;
> +#endif

> Is there any locking for msi_desc? Or are you relying on something else?
Since the code determinces whether this entry is NULL or not, I think any locking for msi_desc may not be required.

> + if (current_vector == FIRST_SYSTEM_VECTOR)
> + panic("ran out of interrupt sources!");

> Please just fail and return -ENOSPC or something like that.
Good. Thanks!

Thanks,
Long

2003-08-12 17:32:43

[permalink] [raw]

Subject: RE: Updated MSI Patches

> > There are two types of MSI capable devices: one supports the MSI
> > capability structure and other supports the MSI-X capability structure.
> > The patches provide two APIs msix_alloc_vectors/msix_free_vectors for
> > only devices, which support the MSI-X capability structure, to request
> > for additional messages. By default, each MSI/MSI-X capable device
> > function is allocated with one vector for below reasons:
> > - To achieve a backward compatibility with existing drivers if possible.
> > - Due to the current implementation of vector assignment, all devices
> > that support the MSI-capability structure work with no more than one
> > allocated vector.
> > - The hardware devices, which support the MSI-X capability structure,
> > may indicate the maximum capable number of vectors supported (32
> > vectors as example). However, the device drivers may require only
> > four. With provided APIs, the optimization of MSI vector allocation
> > is achievable.

> IMO Multiplexing would be preferred, we can't be allocating that many
> vectors for one device/device driver
All pre-assigned vectors to all enabled IOxAPIC(s) are reserved.
Allocating additional vectors to MSI-X driver is determined based on the
available vectors, which must be greater than the number of vectors requested
by MSI-X driver.

Thanks,
Long

2003-08-12 18:23:21

[permalink] [raw]

Subject: RE: Updated MSI Patches

Your message would be easier to reply to and read if it was wrapped at 72
characters, please look into fixing that for the sake of your recipients.
Thanks.

On Tue, 12 Aug 2003, Nguyen, Tom L wrote:

> I understand that mixing up vector and irq is very confusing. However,
> to support non-PCI legacy devices with IRQ less than 16, such as
> keyboard and mouse for example, may be impossilbe to achieve without
> mixing up. Some existing driver of legacy keyboard/mouse devices, for
> example, may use fixed IO ranges and fixed IRQs (as assigned to 1 for
> keyboard and 12 for mouse). If these device drivers use these fixed
> legacy IRQs and the interrupt routings for these non-PCI legacy devices
> use vectors, then the system may break. As you know, MSI support
> requires vector allocation instead of IRQ allocation since MSI does not
> require a support of BIOS IRQ table. Mixing vector with IRQ to be
> compatible with non-PCI legacy devices must be achieved. Last time,
> your suggestion of changing variable name from irq to vector is the
> good approach. I am looking at restructuring the code of the
> vector-base patch. I will send you an update when I am done for your
> feedback.

For the legacy devices, an irq_vector mapping should be sufficient as you
currently maintain. You could also possibly have a seperate do_MSI which
gets a vector pushed to it and then uses mesc_desc instead to do the
handlers. You then install the IDT entry using the seperate MSI interrupt
stub; set_intr_gate(vector, msi_interrupt[msi_num]) where msi_interrupt is
a NR_MSI sized array. This way you can avoid touching do_IRQ entirely.

> Since the code determinces whether this entry is NULL or not, I think any
> locking for msi_desc may not be required.

Yes but there is other code which modifies msi_desc members. i think a per
msi_desc lock is needed. You could also use a kmem_cache to allocate them,
and perhaps utilise HWCACHE_ALIGN.

Zwane

2003-08-12 18:36:26

[permalink] [raw]

Subject: RE: Updated MSI Patches

> For the legacy devices, an irq_vector mapping should be sufficient as
you
> currently maintain. You could also possibly have a seperate do_MSI
which
> gets a vector pushed to it and then uses mesc_desc instead to do the
> handlers. You then install the IDT entry using the seperate MSI
interrupt
> stub; set_intr_gate(vector, msi_interrupt[msi_num]) where
msi_interrupt is
> a NR_MSI sized array. This way you can avoid touching do_IRQ entirely.

The issue with do_MSI() approach is that it's very similar to do_IRQ(),
and we may have maintenance issues there. However, if we make a common
do_MSI() code, that might be worth it, and I would expect much fewer
architecture-dependent issues there, compared to do_IRQ (the common
do_IRQ() hasn't happened yet as far as I know).

Thanks,
Jun

> -----Original Message-----
> From: Zwane Mwaikambo [mailto:[email protected]]
> Sent: Tuesday, August 12, 2003 11:11 AM
> To: Nguyen, Tom L
> Cc: Linux Kernel; Nakajima, Jun; long
> Subject: RE: Updated MSI Patches
>
> Your message would be easier to reply to and read if it was wrapped at
72
> characters, please look into fixing that for the sake of your
recipients.
> Thanks.
>
> On Tue, 12 Aug 2003, Nguyen, Tom L wrote:
>
> > I understand that mixing up vector and irq is very confusing.
However,
> > to support non-PCI legacy devices with IRQ less than 16, such as
> > keyboard and mouse for example, may be impossilbe to achieve without
> > mixing up. Some existing driver of legacy keyboard/mouse devices,
for
> > example, may use fixed IO ranges and fixed IRQs (as assigned to 1
for
> > keyboard and 12 for mouse). If these device drivers use these fixed
> > legacy IRQs and the interrupt routings for these non-PCI legacy
devices
> > use vectors, then the system may break. As you know, MSI support
> > requires vector allocation instead of IRQ allocation since MSI does
not
> > require a support of BIOS IRQ table. Mixing vector with IRQ to be
> > compatible with non-PCI legacy devices must be achieved. Last time,
> > your suggestion of changing variable name from irq to vector is the
> > good approach. I am looking at restructuring the code of the
> > vector-base patch. I will send you an update when I am done for your
> > feedback.
>
> For the legacy devices, an irq_vector mapping should be sufficient as
you
> currently maintain. You could also possibly have a seperate do_MSI
which
> gets a vector pushed to it and then uses mesc_desc instead to do the
> handlers. You then install the IDT entry using the seperate MSI
interrupt
> stub; set_intr_gate(vector, msi_interrupt[msi_num]) where
msi_interrupt is
> a NR_MSI sized array. This way you can avoid touching do_IRQ entirely.
>
> > Since the code determinces whether this entry is NULL or not, I
think
> any
> > locking for msi_desc may not be required.
>
> Yes but there is other code which modifies msi_desc members. i think a
per
> msi_desc lock is needed. You could also use a kmem_cache to allocate
them,
> and perhaps utilise HWCACHE_ALIGN.
>
> Zwane

2003-08-12 18:25:54

[permalink] [raw]

Subject: RE: Updated MSI Patches

On Tue, 12 Aug 2003, Nguyen, Tom L wrote:

> > IMO Multiplexing would be preferred, we can't be allocating that many
> > vectors for one device/device driver
> All pre-assigned vectors to all enabled IOxAPIC(s) are reserved.
> Allocating additional vectors to MSI-X driver is determined based on the
> available vectors, which must be greater than the number of vectors requested
> by MSI-X driver.

Hmm can you avoid grabbing all the free vectors? I think the irq
controller subsystem should handle allocation of vectors. Letting MSI grab
everything might leave us with problems later on.

Zwane

2003-08-12 18:49:12

by Jeff Garzik

[permalink] [raw]

Subject: Re: Updated MSI Patches

Nakajima, Jun wrote:
> The issue with do_MSI() approach is that it's very similar to do_IRQ(),
> and we may have maintenance issues there. However, if we make a common

I agree

> do_MSI() code, that might be worth it, and I would expect much fewer
> architecture-dependent issues there, compared to do_IRQ (the common
> do_IRQ() hasn't happened yet as far as I know).

However, we have maintenance issues in this area as well :)

If you look at each architecture's implementation of do_IRQ, you can see
each implementation is strikingly similar... except for some subtle
differences. So there are arguments both ways: creating a common
do_IRQ may add maintenance value... but also create corner-case
problems for the arch maintainers.

So, IMO, do_IRQ is one special case where copying code may be preferred
over common code.

And I also feel the same way about do_MSI(). However, I have not looked
at non-ia32 MSI implementations to know what sort of issues exist.

Jeff

2003-08-12 19:26:20

[permalink] [raw]

Subject: Re: Updated MSI Patches

On Tue, 12 Aug 2003, Jeff Garzik wrote:

> So, IMO, do_IRQ is one special case where copying code may be preferred
> over common code.
>
> And I also feel the same way about do_MSI(). However, I have not looked
> at non-ia32 MSI implementations to know what sort of issues exist.

The main reason i have a preference for a seperate MSI handling path is so
that we don't have to do the platform_irq thing in do_IRQ and we know
what to expect wrt irq or vector. If platform_irq stays we should at
least try and pick up on what the IA64 folks have done, But that would be
even harder to get done right now.

Zwane

2003-08-12 19:45:24

by Jeff Garzik

[permalink] [raw]

Subject: Re: Updated MSI Patches

Zwane Mwaikambo wrote:
> On Tue, 12 Aug 2003, Jeff Garzik wrote:
>
>
>>So, IMO, do_IRQ is one special case where copying code may be preferred
>>over common code.
>>
>>And I also feel the same way about do_MSI(). However, I have not looked
>>at non-ia32 MSI implementations to know what sort of issues exist.
>
>
> The main reason i have a preference for a seperate MSI handling path is so
> that we don't have to do the platform_irq thing in do_IRQ and we know
> what to expect wrt irq or vector. If platform_irq stays we should at
> least try and pick up on what the IA64 folks have done, But that would be
> even harder to get done right now.

Oh, I definitely prefer a separate MSI handling path too.

In the future we'll be writing drivers that _require_ MSI interrupt
handling, and we'll be optimizing the various MSI hot paths to reclaim
even the most minute amount of CPU cycles. And we want to escape any
shackles the evil phrase "legacy interrupts" dares to try to lay upon us.

But there is a flip side to that: do_IRQ is not solely hardware
interrupts. That area of code is central dispatcher for
softirq/tasklet/timer delivery as well. So a separate do_MSI() needs to
take that stuff into account.

Overall, I'm pretty happy with how Tom's MSI patches are going so far,
and he seems to be responding to feedback. So, we'll get there.

Jeff

2003-08-12 23:59:31

[permalink] [raw]

Subject: RE: Updated MSI Patches

If the platform_irq thing is the issue, we can remove it by having like

#ifdef PCI_USE_VECTOR
static struct hw_interrupt_type ioapic_edge_vector_type = {
.typename = "IO-APIC-edge",
.startup = startup_edge_ioapic_vector,
.shutdown = shutdown_edge_ioapic_vector,
.enable = enable_edge_ioapic_vector,
.disable = disable_edge_ioapic_vector,
.ack = ack_edge_ioapic_vector,
.end = end_edge_ioapic_vector,
.set_affinity = set_ioapic_affinity,
};

static struct hw_interrupt_type ioapic_level_vector_type = {
...

#else
...

They (startup_edge_ioapic_vector, etc.) convert the vector to IRQ when
needed. Note that we need that because interrupt controller(s) still
needs IRQ info for the conventional interrupts when we use vector-based
do_IRQ().

For example, startup_edge_ioapic_vector() looks like this:

static unsigned int startup_edge_ioapic_vector(unsigned int vector)
{
int was_pending = 0;
unsigned long flags;
int irq = vector_to_irq (vector);

spin_lock_irqsave(&ioapic_lock, flags);
if (irq < 16) {
disable_8259A_irq(irq);
if (i8259A_irq_pending(irq))
was_pending = 1;
}
__unmask_IO_APIC_irq(irq);
spin_unlock_irqrestore(&ioapic_lock, flags);

return was_pending;
}

Thanks,
Jun

> -----Original Message-----
> From: Jeff Garzik [mailto:[email protected]]
> Sent: Tuesday, August 12, 2003 12:45 PM
> To: Zwane Mwaikambo
> Cc: Nakajima, Jun; Nguyen, Tom L; Linux Kernel; long
> Subject: Re: Updated MSI Patches
>
> Zwane Mwaikambo wrote:
> > On Tue, 12 Aug 2003, Jeff Garzik wrote:
> >
> >
> >>So, IMO, do_IRQ is one special case where copying code may be
preferred
> >>over common code.
> >>
> >>And I also feel the same way about do_MSI(). However, I have not
looked
> >>at non-ia32 MSI implementations to know what sort of issues exist.
> >
> >
> > The main reason i have a preference for a seperate MSI handling path
is
> so
> > that we don't have to do the platform_irq thing in do_IRQ and we
know
> > what to expect wrt irq or vector. If platform_irq stays we should at
> > least try and pick up on what the IA64 folks have done, But that
would
> be
> > even harder to get done right now.
>
>
> Oh, I definitely prefer a separate MSI handling path too.
>
> In the future we'll be writing drivers that _require_ MSI interrupt
> handling, and we'll be optimizing the various MSI hot paths to reclaim
> even the most minute amount of CPU cycles. And we want to escape any
> shackles the evil phrase "legacy interrupts" dares to try to lay upon
us.
>
> But there is a flip side to that: do_IRQ is not solely hardware
> interrupts. That area of code is central dispatcher for
> softirq/tasklet/timer delivery as well. So a separate do_MSI() needs
to
> take that stuff into account.
>
> Overall, I'm pretty happy with how Tom's MSI patches are going so far,
> and he seems to be responding to feedback. So, we'll get there.
>
> Jeff
>
>

2003-08-13 01:00:51

[permalink] [raw]

Subject: RE: Updated MSI Patches

On Tue, 12 Aug 2003, Nakajima, Jun wrote:

> static unsigned int startup_edge_ioapic_vector(unsigned int vector)
> {
> int was_pending = 0;
> unsigned long flags;
> int irq = vector_to_irq (vector);
>
> spin_lock_irqsave(&ioapic_lock, flags);
> if (irq < 16) {
> disable_8259A_irq(irq);
> if (i8259A_irq_pending(irq))
> was_pending = 1;
> }
> __unmask_IO_APIC_irq(irq);
> spin_unlock_irqrestore(&ioapic_lock, flags);
>
> return was_pending;
> }

Hmm that doesn't look too bad, i like, ok another question, do you think
this could be made to scale on systems with lots of devices requiring
vectors?

Thanks,
Zwane

2003-08-13 01:34:34

[permalink] [raw]

Subject: RE: Updated MSI Patches

Salability means many things. I'm not sure which aspect you are talking
about, but a good thing with MSI is that it does not depend on I/O
APICs. A typical I/O APIC has 24 RTEs, and we need about 10 I/O APICs to
consume the vectors.

You can make this scale on SMP systems. The vector-based approach would
be easier to extend it per-CPU because vectors are inherently per-CPU,
compared with IRQ-based. Today we have IRQ balancing as you know, and
the key part is to bind IRQ to a particular CPU (BTW, with our patch it
happens automatically because the balancer does not care if they are IRQ
or vector).

Thanks,
Jun

> -----Original Message-----
> From: Zwane Mwaikambo [mailto:[email protected]]
> Sent: Tuesday, August 12, 2003 5:49 PM
> To: Nakajima, Jun
> Cc: Jeff Garzik; Nguyen, Tom L; Linux Kernel; long
> Subject: RE: Updated MSI Patches
>
> On Tue, 12 Aug 2003, Nakajima, Jun wrote:
>
> > static unsigned int startup_edge_ioapic_vector(unsigned int vector)
> > {
> > int was_pending = 0;
> > unsigned long flags;
> > int irq = vector_to_irq (vector);
> >
> > spin_lock_irqsave(&ioapic_lock, flags);
> > if (irq < 16) {
> > disable_8259A_irq(irq);
> > if (i8259A_irq_pending(irq))
> > was_pending = 1;
> > }
> > __unmask_IO_APIC_irq(irq);
> > spin_unlock_irqrestore(&ioapic_lock, flags);
> >
> > return was_pending;
> > }
>
> Hmm that doesn't look too bad, i like, ok another question, do you
think
> this could be made to scale on systems with lots of devices requiring
> vectors?
>
> Thanks,
> Zwane

2003-08-13 01:48:58

[permalink] [raw]

Subject: RE: Updated MSI Patches

On Tue, 12 Aug 2003, Nakajima, Jun wrote:

> Salability means many things. I'm not sure which aspect you are talking
> about, but a good thing with MSI is that it does not depend on I/O
> APICs. A typical I/O APIC has 24 RTEs, and we need about 10 I/O APICs to
> consume the vectors.

Yes there are already a number of systems which exhibit this problem (see
the URL in the next comment)

> You can make this scale on SMP systems. The vector-based approach would
> be easier to extend it per-CPU because vectors are inherently per-CPU,
> compared with IRQ-based. Today we have IRQ balancing as you know, and
> the key part is to bind IRQ to a particular CPU (BTW, with our patch it
> happens automatically because the balancer does not care if they are IRQ
> or vector).

Sorry, yeah i meant device scalability essentially, the ability to plug in
a fair amount of these and other devices into the same system. The per cpu
IDT route is also worthwhile following a bit later on. I've already
written something along the lines of a per node IDT (you could make the
granularity per cpu of course) if you're interested you can have a look
here http://www.osdl.org/projects/numaqhwspprt/results/patch-numaq-highirq10 it
also has a companion patch to dynamically allocate the irq_desc's which
applies on top of it http://function.linuxpower.ca/patches/patch-dynirq-wli

But back to the subject, your patch looks good, i'll test it for
regression (no MSI devices but regression testing is just as important)
when you next post.

Thanks!
Zwane

2003-08-13 04:14:43

[permalink] [raw]

Subject: RE: Updated MSI Patches

> But back to the subject, your patch looks good, i'll test it for
> regression (no MSI devices but regression testing is just as
important)
> when you next post.

Thanks. Long will make another patch that reflects my last comments.

Jun

> -----Original Message-----
> From: Zwane Mwaikambo [mailto:[email protected]]
> Sent: Tuesday, August 12, 2003 6:37 PM
> To: Nakajima, Jun
> Cc: Jeff Garzik; Nguyen, Tom L; Linux Kernel; long
> Subject: RE: Updated MSI Patches
>
> On Tue, 12 Aug 2003, Nakajima, Jun wrote:
>
> > Salability means many things. I'm not sure which aspect you are
talking
> > about, but a good thing with MSI is that it does not depend on I/O
> > APICs. A typical I/O APIC has 24 RTEs, and we need about 10 I/O
APICs to
> > consume the vectors.
>
> Yes there are already a number of systems which exhibit this problem
(see
> the URL in the next comment)
>
> > You can make this scale on SMP systems. The vector-based approach
would
> > be easier to extend it per-CPU because vectors are inherently
per-CPU,
> > compared with IRQ-based. Today we have IRQ balancing as you know,
and
> > the key part is to bind IRQ to a particular CPU (BTW, with our patch
it
> > happens automatically because the balancer does not care if they are
IRQ
> > or vector).
>
> Sorry, yeah i meant device scalability essentially, the ability to
plug in
> a fair amount of these and other devices into the same system. The per
cpu
> IDT route is also worthwhile following a bit later on. I've already
> written something along the lines of a per node IDT (you could make
the
> granularity per cpu of course) if you're interested you can have a
look
> here http://www.osdl.org/projects/numaqhwspprt/results/patch-numaq-
> highirq10 it
> also has a companion patch to dynamically allocate the irq_desc's
which
> applies on top of it
http://function.linuxpower.ca/patches/patch-dynirq-
> wli
>
> But back to the subject, your patch looks good, i'll test it for
> regression (no MSI devices but regression testing is just as
important)
> when you next post.
>
> Thanks!
> Zwane

2003-08-13 15:19:13

[permalink] [raw]

Subject: RE: Updated MSI Patches

>> Salability means many things. I'm not sure which aspect you are talking
>> about, but a good thing with MSI is that it does not depend on I/O
>> APICs. A typical I/O APIC has 24 RTEs, and we need about 10 I/O APICs to
>> consume the vectors.

> Yes there are already a number of systems which exhibit this problem (see
> the URL in the next comment)

>> You can make this scale on SMP systems. The vector-based approach would
>> be easier to extend it per-CPU because vectors are inherently per-CPU,
>> compared with IRQ-based. Today we have IRQ balancing as you know, and
>> the key part is to bind IRQ to a particular CPU (BTW, with our patch it
>> happens automatically because the balancer does not care if they are IRQ
>> or vector).

>Sorry, yeah i meant device scalability essentially, the ability to plug in
>a fair amount of these and other devices into the same system. The per cpu
>IDT route is also worthwhile following a bit later on. I've already
>written something along the lines of a per node IDT (you could make the
>granularity per cpu of course) if you're interested you can have a look
>here http://www.osdl.org/projects/numaqhwspprt/results/patch-numaq-highirq10 it
>also has a companion patch to dynamically allocate the irq_desc's which
>applies on top of it http://function.linuxpower.ca/patches/patch-dynirq-wli

>But back to the subject, your patch looks good, i'll test it for
>regression (no MSI devices but regression testing is just as important)
>when you next post.
I already had discussion with Jun about this change. We will post it as soon as
we validate this change on our system.

Thanks,
Long

2003-08-14 23:46:58

[permalink] [raw]

Subject: RE: Updated MSI Patches

On Tue, 12 Aug 2003, Nguyen, Tom L wrote:
> > IMO Multiplexing would be preferred, we can't be allocating that many
> > vectors for one device/device driver
> All pre-assigned vectors to all enabled IOxAPIC(s) are reserved.
> Allocating additional vectors to MSI-X driver is determined based on the
> available vectors, which must be greater than the number of vectors requested
> by MSI-X driver.

>Hmm can you avoid grabbing all the free vectors? I think the irq
>controller subsystem should handle allocation of vectors. Letting MSI grab
>everything might leave us with problems later on.
Good point. Thanks! we are studying the policy, which ensures that no MSI-X
can grab everything from others. Will post this changes after done ...

Thanks,
Long

2003-08-19 15:42:16