Re: [Qemu-devel] [Qemu-ppc] [PATCH 16/19] openpic: add Shared MSI support

[Scott Wood <scottwood@freescale.com>]

On 12/11/2012 02:10:14 AM, Alexander Graf wrote:
> 
> 
> On 11.12.2012, at 01:36, Scott Wood <scottwood@freescale.com> 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 <agraf@suse.de>
> >> ---
> >> 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?

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.

-Scott