Re: [Qemu-devel] [PATCHv3 02/14] unicore32-softmmu: Add coprocessor 0(sysctrl) and 1(ocd) instruction support

[Blue Swirl]

On Wed, Jun 20, 2012 at 1:40 AM, Guan Xuetao <address@hidden> wrote:
> On Mon, 2012-06-18 at 19:51 +0000, Blue Swirl wrote:
>> On Mon, Jun 18, 2012 at 9:24 AM, Guan Xuetao <address@hidden> wrote:
>> > Coprocessor 0 is system control coprocessor, and we need get/set its 
>> > contents.
>> > Also, all cache/tlb ops shoule be implemented here, but just ignored with 
>> > no harm.
>> >
>> > Coprocessor 1 is OCD (on-chip-debugger), which is used for faked console,
>> > so we could output chars to this console without graphic card.
>> >
>> > Signed-off-by: Guan Xuetao <address@hidden>
>> > ---
>> >  target-unicore32/helper.c    |  177 
>> > +++++++++++++++++++++++++++++++++---------
>> >  target-unicore32/helper.h    |   17 ++---
>> >  target-unicore32/translate.c |   75 ++++++++++++++++++-
>> >  3 files changed, 221 insertions(+), 48 deletions(-)
>> >
>> > diff --git a/target-unicore32/helper.c b/target-unicore32/helper.c
>> > index 9b8ff06..42a39e5 100644
>> > --- a/target-unicore32/helper.c
>> > +++ b/target-unicore32/helper.c
>> > @@ -14,6 +14,14 @@
>> >  #include "helper.h"
>> >  #include "host-utils.h"
>> >
>> > +#undef DEBUG_UC32
>> > +
>> > +#ifdef DEBUG_UC32
>> > +#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
>> > +#else
>> > +#define DPRINTF(fmt, ...) do {} while (0)
>> > +#endif
>> > +
>> >  CPUUniCore32State *uc32_cpu_init(const char *cpu_model)
>> >  {
>> >     UniCore32CPU *cpu;
>> > @@ -45,6 +53,138 @@ uint32_t HELPER(clz)(uint32_t x)
>> >     return clz32(x);
>> >  }
>> >
>> > +#ifndef CONFIG_USER_ONLY
>> > +void helper_cp0_set(CPUUniCore32State *env, uint32_t val, uint32_t creg,
>> > +        uint32_t cop)
>> > +{
>> > +    /*
>> > +     * movc pp.nn, rn, #imm9
>> > +     *      rn: UCOP_REG_D
>> > +     *      nn: UCOP_REG_N
>> > +     *          1: sys control reg.
>> > +     *          2: page table base reg.
>> > +     *          3: data fault status reg.
>> > +     *          4: insn fault status reg.
>> > +     *          5: cache op. reg.
>> > +     *          6: tlb op. reg.
>> > +     *      imm9: split UCOP_IMM10 with bit5 is 0
>> > +     */
>> > +    switch (creg) {
>> > +    case 1:
>> > +        if (cop != 0) goto unrecognized;
>>
>> Does this pass scripts/checkpatch.pl? These should become
>> if (cop != 0) {
>>     goto unrecognized;
>> }
> Thanks for pointing it out, and sorry for that.
> I will correct it in next version.
>
>>
>> > +        env->cp0.c1_sys = val;
>> > +        break;
>> > +    case 2:
>> > +        if (cop != 0) goto unrecognized;
>> > +        env->cp0.c2_base = val;
>> > +        break;
>> > +    case 3:
>> > +        if (cop != 0) goto unrecognized;
>> > +        env->cp0.c3_faultstatus = val;
>> > +        break;
>> > +    case 4:
>> > +        if (cop != 0) goto unrecognized;
>> > +        env->cp0.c4_faultaddr = val;
>> > +        break;
>> > +    case 5:
>> > +        switch(cop) {
>> > +        case 28:
>> > +            DPRINTF("Invalidate Entire I&D cache\n");
>> > +            return;
>> > +        case 20:
>> > +            DPRINTF("Invalidate Entire Icache\n");
>> > +            return;
>> > +        case 12:
>> > +            DPRINTF("Invalidate Entire Dcache\n");
>> > +            return;
>> > +        case 10:
>> > +            DPRINTF("Clean Entire Dcache\n");
>> > +            return;
>> > +        case 14:
>> > +            DPRINTF("Flush Entire Dcache\n");
>> > +            return;
>> > +        case 13:
>> > +            DPRINTF("Invalidate Dcache line\n");
>> > +            return;
>> > +        case 11:
>> > +            DPRINTF("Clean Dcache line\n");
>> > +            return;
>> > +        case 15:
>> > +            DPRINTF("Flush Dcache line\n");
>> > +            return;
>> > +        }
>> > +        break;
>> > +    case 6:
>> > +        if ((cop <= 6) && (cop >=2)) {
>> > +            /* invalid all tlb */
>> > +            tlb_flush(env, 1);
>> > +            return;
>> > +        }
>> > +        break;
>> > +    default:
>> > +        goto unrecognized;
>> > +    }
>> > +    return;
>> > +unrecognized:
>> > +    cpu_abort(env, "Wrong register (%d) or wrong operation (%d) in 
>> > cp0_set!\n",
>> > +            creg, cop);
>>
>> The call to cpu_abort() would mean that the guest is able to terminate
>> QEMU at will, which is not OK. What does real HW do?
> In my opinion, I just want to terminate qemu when any unhandled or
> unknown operations happen.

This can make the emulator vulnerable in the security sense. Probably
Unicore CPUs are not used now in an environment where the guest can
not be trusted (like cloud computing), but who knows the future?

>
>>
>> > +}
>> > +
>> > +uint32_t helper_cp0_get(CPUUniCore32State *env, uint32_t creg, uint32_t 
>> > cop)
>> > +{
>> > +    /*
>> > +     * movc rd, pp.nn, #imm9
>> > +     *      rd: UCOP_REG_D
>> > +     *      nn: UCOP_REG_N
>> > +     *          0: cpuid and cachetype
>> > +     *          1: sys control reg.
>> > +     *          2: page table base reg.
>> > +     *          3: data fault status reg.
>> > +     *          4: insn fault status reg.
>> > +     *      imm9: split UCOP_IMM10 with bit5 is 0
>> > +     */
>> > +    switch (creg) {
>> > +    case 0:
>> > +        switch (cop) {
>> > +        case 0:
>> > +            return env->cp0.c0_cpuid;
>> > +        case 1:
>> > +            return env->cp0.c0_cachetype;
>> > +        }
>> > +        break;
>> > +    case 1:
>> > +        if (cop == 0) {
>> > +            return env->cp0.c1_sys;
>> > +        }
>> > +        break;
>> > +    case 2:
>> > +        if (cop == 0) {
>> > +            return env->cp0.c2_base;
>> > +        }
>> > +        break;
>> > +    case 3:
>> > +        if (cop == 0) {
>> > +            return env->cp0.c3_faultstatus;
>> > +        }
>> > +        break;
>> > +    case 4:
>> > +        if (cop == 0) {
>> > +            return env->cp0.c4_faultaddr;
>> > +        }
>> > +        break;
>> > +    }
>> > +    cpu_abort(env, "Wrong register (%d) or wrong operation (%d) in 
>> > cp0_set!\n",
>> > +            creg, cop);
>> > +}
>> > +
>> > +void helper_cp1_putc(target_ulong x)
>> > +{
>> > +    printf("%c", x);
>> > +    fflush(NULL);
>> > +    return;
>>
>> Useless return.
> Yes, thanks.
>
>>
>> The printout should be enabled only for DEBUG_UC32.
> Here, I just want to print a char in the text console.
> I tried printw and addch under curses environment, but their color
> schemes had some problems in my server, and I must call scrollok() at
> every new-line. (scrl() didn't work) So, I left printf here to output a
> character from ocd_console in kernel, and it works.

It breaks the abstraction layer. CPUs very rarely have any direct
instructions for high level I/O (like console output), instead I/O is
handled via devices which are accessible via MMIO (or I/O ports for
x86).

For debugging, anything can be possible, but that's why I suggested
using DEBUG_UC32.

>
> Thanks & Regards,
>
> Guan Xuetao
>