From: Eric W. Biederman <hidden> Date: 2007-01-29 09:04:50
Benjamin Herrenschmidt [off-list ref] writes:
On Sun, 2007-01-28 at 21:25 -0800, David Miller wrote:
quoted
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sun, 28 Jan 2007 22:18:59 -0700
quoted
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):
pci_generic_enable_msi
pci_generic_disable_msi
pci_generic_enable_msix
pci_generic_disable_msix
pci_generic_save_msi_state
pci_generic_restore_msi_state
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
etc...
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...);
}
etc...
(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
code.
This is the most straight forward and handles machines with really
weird msi setups, so I lean in this direction.
The question is there anything at all we can do generically?
I can't see a case where ppc_md would not wind up with the hooks
that decide if it is a hypervisor or not. Even if we came up
with a better set of functions you need to hook.
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.
Ok. I think I get the point of check. I believe I need to look at your
code a little more and see what you are doing to see if there is anything
generic worth doing, that we can always do outside of architecture code
no matter how much of the job the Hypervisor wants to do for us.
I'd hate to hit a different Hypervisor that did something close but
not quite the same and have the code fail then. So definitely
avoiding touching pci config space at all in the calls seems to make a
lot of sense. This includes avoiding pci_find_capability right?
Off the top of my head the only things we can do generically are
some data structure things and flags like dev->msi_enabled or
dev->msix_enabled.
Anyway have a nice night and more in the morning.
Eric
From: Michael Ellerman <hidden> Date: 2007-01-29 10:11:31
On Mon, 2007-01-29 at 02:03 -0700, Eric W. Biederman wrote:
Benjamin Herrenschmidt [off-list ref] writes:
quoted
On Sun, 2007-01-28 at 21:25 -0800, David Miller wrote:
quoted
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sun, 28 Jan 2007 22:18:59 -0700
quoted
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):
pci_generic_enable_msi
pci_generic_disable_msi
pci_generic_enable_msix
pci_generic_disable_msix
pci_generic_save_msi_state
pci_generic_restore_msi_state
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
etc...
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...);
}
etc...
(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
code.
This is the most straight forward and handles machines with really
weird msi setups, so I lean in this direction.
The question is there anything at all we can do generically?
I can't see a case where ppc_md would not wind up with the hooks
that decide if it is a hypervisor or not. Even if we came up
with a better set of functions you need to hook.
quoted
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.
Ok. I think I get the point of check. I believe I need to look at your
code a little more and see what you are doing to see if there is anything
generic worth doing, that we can always do outside of architecture code
no matter how much of the job the Hypervisor wants to do for us.
I'd hate to hit a different Hypervisor that did something close but
not quite the same and have the code fail then. So definitely
avoiding touching pci config space at all in the calls seems to make a
lot of sense. This includes avoiding pci_find_capability right?
You can read config space, but it's not clear to me if the HV is allowed
to filter it and hide things. It's also possible that the device
supports MSI, but for some reason the HV doesn't allow it on that device
etc. so you really have to ask the HV if it's enabled. So pci_find_cap()
shouldn't crash or anything, but it may lie to you.
Off the top of my head the only things we can do generically are
some data structure things and flags like dev->msi_enabled or
dev->msix_enabled.
It would be good to have a common data structure if possible. My
thinking was that most of the information is per pci_dev, so that's
where I put it. I realise the Intel code stores some info that's
per-irq, but most of it is per-device. I hadn't got anywhere near coding
it, but my vague idea was to add a arch_data (or whatever) pointer to my
msi_info struct, which would allow backends to stash stuff.
I think the pci_intx() calls can be in the core.
Munging dev->irq could be in the core, assuming it's left in some known
location by the code. On the other hand we might want to decide it's a
bad idea altogether.
One thing I did like about my code, is that pci_enable_msi() and
pci_enable_msix() are just small wrappers around generic_enable_msi() -
which does all the work, and is the same regardless of whether it's an
MSI or MSI-X. Although that's facilitated by the type arg which you
don't like.
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-29 20:27:29
This is the most straight forward and handles machines with really
weird msi setups, so I lean in this direction.
The question is there anything at all we can do generically?
I can't see a case where ppc_md would not wind up with the hooks
that decide if it is a hypervisor or not. Even if we came up
with a better set of functions you need to hook.
Sure, but with Michael's approach, the only hook was get_msi_ops(pdev)
Anyway, there isn't -that- much that can be done generically in the HV
case. Mostly some argument sanity checking, the logic for saving &
restoring pdev->irq for MSIs, that sort of thing.
Ok. I think I get the point of check. I believe I need to look at your
code a little more and see what you are doing to see if there is anything
generic worth doing, that we can always do outside of architecture code
no matter how much of the job the Hypervisor wants to do for us.
I understand.
I'd hate to hit a different Hypervisor that did something close but
not quite the same and have the code fail then. So definitely
avoiding touching pci config space at all in the calls seems to make a
lot of sense. This includes avoiding pci_find_capability right?
Quite possibly yes. I'm pretty sure it will work on IBM HV but we aren't
really supposed to use it...
Off the top of my head the only things we can do generically are
some data structure things and flags like dev->msi_enabled or
dev->msix_enabled.
That and the saving & restoring of pdev->irq. That is not very much.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-29 20:33:26
You can read config space, but it's not clear to me if the HV is allowed
to filter it and hide things.
I've seen it do it for example with EADS bridges. I haven't seen doing
it with devices (other than hiding entire functions) but I wouldn't
exclude it...
It's also possible that the device
supports MSI, but for some reason the HV doesn't allow it on that device
etc. so you really have to ask the HV if it's enabled. So pci_find_cap()
shouldn't crash or anything, but it may lie to you.
Yup.
One thing I did like about my code, is that pci_enable_msi() and
pci_enable_msix() are just small wrappers around generic_enable_msi() -
which does all the work, and is the same regardless of whether it's an
MSI or MSI-X. Although that's facilitated by the type arg which you
don't like.
Part of the reason is you make MSI look like MSI-X (a vector of 1 entry)
while Eric does the opposite.
Ben.
From: Paul Mackerras <hidden> Date: 2007-01-29 23:05:55
Benjamin Herrenschmidt writes:
quoted
I'd hate to hit a different Hypervisor that did something close but
not quite the same and have the code fail then. So definitely
avoiding touching pci config space at all in the calls seems to make a
lot of sense. This includes avoiding pci_find_capability right?
Quite possibly yes. I'm pretty sure it will work on IBM HV but we aren't
really supposed to use it...
Actually, I don't know of any reason why we can't use
pci_find_capability. We are supposed to avoid trying to touch config
space of devices (in fact, functions) that aren't assigned to our
partition, but we're not talking about that here.
I just got an answer from the hypervisor architects. It turns out
that the hardware _does_ prevent the device from sending MSI messages
to another partition. The OS _can_ write whatever it likes to the MSI
address and data registers. It can potentially lose interrupts (or, I
expect, get the device isolated by EEH) but it can't disrupt another
partition.
I think the reason why the hypervisor call writes the values straight
into the MSI/MSI-X registers in the device is (a) that's convenient
for AIX, since it saves it from immediately having to do more calls
into the hypervisor to write those values to the device, and (b) there
are some ABI complications in returning a lot of values, so the device
registers provide a convenient place to return those values.
So it would be possible, although gross, to do the hypervisor call,
read the values from config space and return them to the generic code,
then let the generic code write them to config space for us. :P
The remaining point of difference then seems to be that for MSI-X, we
really want to know up-front how many interrupts the device driver is
asking for, rather than having a series of alloc requests dribble in
one at a time.
Regards,
Paul.
From: Paul Mackerras <hidden> Date: 2007-01-29 23:29:20
Michael Ellerman writes:
You can read config space, but it's not clear to me if the HV is allowed
to filter it and hide things. It's also possible that the device
It appears that the HV does not prevent us from reading or writing any
config space registers for functions that are assigned to us.
supports MSI, but for some reason the HV doesn't allow it on that device
etc. so you really have to ask the HV if it's enabled. So pci_find_cap()
shouldn't crash or anything, but it may lie to you.
It's possible that the device can do MSI(X), but that using MSI(X)
requires other platform resources (e.g. interrupt source numbers) and
there are none free. I believe the platform guarantees a minimum
number of MSI(X) interrupts per function, but a pci_enable_msix() call
may not be able to give the driver as many MSI-X interrupts as it is
requesting even if the function can handle that many.
Paul.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-29 23:41:20
It's possible that the device can do MSI(X), but that using MSI(X)
requires other platform resources (e.g. interrupt source numbers) and
there are none free. I believe the platform guarantees a minimum
number of MSI(X) interrupts per function, but a pci_enable_msix() call
may not be able to give the driver as many MSI-X interrupts as it is
requesting even if the function can handle that many.
However, the ibm,req#msi(-x) properties contain the number as requested
by the device, and thus I expect them to be identical to the config
space value. So if you are confident enough that our HV won't play any
tricks there in the future, reading the config space is as good as
hooking that check() callback, though it might not be vs. some other HV
for some other platform that might be more strict.
We cannot know in advance how much max the HV will give us without
actually trying ibm,change-msi and see the result code for it
unfortunately.
Ben.
I just got an answer from the hypervisor architects. It turns out
that the hardware _does_ prevent the device from sending MSI messages
to another partition. The OS _can_ write whatever it likes to the MSI
address and data registers. It can potentially lose interrupts (or, I
expect, get the device isolated by EEH) but it can't disrupt another
partition.
The OS however has to write the values the HV wants to
the device, or things won't work -- so the HV can just
as well do it itself. Also, pulling all the work into
the HV makes for a cleaner, more generic design (who
knows what hardware will show up within the next few
years, the HV interface had better be prepared).
Segher