qemu-devel
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]