Re: [Qemu-devel] [PATCH v3 5/6] target-mips: Adding support for Cavium specific instructions

[Andreas Färber]

Am 28.10.2011 06:42, schrieb Khansa Butt:
> 
> 
> On Sat, Oct 22, 2011 at 4:36 PM, Andreas Färber <address@hidden
> <mailto:address@hidden>> wrote:
> 
>     Am 22.10.2011 12:11, schrieb address@hidden
>     <mailto:address@hidden>:

HTML again :(

>     > From: Khansa Butt <address@hidden <mailto:address@hidden>>
> 
>     Commit message should mention here at least that new registers are
>     introduced and that load/save format is being changed.
> 
>     > Signed-off-by: Khansa Butt <address@hidden
>     <mailto:address@hidden>>
>     > Signed-off-by: Ehsan Ul Haq <address@hidden
>     <mailto:address@hidden>>
>     > Signed-off-by: Abdul Qadeer <address@hidden
>     <mailto:address@hidden>>
>     > Signed-off-by: Abdul Waheed <address@hidden
>     <mailto:address@hidden>>
>     > ---
> 
>     > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
>     > index 79e2558..9180ee9 100644
>     > --- a/target-mips/cpu.h
>     > +++ b/target-mips/cpu.h
>     > @@ -173,6 +173,13 @@ struct TCState {
>     >      target_ulong CP0_TCSchedule;
>     >      target_ulong CP0_TCScheFBack;
>     >      int32_t CP0_Debug_tcstatus;
>     > +    /* Multiplier registers for Octeon */
>     > +    target_ulong MPL0;
>     > +    target_ulong MPL1;
>     > +    target_ulong MPL2;
>     > +    target_ulong P0;
>     > +    target_ulong P1;
>     > +    target_ulong P2;
>     >  };
>     >
>     >  typedef struct CPUMIPSState CPUMIPSState;
> 
>     > diff --git a/target-mips/machine.c b/target-mips/machine.c
>     > index be72b36..a274ce2 100644
>     > --- a/target-mips/machine.c
>     > +++ b/target-mips/machine.c
>     > @@ -25,6 +25,12 @@ static void save_tc(QEMUFile *f, TCState *tc)
>     >      qemu_put_betls(f, &tc->CP0_TCSchedule);
>     >      qemu_put_betls(f, &tc->CP0_TCScheFBack);
>     >      qemu_put_sbe32s(f, &tc->CP0_Debug_tcstatus);
>     > +    qemu_put_betls(f, &tc->MPL0);
>     > +    qemu_put_betls(f, &tc->MPL1);
> 
>     MPL2 is not being saved but loaded below.
> 
>     > +    qemu_put_betls(f, &tc->P0);
>     > +    qemu_put_betls(f, &tc->P1);
>     > +    qemu_put_betls(f, &tc->P2);
>     > +
>     >  }
>     >
>     >  static void save_fpu(QEMUFile *f, CPUMIPSFPUContext *fpu)
>     > @@ -173,6 +179,12 @@ static void load_tc(QEMUFile *f, TCState *tc)
>     >      qemu_get_betls(f, &tc->CP0_TCSchedule);
>     >      qemu_get_betls(f, &tc->CP0_TCScheFBack);
>     >      qemu_get_sbe32s(f, &tc->CP0_Debug_tcstatus);
>     > +    qemu_get_betls(f, &tc->MPL0);
>     > +    qemu_get_betls(f, &tc->MPL1);
>     > +    qemu_get_betls(f, &tc->MPL2);
>     > +    qemu_get_betls(f, &tc->P0);
>     > +    qemu_get_betls(f, &tc->P1);
>     > +    qemu_get_betls(f, &tc->P2);
>     >  }
>     >
>     >  static void load_fpu(QEMUFile *f, CPUMIPSFPUContext *fpu)
> 
>     You're saving new fields, so you'll need to bump the version somewhere.
>     For loading, since you're adding at the end, you might be able to make
>     your additions conditional on the to-be-bumped version.
> 
> 
> I 'm not able to understand " bump the version somewhere"  kindly
> explain this.

"Somewhere" indicates I don't know the exact line for mips. Compare the
recent patch to arm_gic.
The general idea is that QEMU needs to be able to load files saved with
an older version, the file format is therefore versioned. If you
unconditionally try to load your new registers, you break loading older
files that don't include them.

>     I'm wondering whether those register and serialization additions could
>     and should be limited to TARGET_MIPS64.
> 
> you want me to limit these registers to TARGET_OCTEON

No, there shouldn't be a TARGET_OCTEON, there's no Octeon-specific
executable.

My point is, IIUC, qemu-system-mips will never have Octeon registers
because they're in qemu-system-mips64 only. So without #ifdef it would
save and load unused registers.

Andreas