[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, no
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState |
Date: |
Fri, 7 Jan 2011 17:01:17 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
Hi,
On Fri, Jan 07, 2011 at 03:06:27PM +0000, Peter Maydell wrote:
> This patchset corrects a number of places in the ARM translation code
> which were generating code which was dependent on values in the CPUState
> structure which might change at runtime. This is a bad idea for two
> reasons. Firstly, we might try to reuse the generated code later when
> the assumptions baked into the generated code were no longer valid.
> Secondly, we might try to retranslate the same TB (eg when an exception
> results in our calling cpu_restore_state()) but get different generated
> code, which could result in qemu crashing.
>
> Bug https://bugs.launchpad.net/bugs/604872 is a particular example
> of the latter case involving the IT bits; this patchset fixes that bug.
>
> I believe that this patchset deals with all the problems. Remaining
> CPUState fields referred to in translate.c are either constant after
> system init or trigger flushing of affected TBs when they are changed.
>
> Peter Maydell (7):
> target-arm: Don't generate code specific to current CPU mode for SRS
> target-arm: Translate with VFP-enabled from TB flags, not CPUState
> target-arm: Translate with VFP len/stride from TB flags, not CPUState
> target-arm: Translate with Thumb state from TB flags, not CPUState
> target-arm: Translate with condexec bits from TB flags, not CPUState
> target-arm: Set privileged bit in TB flags correctly for M profile
> target-arm: Translate with user-state from TB flags, not CPUState
>
> target-arm/cpu.h | 17 +++++++++-
> target-arm/helper.c | 12 +++++-
> target-arm/translate.c | 88 ++++++++++++++++++-----------------------------
> 3 files changed, 60 insertions(+), 57 deletions(-)
>
Commenting here as it concerns all patches.
In overall I think it's the correct approach to fix the issue, this is
a really good cleanup. I have tested this patch series, and it clearly
improve armv7 support. However I am surprised it doesn't fix the issue
mentioned in https://bugs.launchpad.net/qemu/+bug/581335 , which seems
to be the same issue. Executing the testcase still returns 1 instead of
0 on QEMU.
My other concern is about the definition of the individual bits in the
flags. I have seen that you have tried to summarize the usage in the
patch 6, but the masks and shifts are still duplicated in different
files, which may leads to mistakes if the flags definition are changed.
Have you considered using #define as for example in the MIPS target?
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net
- [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 6/7] target-arm: Set privileged bit in TB flags correctly for M profile, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 3/7] target-arm: Translate with VFP len/stride from TB flags, not CPUState, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 2/7] target-arm: Translate with VFP-enabled from TB flags, not CPUState, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 4/7] target-arm: Translate with Thumb state from TB flags, not CPUState, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 1/7] target-arm: Don't generate code specific to current CPU mode for SRS, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 5/7] target-arm: Translate with condexec bits from TB flags, not CPUState, Peter Maydell, 2011/01/07
- [Qemu-devel] [PATCH 7/7] target-arm: Translate with user-state from TB flags, not CPUState, Peter Maydell, 2011/01/07
- Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState,
Aurelien Jarno <=