From: Eric W. Biederman <hidden> Date: 2007-01-28 21:21:53
Jeff Garzik [off-list ref] writes:
Benjamin Herrenschmidt wrote:
quoted
quoted
The only architecture problem that isn't solvable in this context is
the problem of supporting the crazy hypervisor on the ppc RTAS, which
asks us to drive the hardware but does not give us access to the
hardware registers.
So you are saying that we should use your model while admitting that it
can't solve our problems...
I really don't understand why you seem so totally opposed to Michael's
approach which definitely looks to me like the sane thing to do. Note
that in the end, Michael's approach isn't -that- different from yours,
just a bit more abstracted.
I think the high-level ops approach makes more sense. It's more future proof,
in addition to covering all existing implementations.
I'm not arguing against an operations based approach. I'm arguing for simple
obviously correct steps, and not throwing the baby out with the bath
water.
My patches should be a precursor to an operations based approach
because they are simple step from where we are now.
Every keeps telling me the operations approach is the right thing to
do and I see code that doesn't work, and can't work without extreme
difficulty on the architectures currently supported. That makes me
irritated, and unfortunately much less accepting.
I see people pushing ridiculous interfaces like the RTAS hypervisor
interface at me, and saying we must support running firmware drivers
in the msi code.
I just ask for simple evolutionary change as I presented, so we don't
break things or loose requirements along the way.
Please argue with me on the details of what the ops based approach does
better, which specific problems does it solve.
The proposed ops base approach mixes different kinds of operations
in the same structure:
We have the hardware operations:
+ /* enable - Enable the MSIs on the given device.
+ *
+ * @pdev: PCI device structure.
+ * @num: The number of MSIs being requested.
+ * @entries: An array of @num msix_entry structures.
+ * @type: The type, MSI or MSI-X.
+ *
+ * This routine enables the MSIs on the given PCI device.
+ *
+ * If the enable completes succesfully this routine must return 0.
+ *
+ * This callback is optional.
+ */
+ int (*enable) (struct pci_dev *pdev, int num,
+ struct msix_entry *entries, int type);
+
+ /* disable - disable the MSI for the given device.
+ *
+ * @pdev: PCI device structure.
+ * @num: The number of MSIs to disable.
+ * @entries: An array of @num msix_entry structures.
+ * @type: The type, MSI or MSI-X.
+ *
+ * This routine should perform the inverse of enable.
+ */
+ void (*disable) (struct pci_dev *pdev, int num,
+ struct msix_entry *entries, int type);
+
Which are either talking directly to the hardware, or are talking
to the hypervisor, which is using hardware isolation so it is safe to
talk directly to the hardware but isn't leting us? If we could use
things to work around errata in card implementation details it would
make some sense to me (although we don't seem to have any cards with
that got the MSI registers wrong at this point). Regardless these
operations clearly have a different granularity than the other
operations, and should have a different lookup method.
We have the irq operations.
+ /* check - Check that the requested MSI allocation is OK.
+ *
+ * @pdev: PCI device structure.
+ * @num: The number of MSIs being requested.
+ * @entries: An array of @num msix_entry structures.
+ * @type: The type, MSI or MSI-X.
+ *
+ * This routine is responsible for checking that the given PCI device
+ * can be allocated the requested type and number of MSIs.
+ *
+ * It is up to this routine to determine if the requested number of
+ * MSIs is valid for the device in question. If the number of MSIs,
+ * or the particular MSI entries, can not be supported for any
+ * reason this routine must return non-zero.
+ *
+ * If the check is succesful this routine must return 0.
+ */
+ int (*check) (struct pci_dev *pdev, int num,
+ struct msix_entry *entries, int type);
+
+ /* alloc - Allocate MSIs for the given device.
+ *
+ * @pdev: PCI device structure.
+ * @num: The number of MSIs being requested.
+ * @entries: An array of @num msix_entry structures.
+ * @type: The type, MSI or MSI-X.
+ *
+ * This routine is responsible for allocating the number of
+ * MSIs to the given PCI device.
+ *
+ * Upon completion there must be @num MSIs assigned to this device,
+ * the "vector" member of each struct msix_entry must be filled in
+ * with the Linux irq number allocated to it. The corresponding
+ * irq_descs must also be setup with an appropriate handler if
+ * required.
+ *
+ * If the allocation completes succesfully this routine must return 0.
+ */
+ int (*alloc) (struct pci_dev *pdev, int num,
+ struct msix_entry *entries, int type);
+
+ /* free - free the MSIs assigned to the device.
+ *
+ * @pdev: PCI device structure.
+ * @num: The number of MSIs.
+ * @entries: An array of @num msix_entry structures.
+ * @type: The type, MSI or MSI-X.
+ *
+ * Free all MSIs and associated resources for the device. If any
+ * MSIs have been enabled they will have been disabled already by
+ * the generic code.
+ */
+ void (*free) (struct pci_dev *pdev, int num,
+ struct msix_entry *entries, int type);
These because they are per irq make sense as per bus operations unless
you have a good architecture definition like x86 has. Roughly those
operations are what we currently have except the current operations
are a little simpler and easier to deal with for the architecture
code.
And then there are the operations that are going in the wrong
direction.
+ /* setup_msi_msg - Setup an MSI message for the given device.
+ *
+ * @pdev: PCI device structure.
+ * @entry: The MSI entry to create a msi_msg for.
+ * @msg: Written with the magic address and data.
+ * @type: The type, MSI or MSI-X.
+ *
+ * Returns the "magic address and data" used to trigger the msi.
+ * If the setup is succesful this routine must return 0.
+ *
+ * This callback is optional.
+ */
+ int (*setup_msi_msg) (struct pci_dev *pdev, struct msix_entry *entry,
+ struct msi_msg *msg, int type);
Much to much of the operations base approach as proposed looks like
when you have a hammer every problem looks like a nail, given how much
confusion about what was put into the operations structure.
I don't mind taking a small step and making the alloc/free primitives
per bus in a generic fashion.
I don't mind supporting poorly designed hypervisor interfaces, if it
is easy.
I do strongly mind code that doesn't work, or we can't git-bisect
through to find where bugs were introduced.
Eric
I'm not arguing against an operations based approach. I'm arguing for
simple obviously correct steps, and not throwing the baby out with the
bath water.
My patches should be a precursor to an operations based approach
because they are simple step from where we are now.
yeah. I'd say your approach is to go from A to B:
[A] -----------------------------------------------------> [B]
|
[C]
while there might be some other arguments that "no, lets go to C
instead", i say lets not throw away the already implemented and already
working and nicely layered [A]->[B] transition, just because there's an
argument whether the end result should be 'B' or 'C'. Unless someone who
wants to see 'C' produces a patchset that walks the whole way i dont see
any reason to not go with your patchset. It clearly removes alot of
cruft.
Acked-by: Ingo Molnar <redacted>
Ingo
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-28 22:10:19
.../... (enable/disable bits)
Which are either talking directly to the hardware, or are talking
to the hypervisor, which is using hardware isolation so it is safe to
talk directly to the hardware but isn't leting us? If we could use
things to work around errata in card implementation details it would
make some sense to me (although we don't seem to have any cards with
that got the MSI registers wrong at this point). Regardless these
operations clearly have a different granularity than the other
operations, and should have a different lookup method.
I'm not sure I undersdand the point of your rant here. The hypervisor
case hooks at alloc/free and does everything from there. It doens't use
an enable or a diable hook.
The enable/disable ops are optional for that reason. When not present,
it's assumed that alloc/free do it all.
When using a "direct" approach (what we call "raw"), we expect backends
to just plug the provided helper functions in enable/disable. It's still
a hook so that one can do additional platform specific bits if
necessary, but in that specific case, I do agree we could just remove it
and move the "raw" code back into the toplevel functions, with a way
(via a special return code from alloc maybe ?) for the HV case to tell
us not to go through there. That was one of our initial approaches when
working with Michael on the design.
However, that sort of hurts my sense of aestetics :-) I quite like the
toplevel to be just a toplevel, and clearly separate the raw "helpers"
and the backend. Provides more flexibility to handle all possible crazy
cases in the future.
You seem to absolutely want to get the HV case to go throuh the same
code path as the "raw" case, and that will not happen.
.../... (irq operations)
These because they are per irq make sense as per bus operations unless
you have a good architecture definition like x86 has. Roughly those
operations are what we currently have except the current operations
are a little simpler and easier to deal with for the architecture
code.
Oh ? How so ? (easier/simpler ?)
And then there are the operations that are going in the wrong
direction.
+ /* setup_msi_msg - Setup an MSI message for the given device.
+ *
+ * @pdev: PCI device structure.
+ * @entry: The MSI entry to create a msi_msg for.
+ * @msg: Written with the magic address and data.
+ * @type: The type, MSI or MSI-X.
+ *
+ * Returns the "magic address and data" used to trigger the msi.
+ * If the setup is succesful this routine must return 0.
+ *
+ * This callback is optional.
+ */
+ int (*setup_msi_msg) (struct pci_dev *pdev, struct msix_entry *entry,
+ struct msi_msg *msg, int type);
Much to much of the operations base approach as proposed looks like
when you have a hammer every problem looks like a nail, given how much
confusion about what was put into the operations structure.
This is indeed a lower level hook to be used by "raw" enable/disable. An
other approach would be to remove it, have each backend have it's own
enable/disable that obtains the address/data and calls into a helper to
program them. This would indeed be a little bit nicer in a layering
perspective. But it adds a bit more code to each backend, so we kept
things closer to the way they used to be. I don't have a firm reason not
to change it however, I need talk to Michael in case he has more good
reasons to keep it that way around.
I don't mind taking a small step and making the alloc/free primitives
per bus in a generic fashion.
I don't mind supporting poorly designed hypervisor interfaces, if it
is easy.
And it it's not, we don't support them ? Ugh ? Well, it happens to be
fairly easy but still, I don't understand your approach there.
I do strongly mind code that doesn't work, or we can't git-bisect
through to find where bugs were introduced.
It doesn't work yet for you which is why it's not -replacing- your
current code. Again, this was intended as arch code in the first place,
until other archs and maintainers voiced their opinion that we should
move that to generic code. It may not be perfect, we may still want to
change things, maybe make some things closer to the direction you are
taking for the x86 code, but I don't understand the root of such a
strong opposition except mayeb that you've spent time trying to fix the
x86 junk and now are annoyed to see some of that work possibly
replaced ?
I agree with the problem if small changes & bisecting in the general
case. In fact, it would be nice if we could use your fixed code with
little change to "plug" it in as the x86 backend in many ways. Michael's
work isn't a re-implementation of everything, it's a re-structuring,
lots of bits of code that are missing can possibly be lifted from the
existing working implementation.
If we followed that "only do incrementental changes" rule all the time,
imagine in what state would be our USB stack today since we couldn't
have dropped in Linus replacement one ...
Ben.
On Mon, Jan 29, 2007 at 09:09:14AM +1100, Benjamin Herrenschmidt wrote:
If we followed that "only do incrementental changes" rule all the time,
imagine in what state would be our USB stack today since we couldn't
have dropped in Linus replacement one ...
Bad example, that is not what happened at all. There was not an
in-kernel USB stack when Linus wrote his. Inaky had his
all-singing-all-dancing stack outside of the tree, and no one was really
helping out with it.
Only when Linus added his code to mainline did we all jump on it and
_incrementally_ improve it to what we have today.
So, in a way, you just proved that we need to do this in an incremental
fashion, which is what I was also saying all along :)
thanks,
greg k-h
From: David Miller <davem@davemloft.net> Date: 2007-01-28 23:44:55
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sun, 28 Jan 2007 14:20:12 -0700
I see people pushing ridiculous interfaces like the RTAS hypervisor
interface at me, and saying we must support running firmware drivers
in the msi code.
This is not what's going on.
The hypervisor does the PCI config space programming on the
device to setup the MSI so that it can be done in a controlled
manner and such that the device cannot ever be configured by
one domain to shoot MSI packets over at devices which belong
to another domain.
It's that simple.
That's absolutely reasonable, and is I believe what you'll see the
sparc64 hypervisor(s) all needing as well.