Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 02:58:20AM +0200, Petar Jovanovic wrote:
From: Petar Jovanovic petar.jovano...@imgtec.com
From MIPS documentation (Volume III):
UserLocal Register (CP0 Register 4, Select 2)
Compliance Level: Recommended.
The UserLocal register is a read-write register that is not interpreted by
the hardware and conditionally readable via the RDHWR instruction.
This register only exists if the Config3-ULRI register field is set.
Privileged software may write this register with arbitrary information and
make it accessible to unprivileged software via register 29 (ULR) of the
RDHWR instruction. To do so, bit 29 of the HWREna register must be set to a
1 to enable unprivileged access to the register.
Signed-off-by: Petar Jovanovic petar.jovano...@imgtec.com
---
v3:
- new hflag MIPS_HFLAG_HWRENA_ULR introduced, it is set when ULR bit
from HWREna is set
- helper rdhwr_ul removed, now the checks for rdhwr are done at
translation time
- CPU_SAVE_VERSION switched to 4, load_tc supports both (3 and 4)
version ids
v2:
- Defined MIPS_HFLAG_CP0UL flag, checks are now based on hflags
- CP0_UserLocal moved to struct TCState
- Added tc-CP0_UserLocal in save_tc/load_tc in target-mips/machine.c
- Reused CP0_UserLocal field for user-mode purpose
linux-user/mips/target_cpu.h |2 +-
linux-user/syscall.c |2 +-
target-mips/cpu.h| 13 +++---
target-mips/machine.c| 13 +++---
target-mips/op_helper.c | 14 ++-
target-mips/translate.c | 55
+++---
6 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/linux-user/mips/target_cpu.h b/linux-user/mips/target_cpu.h
index ba8e9eb..19b8855 100644
--- a/linux-user/mips/target_cpu.h
+++ b/linux-user/mips/target_cpu.h
@@ -30,7 +30,7 @@ static inline void cpu_clone_regs(CPUMIPSState *env,
target_ulong newsp)
static inline void cpu_set_tls(CPUMIPSState *env, target_ulong newtls)
{
-env-tls_value = newtls;
+env-active_tc.CP0_UserLocal = newtls;
}
#endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6efeeff..fda8dd6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8686,7 +8686,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
#ifdef TARGET_NR_set_thread_area
case TARGET_NR_set_thread_area:
#if defined(TARGET_MIPS)
- ((CPUMIPSState *) cpu_env)-tls_value = arg1;
+ ((CPUMIPSState *) cpu_env)-active_tc.CP0_UserLocal = arg1;
ret = 0;
break;
#elif defined(TARGET_CRIS)
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 6c2014e..67bf441 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -167,6 +167,7 @@ struct TCState {
target_ulong CP0_TCSchedule;
target_ulong CP0_TCScheFBack;
int32_t CP0_Debug_tcstatus;
+target_ulong CP0_UserLocal;
};
typedef struct CPUMIPSState CPUMIPSState;
@@ -361,6 +362,7 @@ struct CPUMIPSState {
int32_t CP0_Config3;
#define CP0C3_M31
#define CP0C3_ISA_ON_EXC 16
+#define CP0C3_ULRI 13
#define CP0C3_DSPP 10
#define CP0C3_LPA 7
#define CP0C3_VEIC 6
@@ -469,6 +471,10 @@ struct CPUMIPSState {
/* MIPS DSP resources access. */
#define MIPS_HFLAG_DSP 0x4 /* Enable access to MIPS DSP resources. */
#define MIPS_HFLAG_DSPR2 0x8 /* Enable access to MIPS DSPR2 resources.
*/
+/* Extra flags about UserLocal register. */
+#define MIPS_HFLAG_CP0UL 0x10 /* CP0_UserLocal is implemented. */
+#define MIPS_HFLAG_HWRENA_ULR 0x20 /* ULR bit from HWREna is set. */
+#define MIPS_HFLAG_UL_MASK (MIPS_HFLAG_CP0UL | MIPS_HFLAG_HWRENA_ULR)
While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance
reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is
always the same (true or false) during all the run time of the
qemu-system-mips binary, and thus we don't need to take care of code
generated with it being true or being false.
target_ulong btarget;/* Jump / branch target */
target_ulong bcond; /* Branch condition (if needed) */
@@ -478,8 +484,6 @@ struct CPUMIPSState {
uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits in CP0_TCStatus */
int insn_flags; /* Supported instruction set */
-target_ulong tls_value; /* For usermode emulation */
-
CPU_COMMON
/* Fields from here on are preserved across CPU reset. */
@@ -522,7 +526,7 @@ void mips_cpu_list (FILE *f, fprintf_function
cpu_fprintf);
extern void cpu_wrdsp(uint32_t rs, uint32_t mask_num, CPUMIPSState *env);
extern uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env);
-#define CPU_SAVE_VERSION 3
+#define CPU_SAVE_VERSION 4
/* MMU modes definitions. We carefully match the indices with our
hflags layout. */
@@ -681,7 +685,8 @@ static inline void cpu_get_tb_cpu_state(CPUMIPSState
*env, target_ulong *pc,
{
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance
reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is
always the same (true or false) during all the run time of the
qemu-system-mips binary, and thus we don't need to take care of code
generated with it being true or being false.
Initial version of this patch did not have this flag, but it was added
as requested in code review.
What do you suggest?
From: Aurelien Jarno [aurel...@aurel32.net]
Sent: Thursday, May 29, 2014 2:49 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic; afaer...@suse.de; r...@twiddle.net
Subject: Re: [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 02:58:20AM +0200, Petar Jovanovic wrote:
From: Petar Jovanovic petar.jovano...@imgtec.com
From MIPS documentation (Volume III):
UserLocal Register (CP0 Register 4, Select 2)
Compliance Level: Recommended.
The UserLocal register is a read-write register that is not interpreted by
the hardware and conditionally readable via the RDHWR instruction.
This register only exists if the Config3-ULRI register field is set.
Privileged software may write this register with arbitrary information and
make it accessible to unprivileged software via register 29 (ULR) of the
RDHWR instruction. To do so, bit 29 of the HWREna register must be set to a
1 to enable unprivileged access to the register.
Signed-off-by: Petar Jovanovic petar.jovano...@imgtec.com
---
v3:
- new hflag MIPS_HFLAG_HWRENA_ULR introduced, it is set when ULR bit
from HWREna is set
- helper rdhwr_ul removed, now the checks for rdhwr are done at
translation time
- CPU_SAVE_VERSION switched to 4, load_tc supports both (3 and 4)
version ids
v2:
- Defined MIPS_HFLAG_CP0UL flag, checks are now based on hflags
- CP0_UserLocal moved to struct TCState
- Added tc-CP0_UserLocal in save_tc/load_tc in target-mips/machine.c
- Reused CP0_UserLocal field for user-mode purpose
linux-user/mips/target_cpu.h |2 +-
linux-user/syscall.c |2 +-
target-mips/cpu.h| 13 +++---
target-mips/machine.c| 13 +++---
target-mips/op_helper.c | 14 ++-
target-mips/translate.c | 55
+++---
6 files changed, 85 insertions(+), 14 deletions(-)
diff --git a/linux-user/mips/target_cpu.h b/linux-user/mips/target_cpu.h
index ba8e9eb..19b8855 100644
--- a/linux-user/mips/target_cpu.h
+++ b/linux-user/mips/target_cpu.h
@@ -30,7 +30,7 @@ static inline void cpu_clone_regs(CPUMIPSState *env,
target_ulong newsp)
static inline void cpu_set_tls(CPUMIPSState *env, target_ulong newtls)
{
-env-tls_value = newtls;
+env-active_tc.CP0_UserLocal = newtls;
}
#endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 6efeeff..fda8dd6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8686,7 +8686,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
#ifdef TARGET_NR_set_thread_area
case TARGET_NR_set_thread_area:
#if defined(TARGET_MIPS)
- ((CPUMIPSState *) cpu_env)-tls_value = arg1;
+ ((CPUMIPSState *) cpu_env)-active_tc.CP0_UserLocal = arg1;
ret = 0;
break;
#elif defined(TARGET_CRIS)
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 6c2014e..67bf441 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -167,6 +167,7 @@ struct TCState {
target_ulong CP0_TCSchedule;
target_ulong CP0_TCScheFBack;
int32_t CP0_Debug_tcstatus;
+target_ulong CP0_UserLocal;
};
typedef struct CPUMIPSState CPUMIPSState;
@@ -361,6 +362,7 @@ struct CPUMIPSState {
int32_t CP0_Config3;
#define CP0C3_M31
#define CP0C3_ISA_ON_EXC 16
+#define CP0C3_ULRI 13
#define CP0C3_DSPP 10
#define CP0C3_LPA 7
#define CP0C3_VEIC 6
@@ -469,6 +471,10 @@ struct CPUMIPSState {
/* MIPS DSP resources access. */
#define MIPS_HFLAG_DSP 0x4 /* Enable access to MIPS DSP resources. */
#define MIPS_HFLAG_DSPR2 0x8 /* Enable access to MIPS DSPR2 resources.
*/
+/* Extra flags about UserLocal register. */
+#define MIPS_HFLAG_CP0UL 0x10 /* CP0_UserLocal is implemented. */
+#define MIPS_HFLAG_HWRENA_ULR 0x20 /* ULR bit from HWREna is set. */
+#define MIPS_HFLAG_UL_MASK (MIPS_HFLAG_CP0UL | MIPS_HFLAG_HWRENA_ULR)
While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance
reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is
always the same (true or false) during all the run time of the
qemu-system-mips binary, and thus we don't need to take care of code
generated with it being true or being false.
target_ulong btarget;/* Jump / branch target */
target_ulong bcond; /* Branch condition (if needed) */
@@ -478,8 +484,6 @@ struct CPUMIPSState {
uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 01:01:38PM +, Petar Jovanovic wrote: While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is always the same (true or false) during all the run time of the qemu-system-mips binary, and thus we don't need to take care of code generated with it being true or being false. Initial version of this patch did not have this flag, but it was added as requested in code review. What do you suggest? Well maybe I missed something, but it looks to me the value can never change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29/05/14 14:08, Aurelien Jarno wrote: On Thu, May 29, 2014 at 01:01:38PM +, Petar Jovanovic wrote: While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is always the same (true or false) during all the run time of the qemu-system-mips binary, and thus we don't need to take care of code generated with it being true or being false. Initial version of this patch did not have this flag, but it was added as requested in code review. What do you suggest? Well maybe I missed something, but it looks to me the value can never change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. loadvm/migration will write Config3, possibly with a different value if the snapshot being loaded is an older one without userlocal support. Does tlb_flush() definitely get called in that case? I can't see how from a quick look, although memory will have been reloaded so perhaps that is sufficient to guarantee no old translations are lying around. By the way v3 looks reasonable to me, and seems to work (although I failed to test savevm/loadvm as it seemed to be already broken with the particular guest kernel I was using). Cheers James
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29 May 2014 14:22, James Hogan james.ho...@imgtec.com wrote: loadvm/migration will write Config3, possibly with a different value if the snapshot being loaded is an older one without userlocal support. Migration load always happens with a freshly reset system, so this isn't a problem. thanks -- PMM
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. [1] http://patchwork.ozlabs.org/patch/349709/ From: Aurelien Jarno [aurel...@aurel32.net] Sent: Thursday, May 29, 2014 3:08 PM To: Petar Jovanovic Cc: Petar Jovanovic; qemu-devel@nongnu.org; afaer...@suse.de; r...@twiddle.net Subject: Re: [v3 PATCH] target-mips: implement UserLocal Register On Thu, May 29, 2014 at 01:01:38PM +, Petar Jovanovic wrote: While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is always the same (true or false) during all the run time of the qemu-system-mips binary, and thus we don't need to take care of code generated with it being true or being false. Initial version of this patch did not have this flag, but it was added as requested in code review. What do you suggest? Well maybe I missed something, but it looks to me the value can never change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
Am 29.05.2014 15:35, schrieb Petar Jovanovic: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. That was saying you shouldn't add an env argument to the code generator function. Agree. If hflags is the wrong field, then you can add a new field to DisasContext based on env. Note that a while back we eliminated some env fields from other DisasContexts, so please don't just add an env field. Andreas [1] http://patchwork.ozlabs.org/patch/349709/ From: Aurelien Jarno [aurel...@aurel32.net] Sent: Thursday, May 29, 2014 3:08 PM To: Petar Jovanovic Cc: Petar Jovanovic; qemu-devel@nongnu.org; afaer...@suse.de; r...@twiddle.net Subject: Re: [v3 PATCH] target-mips: implement UserLocal Register On Thu, May 29, 2014 at 01:01:38PM +, Petar Jovanovic wrote: While adding MIPS_HFLAG_HWRENA_ULR is a good idea for performance reasons, I don't think we should add MIPS_HFLAG_CP0UL. This value is always the same (true or false) during all the run time of the qemu-system-mips binary, and thus we don't need to take care of code generated with it being true or being false. Initial version of this patch did not have this flag, but it was added as requested in code review. What do you suggest? Well maybe I missed something, but it looks to me the value can never change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 01:35:17PM +, Petar Jovanovic wrote: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. Well, I agree it should not be done to access things that will change at runtime, as in that case the TB might re-executed with different values later. That said in the case of CP0_Config3, it is only used to access a read-only value, so in my opinion it is something fine. It is something already done in plenty of places in this code. What we have there is more a philosophical issue, probably the best way to fix that is to have a copy of CP0_ConfigX in DisasContext, so that we don't explicitly access env anymore from the translator. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29 May 2014 14:35, Petar Jovanovic petar.jovano...@imgtec.com wrote: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. [1] http://patchwork.ozlabs.org/patch/349709/ The intention of that review comment is to say that the bulk of the translator code should not have access directly to env. This is because most of the fields of env are not safe to use as a basis for code generation decisions. Having direct acess to an env pointer makes it very easy to accidentally use a field that's not safe to use. Instead we prefer to set up a DisasContext which has only the required information in it, and pass that around. The DisasContext is initialised in gen_intermediate_code_internal from the TB flags and in some cases from env- fields where this is safe (for instancec ctx.insn_flags is a copy of env-insn_flags). This makes it easy to see at a glance what parts of env are being relied on by the code generation and when something new is added it's easy to see and check in code review. In this case we might choose to copy CP0_Config3 (or just the relevant flag from it) from env into ctx; I have no particular opinion there. thanks -- PMM
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 02:48:28PM +0100, Peter Maydell wrote: On 29 May 2014 14:35, Petar Jovanovic petar.jovano...@imgtec.com wrote: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. [1] http://patchwork.ozlabs.org/patch/349709/ The intention of that review comment is to say that the bulk of the translator code should not have access directly to env. This is because most of the fields of env are not safe to use as a basis for code generation decisions. Having direct acess to an env pointer makes it very easy to accidentally use a field that's not safe to use. Instead we prefer to set up a DisasContext which has only the required information in it, and pass that around. The DisasContext is initialised in gen_intermediate_code_internal from the TB flags and in some cases from env- fields where this is safe (for instancec ctx.insn_flags is a copy of env-insn_flags). This makes it easy to see at a glance what parts of env are being relied on by the code generation and when something new is added it's easy to see and check in code review. In this case we might choose to copy CP0_Config3 (or just the relevant flag from it) from env into ctx; I have no particular opinion there. It looks like one bit of CP0_Config3 is RW when microMIPS is implemented, so it might be better to copy only the corresponding bit. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On Thu, May 29, 2014 at 04:06:56PM +0200, Aurelien Jarno wrote: On Thu, May 29, 2014 at 02:48:28PM +0100, Peter Maydell wrote: On 29 May 2014 14:35, Petar Jovanovic petar.jovano...@imgtec.com wrote: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. [1] http://patchwork.ozlabs.org/patch/349709/ The intention of that review comment is to say that the bulk of the translator code should not have access directly to env. This is because most of the fields of env are not safe to use as a basis for code generation decisions. Having direct acess to an env pointer makes it very easy to accidentally use a field that's not safe to use. Instead we prefer to set up a DisasContext which has only the required information in it, and pass that around. The DisasContext is initialised in gen_intermediate_code_internal from the TB flags and in some cases from env- fields where this is safe (for instancec ctx.insn_flags is a copy of env-insn_flags). This makes it easy to see at a glance what parts of env are being relied on by the code generation and when something new is added it's easy to see and check in code review. In this case we might choose to copy CP0_Config3 (or just the relevant flag from it) from env into ctx; I have no particular opinion there. It looks like one bit of CP0_Config3 is RW when microMIPS is implemented, so it might be better to copy only the corresponding bit. I have just posted a patch copying CP0_Config1 into DisasContext, and moving existing accesses to this variable accordingly. This can be used as an example how to do it. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29/05/14 14:26, Peter Maydell wrote: On 29 May 2014 14:22, James Hogan james.ho...@imgtec.com wrote: loadvm/migration will write Config3, possibly with a different value if the snapshot being loaded is an older one without userlocal support. Migration load always happens with a freshly reset system, so this isn't a problem. Why does that make a difference? The snapshot may have been saved on a slightly different version without that bit set (if that isn't supported then we may as well not bother trying to support loading different snapshot versions). Also loadvm can happen at any time, not just after reset. Is this patch missing something to actually set that bit in Config3? Cheers James
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29 May 2014 15:29, James Hogan james.ho...@imgtec.com wrote: On 29/05/14 14:26, Peter Maydell wrote: On 29 May 2014 14:22, James Hogan james.ho...@imgtec.com wrote: loadvm/migration will write Config3, possibly with a different value if the snapshot being loaded is an older one without userlocal support. Migration load always happens with a freshly reset system, so this isn't a problem. Why does that make a difference? The snapshot may have been saved on a slightly different version without that bit set (if that isn't supported then we may as well not bother trying to support loading different snapshot versions). A key requirement of migration or vm restore is that the machine configuration (including all command line parameters) must be exactly identical at both ends. (We don't attempt to catch this user error, which typically results in weird stuff happening or in a migration failure with an obscure message). So you'll never get a migration from a bit-set config to a bit-cleared config. Note that this is different from a cross version migration, which would be from QEMU 2.0 with config-X to QEMU 2.1 with config-X. Also loadvm can happen at any time, not just after reset. The loadvm monitor command is implemented as first reset; then load state. thanks -- PMM
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
On 29/05/14 16:38, Peter Maydell wrote: On 29 May 2014 15:29, James Hogan james.ho...@imgtec.com wrote: On 29/05/14 14:26, Peter Maydell wrote: On 29 May 2014 14:22, James Hogan james.ho...@imgtec.com wrote: loadvm/migration will write Config3, possibly with a different value if the snapshot being loaded is an older one without userlocal support. Migration load always happens with a freshly reset system, so this isn't a problem. Why does that make a difference? The snapshot may have been saved on a slightly different version without that bit set (if that isn't supported then we may as well not bother trying to support loading different snapshot versions). A key requirement of migration or vm restore is that the machine configuration (including all command line parameters) must be exactly identical at both ends. (We don't attempt to catch this user error, which typically results in weird stuff happening or in a migration failure with an obscure message). So you'll never get a migration from a bit-set config to a bit-cleared config. Note that this is different from a cross version migration, which would be from QEMU 2.0 with config-X to QEMU 2.1 with config-X. Presumably we want to be able to add the bit to Config3 in existing machines in target-mips/translate_init.c at some point though to take advantage of the extra performance (e.g. in v2.1), in which case you could get a migration between bit-clear and bit-set... Also loadvm can happen at any time, not just after reset. The loadvm monitor command is implemented as first reset; then load state. Okay cool. It's fine then as long as there are no old translations lying around and the Config3 etc are migrated unchanged from the snapshot. Thanks James
Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register
I have just posted a patch copying CP0_Config1 into DisasContext, and moving existing accesses to this variable accordingly. This can be used as an example how to do it. Done in v4. Thanks. Regards, Petar From: Aurelien Jarno [aurel...@aurel32.net] Sent: Thursday, May 29, 2014 4:26 PM To: Peter Maydell Cc: Petar Jovanovic; r...@twiddle.net; Petar Jovanovic; afaer...@suse.de; qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [v3 PATCH] target-mips: implement UserLocal Register On Thu, May 29, 2014 at 04:06:56PM +0200, Aurelien Jarno wrote: On Thu, May 29, 2014 at 02:48:28PM +0100, Peter Maydell wrote: On 29 May 2014 14:35, Petar Jovanovic petar.jovano...@imgtec.com wrote: change as CP0_Config3 is a read-only register. If I am right, probably the best is to check directly env-CP0_Config3. If you take a look at v1 of the patch, that's what was done. In the code review, this was marked as unacceptable because it required [1] passing env within the translator. [1] http://patchwork.ozlabs.org/patch/349709/ The intention of that review comment is to say that the bulk of the translator code should not have access directly to env. This is because most of the fields of env are not safe to use as a basis for code generation decisions. Having direct acess to an env pointer makes it very easy to accidentally use a field that's not safe to use. Instead we prefer to set up a DisasContext which has only the required information in it, and pass that around. The DisasContext is initialised in gen_intermediate_code_internal from the TB flags and in some cases from env- fields where this is safe (for instancec ctx.insn_flags is a copy of env-insn_flags). This makes it easy to see at a glance what parts of env are being relied on by the code generation and when something new is added it's easy to see and check in code review. In this case we might choose to copy CP0_Config3 (or just the relevant flag from it) from env into ctx; I have no particular opinion there. It looks like one bit of CP0_Config3 is RW when microMIPS is implemented, so it might be better to copy only the corresponding bit. I have just posted a patch copying CP0_Config1 into DisasContext, and moving existing accesses to this variable accordingly. This can be used as an example how to do it. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurel...@aurel32.net http://www.aurel32.net
