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

[Alexander Graf <agraf@suse.de>]

On 04.01.2013, at 19:50, Scott Wood wrote:

> 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.

Phew. That one's tricky. I'll just add a comment for now, yeah.

> 
>>         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?

IIRC we can't save/restore bools easily using savevm, and this one should be saved/restored.

> 
>> 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?

Not really. I'll change it accordingly.


Alex