[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH 16/19] openpic: add Shared MSI suppor
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH 16/19] openpic: add Shared MSI support |
Date: |
Wed, 12 Dec 2012 12:12:10 +0100 |
On 12.12.2012, at 02:42, Scott Wood wrote:
> On 12/11/2012 06:53:44 PM, Alexander Graf wrote:
>> On 11.12.2012, at 18:35, Scott Wood wrote:
>> > On 12/11/2012 02:10:14 AM, Alexander Graf wrote:
>> >> On 11.12.2012, at 01:36, Scott Wood <address@hidden> wrote:
>> >> > On 12/08/2012 07:44:39 AM, Alexander Graf wrote:
>> >> >> The OpenPIC allows MSI access through shared MSI registers. Implement
>> >> >> them for the MPC8544 MPIC, so we can support MSIs.
>> >> >> Signed-off-by: Alexander Graf <address@hidden>
>> >> >> ---
>> >> >> hw/openpic.c | 150
>> >> >> ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>> >> >> 1 files changed, 130 insertions(+), 20 deletions(-)
>> >> >> diff --git a/hw/openpic.c b/hw/openpic.c
>> >> >> index f2f152f..f71d668 100644
>> >> >> --- a/hw/openpic.c
>> >> >> +++ b/hw/openpic.c
>> >> >> @@ -38,6 +38,7 @@
>> >> >> #include "pci.h"
>> >> >> #include "openpic.h"
>> >> >> #include "sysbus.h"
>> >> >> +#include "msi.h"
>> >> >> //#define DEBUG_OPENPIC
>> >> >> @@ -52,6 +53,7 @@
>> >> >> #define MAX_TMR 4
>> >> >> #define VECTOR_BITS 8
>> >> >> #define MAX_IPI 4
>> >> >> +#define MAX_MSI 8
>> >> >> #define VID 0x03 /* MPIC version ID */
>> >> >> /* OpenPIC capability flags */
>> >> >> @@ -62,6 +64,8 @@
>> >> >> #define OPENPIC_GLB_REG_SIZE 0x10F0
>> >> >> #define OPENPIC_TMR_REG_START 0x10F0
>> >> >> #define OPENPIC_TMR_REG_SIZE 0x220
>> >> >> +#define OPENPIC_MSI_REG_START 0x1600
>> >> >> +#define OPENPIC_MSI_REG_SIZE 0x200
>> >> >> #define OPENPIC_SRC_REG_START 0x10000
>> >> >> #define OPENPIC_SRC_REG_SIZE (MAX_IRQ * 0x20)
>> >> >> #define OPENPIC_CPU_REG_START 0x20000
>> >> >> @@ -126,6 +130,12 @@
>> >> >> #define IDR_P1_SHIFT 1
>> >> >> #define IDR_P0_SHIFT 0
>> >> >> +#define MSIIR_OFFSET 0x140
>> >> >> +#define MSIIR_SRS_SHIFT 29
>> >> >> +#define MSIIR_SRS_MASK (0x7 << MSIIR_SRS_SHIFT)
>> >> >> +#define MSIIR_IBS_SHIFT 24
>> >> >> +#define MSIIR_IBS_MASK (0x1f << MSIIR_IBS_SHIFT)
>> >> >
>> >> > FWIW, if you want to model newer MPICs such as on p4080, they have
>> >> > multiple banks of MSIs, so you may not want to hardcode one bank.
>> >> The OpenPIC code was suffering a lot from attempts to generalize
>> >> different implementations without implementing them.
>> >> If we want to add support for p4080 MPICs later, we add a new model to
>> >> the emulation and make the nr of msi banks a parameter, like the patch
>> >> set does for all the other raven/mpc8544 differences. That way we don't
>> >> get into the current mess of a halfway accurate emulation unless we
>> >> really want it.
>> >
>> > So because the old code made a mess of it, we're saying "abstraction is
>> > bad" in general?
>> No, because the old code messed up abstraction, I would like to move to a
>> non-abstract level and then start abstracting at the correct layer. What
>> that correct layer is is still up for discussion :).
>> > All I'm saying is this should be done with a runtime data structure (of
>> > which there could be more than one) rather than #defines.
>> Yeah, and my argument was that this should be introduced as part of a new
>> model. But if it makes you happy I can of course also make it generic right
>> now, without a user, which means we can't even test whether the
>> generalization works, which means we might screw it up again ;).
>
> It would have a user. It just wouldn't have multiple users, or a user that
> instantiates multiple banks. This feels like writing a kernel driver that
> can only handle one instance of a device, just because that's all your SoC
> happens to have at the moment. Nobody objects to a driver having its state
> in a struct, or the driver maintaining a list of said structs, just because
> you don't have hardware to test the case where there are multiple instances.
Looking at this a bit more, I am getting more and more reluctant to make this
dynamic with the current openpic models we have.
To get multiple MSI banks cleanly working, we'd need to make a number of fields
in the state struct dynamically allocated (msir backing, sub_io_mem). We'd also
need to find a way to identify which bank a callback happens at, which the
memory api hides from us today. So we again would probably need to allocate
some container struct that we can pass as opaque which tells us the region id
we're in.
And all of the pitfalls above would be something you could only spot using code
review, not by actual execution tests, because we would have to hardcode
msi_banks to 1 for fsl_mpic_20. So every time we assume cur_msi_bank == 0 the
code would "just work".
I would really much rather like to postpone that generalization on to the point
in time we start implementing more than 1 msi bank. Then we can be sure that
the code we're doing generically actually works.
Alex
- [Qemu-devel] [PATCH 01/19] openpic: Remove unused code, (continued)
[Qemu-devel] [PATCH 12/19] openpic: rename openpic_t to OpenPICState, Alexander Graf, 2012/12/08
[Qemu-devel] [PATCH 08/19] openpic: combine openpic and mpic reset functions, Alexander Graf, 2012/12/08
[Qemu-devel] [PATCH 09/19] openpic: unify memory api subregions, Alexander Graf, 2012/12/08
[Qemu-devel] [PATCH 06/19] openpic: combine mpic and openpic irq raise functions, Alexander Graf, 2012/12/08
[Qemu-devel] [PATCH 02/19] mpic: Unify numbering scheme, Alexander Graf, 2012/12/08
[Qemu-devel] [PATCH 14/19] openpic: convert to qdev, Alexander Graf, 2012/12/08