LKML Archive on
help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <>
To: David Miller <>
Subject: Re: [PATCH 0/6] MSI portability cleanups
Date: Mon, 29 Jan 2007 17:05:37 +1100	[thread overview]
Message-ID: <1170050737.26655.233.camel@localhost.localdomain> (raw)
In-Reply-To: <>

On Sun, 2007-01-28 at 21:25 -0800, David Miller wrote:
> From: (Eric W. Biederman)
> Date: Sun, 28 Jan 2007 22:18:59 -0700
> > Regardless of my opinion on the sanity of the hypervisor architects.
> > I have not seen anything that indicates it will be hard to support
> > the hypervisor doing everything or most of everything for us, so
> > I see no valid technical objection to it.  Nor have I ever.
> > 
> > So I have no problem with additional patches in that direction.
> Ok, that's great to hear.
> I know your bi-directional approach isn't exactly what Ben
> wants but he can support his machines with it.  Maybe after
> some time we can agree to move from that more towards the
> totally abstracted scheme.

It can support my machines without HV with trivial changes I reckon: I
need an ops struct to indirect eric's 2 remaining arch hooks
(setup/teardown) but that can be done inline within asm-powerpc. I need
to double check of course and probably actually port the MPIC backend
and possibly go write the Cell Axon one while at it to verify everything
is allright, but the base design seems sound enough.

For the ones with HV (RTAS stuff), we still need to agree on how to
approach it. We can either:

Option 1

Do a hook -above- Eric stuff, by having the toplevel APIs themselves be
arch hooks that can either go toward the RTAS implementation or toward
Eric's code. That is, eric code would define those (pick better names if
you are good at it):


Then we can have asm-i386/msi.h & friends do something like

#define pci_enable_msi	pci_generic_enable_msi
#define pci_disable_msi	pci_generic_disable_msi

And we can have asm-powerpc/msi.h hook then via ppc_md:

static inline int pci_enable_msi(xxx...)
	return ppc_md.pci_enable_msi(xxx...);

(ppc_md is our per-platform global hook structure filled at boot when we
discover on what machine type we are running on) so that pSeries can use
it's own RTAS callbacks, and others can just re-hook those to Eric's

Option 2

That is to make Eric's code itself cope with the HV case. I'm a bit at
loss right now as how precisely to do it. I need to spend more time
staring at the code after Eric latest patches rather than the patches
themselves I suppose :-) (Eric, they don't apply out of the box on
current git, they are against -mm ?).

Some of the main issues here, more/less following the order in which
Eric code calls things:

 - The number of vectors for MSI-X is obtained from config space (at
least for sanity checking the requested argument). On RTAS, it should
come from an OF property (we are really not supposed to go read the
config space even if we can). I -suppose- we can survive for now with
just reading it, but we might well run into trouble with some "special"
devices shared accross partitions or if the IBM magic bridges themselves
ever start sending MSI-X on their own (unlikely but who knows...).
Michael's code handled that by having a callback ->check() do the sanity
checking of the nvec, and then just use the nvec passed in as an
argument once it's sane.

So for that I would propose adding an arch_check_msi(pdev, type, nvec)
or something like that. Note the biggest issue at this point anyway.

 - The real big one: For MSI-X, Eric's code tries to "hide" the fact
that those are MSI-X by allocating the msi-x entry array, then iterating
through them calling arch_setup_msi_irq() for each of them.

For that to work for us, it would need to be different, possibly
pre-allocating the array, and having -one- call taking an array and a
nvec. That's one of the reasons why I liked Michael's approach as
instead of making MSI-X look like MSI, it made MSI look like MSI-X by
passing a 1 entry array in the MSI case. Both approaches can probably be
made to handle multiple MSIs if we ever want to handle them.

The same issue is present for teardown of course.

 - We need HV hooks for suspend/resume at one point. Nothing urgent
there as our HV machines don't do suspend/resume just yet :-) But if we
ever implement something like suspend-to-disk, they'll definitely need
something as we are likely to get different vectors back from the
firmware so we need to re-map them to the same linux IRQ numbers.

I need to have a second look at Eric's code after I manage to find the
right combination of kernel for his patches to apply to check if I
missed anything important.


  parent reply	other threads:[~2007-01-29  6:07 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1169714047.65693.647693675533.qpush@cradle>
2007-01-28 19:40 ` Eric W. Biederman
2007-01-28 19:42   ` [PATCH 1/6] msi: Kill msi_lookup_irq Eric W. Biederman
2007-01-28 19:44     ` [PATCH 2/6] msi: Remove msi_lock Eric W. Biederman
2007-01-28 19:45       ` [PATCH 3/6] msi: Fix msi_remove_pci_irq_vectors Eric W. Biederman
2007-01-28 19:47         ` [PATCH 4/6] msi: Remove attach_msi_entry Eric W. Biederman
2007-01-28 19:52           ` [PATCH 5/6] msi: Kill the msi_desc array Eric W. Biederman
2007-01-28 19:56             ` [PATCH 6/6] msi: Make MSI useable more architectures Eric W. Biederman
2007-01-28 22:01     ` [PATCH 1/6] msi: Kill msi_lookup_irq Paul Mackerras
2007-01-28 22:18       ` Eric W. Biederman
2007-01-28 20:23   ` [PATCH 0/6] MSI portability cleanups Benjamin Herrenschmidt
2007-01-28 20:47     ` Jeff Garzik
2007-01-28 21:20       ` Eric W. Biederman
2007-01-28 21:26         ` Ingo Molnar
2007-01-28 22:09         ` Benjamin Herrenschmidt
2007-01-28 23:26           ` Eric W. Biederman
2007-01-28 23:37             ` David Miller
2007-01-29  5:18               ` Eric W. Biederman
2007-01-29  5:25                 ` David Miller
2007-01-29  5:58                   ` Eric W. Biederman
2007-01-29  6:05                   ` Benjamin Herrenschmidt [this message]
2007-01-29  8:28                     ` Eric W. Biederman
2007-01-29  9:03                     ` Eric W. Biederman
2007-01-29 10:11                       ` Michael Ellerman
2007-01-29 20:32                         ` Benjamin Herrenschmidt
2007-01-29 23:29                         ` Paul Mackerras
2007-01-29 23:40                           ` Benjamin Herrenschmidt
2007-01-29 20:22                       ` Benjamin Herrenschmidt
2007-01-29 23:05                         ` Paul Mackerras
2007-01-30 19:32                           ` Segher Boessenkool
2007-01-29  1:33             ` Benjamin Herrenschmidt
2007-02-01  4:29           ` Greg KH
2007-01-28 23:44         ` David Miller
2007-01-28 22:11       ` Eric W. Biederman
2007-01-28 23:42       ` David Miller
2007-01-28 21:34     ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1170050737.26655.233.camel@localhost.localdomain \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 0/6] MSI portability cleanups' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).