Re: [Qemu-devel] [PATCH] PPC: Bring EPR support closer to reality

[Scott Wood <scottwood@freescale.com>]

On 01/04/2013 04:24:59 AM, Alexander Graf wrote:
> We already used to support the external proxy facility of FSL MPICs,
> but only implemented it halfway correctly.
> 
> This patch adds support for
> 
>   * dynamic enablement of the EPR facility
>   * interrupt acknowledgement only when the interrupt is delivered
> 
> This way the implementation now is closer to real hardware.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/openpic.c                |   20 ++++++++++++++++++++
>  target-ppc/Makefile.objs    |    1 -
>  target-ppc/cpu.h            |    2 ++
>  target-ppc/excp_helper.c    |    4 ++++
>  target-ppc/helper.h         |    1 -
>  target-ppc/mpic_helper.c    |   35  
> -----------------------------------
>  target-ppc/translate_init.c |    7 +------
>  7 files changed, 27 insertions(+), 43 deletions(-)
>  delete mode 100644 target-ppc/mpic_helper.c
> 
> diff --git a/hw/openpic.c b/hw/openpic.c
> index e773d68..6447a47 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -131,6 +131,9 @@ static const int debug_openpic = 0;
>  #define VIR_GENERIC      0x00000000 /* Generic Vendor ID */
> 
>  #define GCR_RESET        0x80000000
> +#define GCR_MODE_PASS    0x00000000
> +#define GCR_MODE_MIXED   0x20000000
> +#define GCR_MODE_PROXY   0x60000000
> 
>  #define TBCR_CI           0x80000000 /* count inhibit */
>  #define TCCR_TOG          0x80000000 /* toggles when decrement to  
> zero */
> @@ -233,6 +236,7 @@ typedef struct OpenPICState {
>      uint32_t ivpr_reset;
>      uint32_t idr_reset;
>      uint32_t brr1;
> +    uint32_t mpic_mode_mask;
> 
>      /* Sub-regions */
>      MemoryRegion sub_io_mem[5];
> @@ -667,6 +671,20 @@ static void openpic_gbl_write(void *opaque,  
> hwaddr addr, uint64_t val,
>      case 0x1020: /* GCR */
>          if (val & GCR_RESET) {
>              openpic_reset(&opp->busdev.qdev);
> +        } else if (opp->mpic_mode_mask) {
> +            CPUArchState *env;
> +            int mpic_proxy = 0;
> +
> +            opp->gcr &= ~opp->mpic_mode_mask;
> +            opp->gcr |= val & opp->mpic_mode_mask;
> +
> +            /* Set external proxy mode */
> +            if ((val & opp->mpic_mode_mask) == GCR_MODE_PROXY) {
> +                mpic_proxy = 1;
> +            }
> +            for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +                env->mpic_proxy = mpic_proxy;
> +            }
>          }
>          break;
>      case 0x1080: /* VIR */
> @@ -1407,6 +1425,8 @@ static int openpic_init(SysBusDevice *dev)
>          opp->irq_tim0 = FSL_MPIC_20_TMR_IRQ;
>          opp->irq_msi = FSL_MPIC_20_MSI_IRQ;
>          opp->brr1 = FSL_BRR1_IPID | FSL_BRR1_IPMJ | FSL_BRR1_IPMN;
> +        opp->mpic_mode_mask = GCR_MODE_PROXY;
> +

Technically this should only be available starting with v4.0, but I  
guess that can wait until we have more general support for a newer MPIC  
model.  Should at least have a comment, though.

>          msi_supported = true;
>          list = list_be;
> 
> diff --git a/target-ppc/Makefile.objs b/target-ppc/Makefile.objs
> index 237a0ed..6c11ef8 100644
> --- a/target-ppc/Makefile.objs
> +++ b/target-ppc/Makefile.objs
> @@ -9,4 +9,3 @@ obj-y += mmu_helper.o
>  obj-y += timebase_helper.o
>  obj-y += misc_helper.o
>  obj-y += mem_helper.o
> -obj-y += mpic_helper.o
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index e88ebe0..0db06d6 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1068,6 +1068,8 @@ struct CPUPPCState {
>      target_ulong ivpr_mask;
>      target_ulong hreset_vector;
>      hwaddr mpic_cpu_base;
> +    /* true when the external proxy facility mode is enabled */
> +    int mpic_proxy;
>  #endif

bool?

> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 41037a7..2b80164 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -178,6 +178,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu,  
> int excp_model, int excp)
>          if (lpes0 == 1) {
>              new_msr |= (target_ulong)MSR_HVB;
>          }
> +        if (env->mpic_proxy) {
> +            /* IACK the IRQ on delivery */
> +            env->spr[SPR_BOOKE_EPR] = ldl_phys(env->mpic_cpu_base +  
> 0xA0);
> +        }

Can we avoid the opencoded 0xA0?  QEMU has too many open-coded magic  
numbers as is.

I don't see where mpic_cpu_base is used other than for EPR; maybe just  
change it to mpic_iack?  Or are there plans to do other things with it?

-Scott