| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1161658168.2149.64.camel@localhost.localdomain>
Date: Tue, 24 Oct 2006 12:49:27 +1000
From: Michael Ellerman <michael@...erman.id.au>
To: Jake Moilanen <moilanen@...tin.ibm.com>
Cc: linux-kernel@...r.kernel.org,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
linuxppc-dev@...abs.org,
"Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [RFC/PATCH 6/7] RTAS MSI implementation
On Tue, 2006-10-03 at 16:53 -0500, Jake Moilanen wrote:
> On Fri, 2006-09-29 at 07:53 +1000, Michael Ellerman wrote:
> > Powerpc MSI support via RTAS. Based on Jake's code.
> >
> > Signed-off-by: Michael Ellerman <michael@...erman.id.au>
> > ---
> >
> > arch/powerpc/kernel/Makefile | 1
> > arch/powerpc/kernel/msi-rtas.c | 246 +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 247 insertions(+)
> >
> > Index: to-merge/arch/powerpc/kernel/Makefile
> > ===================================================================
> > --- to-merge.orig/arch/powerpc/kernel/Makefile
> > +++ to-merge/arch/powerpc/kernel/Makefile
> > @@ -69,6 +69,7 @@ pci32-$(CONFIG_PPC32) := pci_32.o
> > obj-$(CONFIG_PCI) += $(pci64-y) $(pci32-y)
> >
> > msiobj-y := msi.o
> > +msiobj-$(CONFIG_PPC_PSERIES) += msi-rtas.o
> > obj-$(CONFIG_PCI_MSI) += $(msiobj-y)
> >
> > kexec-$(CONFIG_PPC64) := machine_kexec_64.o
> > Index: to-merge/arch/powerpc/kernel/msi-rtas.c
> > ===================================================================
> > --- /dev/null
> > +++ to-merge/arch/powerpc/kernel/msi-rtas.c
> > @@ -0,0 +1,246 @@
> > +/*
> > + * Copyright (C) 2006 Jake Moilanen <moilanen@...tin.ibm.com>, IBM Corp.
> > + * Copyright (C) 2006 Michael Ellerman, IBM Corp.
> > + *
> > + * This program 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; version 2 of the
> > + * License.
> > + *
> > + */
> > +
> > +#define DEBUG 1
> > +
> > +#include <linux/irq.h>
> > +#include <asm/msi.h>
> > +#include <asm/rtas.h>
> > +#include <asm/hw_irq.h>
> > +#include <asm/ppc-pci.h>
> > +
> > +static int query_token, change_token;
> > +
> > +#define RTAS_QUERY_MSI_FN 0
> > +#define RTAS_CHANGE_MSI_FN 1
> > +#define RTAS_RESET_MSI_FN 2
> > +
> > +
> > +/* RTAS Helpers */
> > +
> > +static int rtas_change_msi(struct pci_dn *pdn, u32 function, u32 num_irqs)
> > +{
> > + u32 addr, seq_num, rtas_ret[2];
> > + unsigned long buid;
> > + int rc;
> > +
> > + addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
> > + buid = pdn->phb->buid;
> > +
> > + seq_num = 1;
> > + do {
> > + rc = rtas_call(change_token, 6, 3, rtas_ret, addr,
> > + BUID_HI(buid), BUID_LO(buid),
> > + function, num_irqs, seq_num);
>
> This call is still currently broken in firmware. Hopefully we'll have a
> resolution soon.
Any word on that?
> > +
> > + seq_num = rtas_ret[1];
> > + } while (rtas_busy_delay(rc));
> > +
> > + if (rc) {
> > + printk(KERN_WARNING "Error[%d]: getting the number of"
> > + " MSI interrupts for %s\n", rc, pci_name(pdn->pcidev));
> > + return rc;
> > + }
> > +
> > + return rtas_ret[0];
> > +}
> > +
> > +static int rtas_query_irq_number(struct pci_dn *pdn, int offset)
> > +{
> > + u32 addr, rtas_ret[2];
> > + unsigned long buid;
> > + int rc;
> > +
> > + addr = rtas_config_addr(pdn->busno, pdn->devfn, 0);
> > + buid = pdn->phb->buid;
> > +
> > + do {
> > + rc = rtas_call(query_token, 4, 3, rtas_ret, addr,
> > + BUID_HI(buid), BUID_LO(buid), offset);
> > + } while (rtas_busy_delay(rc));
> > +
> > + if (rc) {
> > + printk(KERN_WARNING "Error[%d]: Querying irq source number "
> > + "for %s\n", rc, pci_name(pdn->pcidev));
> > + return rc;
> > + }
> > +
> > + return rtas_ret[0];
> > +}
> > +
> > +/*
> > + * The spec gives firmware the option to enable either MSI or MSI-X,
> > + * this doesn't wash with the Linux API. For the time beinging, we
> > + * kludge around that by checking ourselves the right type is enabled.
> > + */
> > +static int check_msi_type(struct pci_dev *pdev, int type)
> > +{
> > + int pos, msi_enabled, msix_enabled;
> > + u16 reg;
> > +
> > + pos = pci_find_capability(pdev, PCI_CAP_ID_MSI);
> > + if (!pos)
> > + return -1;
> > +
> > + pci_read_config_word(pdev, pos + PCI_MSI_FLAGS, ®);
> > +
> > + msi_enabled = msix_enabled = 0;
> > +
> > + if (reg & PCI_MSI_FLAGS_ENABLE)
> > + msi_enabled = 1;
> > +
>
> This is not being set correctly by firmware either. I have them looking
> into the problem.
Are you sure? I don't see how MSI could possibly work if they don't
enable it on the device itself. Or is it completely broken at the
moment?
>
> > + if (reg & PCI_MSIX_FLAGS_ENABLE)
> > + msix_enabled = 1;
> > +
> > + if (type == PCI_CAP_ID_MSI && (msix_enabled || !msi_enabled)) {
> > + pr_debug("check_msi_type: Expected MSI but got %s.\n",
> > + msix_enabled ? "MSI-X" : "none");
> > + return -1;
> > + }
> > +
> > + if (type == PCI_CAP_ID_MSIX && (msi_enabled || !msix_enabled)) {
> > + pr_debug("check_msi_type: Expected MSI-X but got %s.\n",
> > + msi_enabled ? "MSI" : "none");
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void msi_rtas_free(struct pci_dev *pdev, int num,
> > + struct msix_entry *entries, int type)
> > +{
> > + struct device_node *dn;
> > + struct pci_dn *pdn;
> > + int i;
> > +
> > + dn = pci_device_to_OF_node(pdev);
> > + if (!dn) {
> > + pr_debug("msi_rtas_free: No OF device node for %s\n",
> > + pci_name(pdev));
> > + return;
> > + }
> > +
> > + pdn = PCI_DN(dn);
> > + if (!pdn) {
> > + pr_debug("msi_rtas_free: No PCI DN for %s\n",
> > + pci_name(pdev));
> > + return;
> > + }
> > +
> > + for (i = 0; i < num; i++) {
> > + irq_dispose_mapping(entries[i].vector);
> > + }
> > +
> > + /* XXX can we do anything sane if this fails? */
> > + rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, 0);
> > +}
> > +
> > +static int msi_rtas_check(struct pci_dev *pdev, int num,
> > + struct msix_entry *entries, int type)
> > +{
> > + struct device_node *dn;
> > + int i;
> > +
> > + dn = pci_device_to_OF_node(pdev);
> > +
> > + if (!of_find_property(dn, "ibm,req#msi", NULL)) {
> > + pr_debug("msi_rtas_check: No ibm,req#msi for %s\n",
> > + pci_name(pdev));
> > + return -1;
> > + }
> > +
> > + /*
> > + * Firmware gives us no control over which entries are allocated
> > + * for MSI-X, it seems to assume we want 0 - n. For now just insist
> > + * that the entries array entry members are 0 - n.
> > + */
> > + for (i = 0; i < num; i++) {
> > + if (entries[i].entry != i) {
> > + pr_debug("msi_rtas_check: entries[i].entry != i\n");
> > + return -1;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int msi_rtas_alloc(struct pci_dev *pdev, int num,
> > + struct msix_entry *entries, int type)
> > +{
> > + struct pci_dn *pdn;
> > + int hwirq, virq, i;
> > +
> > + pdn = PCI_DN(pci_device_to_OF_node(pdev));
> > +
> > + /*
> > + * In the case of an error it's not clear whether the device is left
> > + * with MSI enabled or not, I think we should explicitly disable.
> > + */
> > + if (rtas_change_msi(pdn, RTAS_CHANGE_MSI_FN, num) != num)
> > + goto out_free;
> > +
> > + if (check_msi_type(pdev, type))
> > + goto out_free;
> > +
> > + for (i = 0; i < num; i++) {
> > + hwirq = rtas_query_irq_number(pdn, i);
> > + if (hwirq < 0)
> > + goto out_free;
> > +
> > + virq = irq_create_mapping(NULL, hwirq);
> > +
> > + if (virq == NO_IRQ) {
> > + pr_debug("msi_rtas_alloc: Failed mapping hwirq %d\n",
> > + hwirq);
> > + goto out_free;
> > + }
> > +
> > + entries[i].vector = virq;
> > + }
> > +
> > + return 0;
> > +
> > + out_free:
> > + msi_rtas_free(pdev, num, entries, type);
>
> Shouldn't this be:
>
> msi_rtas_free(pdev, i,.......
>
> Otherwise you'll try freeing unallocated entries.
Yeah maybe. I think I was intending that free be written in such a way
that it could cope with tearing down a partial setup, but I never got
round to writing the patch for irq_dispose_mapping() to make that work -
I should do that.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
Download attachment "signature.asc" of type "application/pgp-signature" (192 bytes)
Powered by blists - more mailing lists